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

[Dmitry Samersoff]

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 ...