Request for review (S, UPDATED) 7017193: Small memory leak in get_stack_bounds // os::create_stack_guard_pages

[Kelly O'Hair]

Everyone should favor using the string functions that include a length  
argument.
e.g. snprintf over sprintf, strnstr over strstr, strncat over strcat,  
etc.

Static analysis tools will often call these 'potential errors' and  
adding more
occurrences just adds to the pile of errors these tools will create  
when run over
the code.

Just an FYI.

-kto

On Feb 25, 2011, at 4:30 AM, Dmitry Samersoff wrote:

> On 2011-02-25 14:50, David Holmes wrote:
>> Dmitry Samersoff said the following on 02/25/11 17:48:
>>> I'm strictly against reading /proc entry using stdio functions,
>>> as (a) /proc file could be changed while we are reading it (b) it's
>>> not fancy as we are buffering kmem.
>>
>> ??? I'm not sure what you are referring to. The sscanf doesn't read  
>> the
>> /proc file it parses the buffer you've already read.
>
> Opps, sorry, posted to a wrong thread.  Yes, we can use sscanf to do  
> the same - it's just a matter of preference.
>
> So if you believe sscanf is better than strtoul - I'll change it.
>
>>> /proc/self/maps has a fixed format and if we have a bad line in a
>>> /proc file it would mean that something fatal happens with system,  
>>> but
>>> I'll change assert to "return false" so the code will silently bail
>>> out in case of malformed line.
>>
>> Ok. But also think about this from the perspective of a tool, like
>> Parfait, analysing this code.
>
> I have no experience with such tools besides an old lint. I'll check  
> it.
>
> -Dmitry
>
>
>>
>> David
>>
>>>
>>> -Dmitry
>>>
>>>
>>> On 2011-02-25 01:07, David Holmes wrote:
>>>> 2662 // Address part of /proc/maps couldn't be more than 128 bytes
>>>> 2663 while ( os::get_line_chars(fd, buf, 128) > 0) {
>>>> 2664 if (::strstr(buf, "[stack]") != 0) {
>>>> 2665 // Extract addresses
>>>> 2666 *bottom = ::strtoul(buf, &np, 16);
>>>> 2667 assert(*np == '-', "Should be a dash");
>>>> 2668 *top = ::strtoul(np + 1, 0, 16);
>>>>
>>>> Don't you need to verify that you actually read data into the  
>>>> parts of
>>>> the buf you're attempting to convert. The line might be malformed  
>>>> for
>>>> some reason, or truncated. And why not just keep the sscanf code?
>>>
>>>
>>>
>>>>
>>>> David
>>>>
>>>> Dmitry Samersoff said the following on 02/25/11 07:24:
>>>>> [got mailer error for openjdk alias, so resending]
>>>>>
>>>>> Hi Everyone,
>>>>>
>>>>> Here is updated fix:
>>>>>
>>>>> http://cr.openjdk.java.net/~dsamersoff/7017193/webrev.03/
>>>>>
>>>>> I introduce a new convenience function primary designed to read
>>>>> variety of /proc files. It reads first N characters of line and  
>>>>> then
>>>>> skip until eol.
>>>>>
>>>>> Than get_stack_bounds() is changed to use it instead of stdio
>>>>> getline();
>>>>>
>>>>> -Dmitry
>>>>>
>>>
>>>
>
>
> -- 
> Dmitry Samersoff
> Java Hotspot development team, SPB04
> * There will come soft rains ...