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

[David Holmes]

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.

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

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