XS RFR: 8006001: [parfait] Possible file leak in hotspot/src/os/linux/vm/perfMemory_linux.cpp

[Calvin Cheung]

David,

Thanks for your clarifications.

Zhengyu/Coleen,

Could you review the latest version of webrev?
http://cr.openjdk.java.net/~ccheung/8006001/webrev/

thanks,
Calvin

On 4/4/2013 7:35 PM, David Holmes wrote:
> To clarify a couple of things.
>
> 1. The change to use THROW_MSG_(..., OS_ERR) is semantically more 
> correct for the case where the caller does not use the CHECK macro. 
> (There is no such case in current code of course as there is only one 
> caller). The fd can not leak and parfait now seems silent, so this 
> aspect of the change is fine.
>
> 2. With regard to the initialization and use of "size" ... So ... this 
> code only has one caller PerfMemory::attach, which in turn only has 
> one caller, Perf_attach, which always has *sizep == 0. So an 
> uninitialized size is never seen. The fact that they test for 
> *sizep==0 indicates to me that someone thought it might have a 
> different value, but this is near enough day-one code. So I think my 
> suggested change is quite reasonable. With my change you don't need to 
> initialize size, but it doesn't hurt. And I agree the assert should 
> then be after the if/else block.
>
> Thanks,
> David
>
> On 5/04/2013 7:13 AM, David Holmes wrote:
>> On 5/04/2013 4:45 AM, Calvin Cheung wrote:
>>> On 4/3/2013 9:32 PM, David Holmes wrote:
>>>> This may be too late but ...
>>> Not too late as the fix hasn't been pushed yet. Some comments below...
>>>>
>>>> On 4/04/2013 3:42 AM, Calvin Cheung wrote:
>>>>> Fixing the following 2 warnings reported by the parfait tool:
>>>>>
>>>>> 1) fd leaks when ThreadShadow::has_pending_exception(((unresolved
>>>>> type*)__the_thread__)) is true at line 897
>>>>>      (I couldn't reproduce this warning with parfait even with the
>>>>> "--warning" option)
>>>>>
>>>>> Fix: instead of using THROW_MSG_0 which returns 0, use THROW_MSG_ 
>>>>> with
>>>>> return value of OS_ERR.
>>>>>         There's already an assert(fd != OS_ERR) after calling
>>>>> open_sharedmem_file().
>>>>
>>>> I don't understand the problem or the fix. Whether we use THROW_MSG_0
>>>> or THROW_MSG_(..., OS_ERR) line 897:
>>>>
>>>> 897   fd = open_sharedmem_file(rfilename, file_flags, CHECK);
>>>>
>>>> is simply going to trigger a return due to the CHECK macro. And if we
>>>> posted an exception it means the ::open failed and so we can not
>>>> possibly be leaking a fd.
>>>>
>>>> ???
>>> As I mentioned that I couldn't reproduce the warning as stated in the
>>> bug report.
>>> My fix is to address the usage of THROW_MSG_0, it returns 0. So if
>>> open_sharedmem_file() fails, it'll return 0 instead of OS_ERR and the
>>> assert(fd != OS_ERR) won't be triggered.
>>> The windows version correctly uses THROW_MSG_.
>>
>> The value returned is irrelevant to the assert as we return before that:
>>
>>    fd = open_sharedmem_file(rfilename, file_flags, CHECK);
>>    assert(fd != OS_ERR, "unexpected value");
>>
>> expands to
>>
>>    fd = open_sharedmem_file(rfilename, file_flags, THREAD); if
>> (HAS_PENDING_EXCEPTION) return       ; (0);
>>    assert(fd != OS_ERR, "unexpected value");
>>
>>>>> 2) Possible uninitialised variable (CWE 457): Uninitialised variable
>>>>> 'size' may be used as argument 2 when calling mmap at line 905 of
>>>>> src/os/linux/vm/perfMemory_linux.cpp
>>>>>
>>>>> Fix: add an assert(size > 0) before calling ::mmap().
>>>>
>>>> Again I don't understand the fix. It seems to me this current code is
>>>> broken and that really we should have:
>>>>
>>>>  900   if (*sizep == 0) {
>>>>  901     size = sharedmem_filesize(fd, CHECK);
>>>>  902   }
>>>>        else {
>>>>          size = *sizep;
>>>>        }
>>> I've included this suggestion in my fix.
>>> The windows version also needs fixing.
>>
>> Have you verified that my suggestion actually reflects the intended 
>> usage?
>>
>> Thanks,
>> David
>> -----
>>
>>>>
>>>> The initialization to zero might fix parfait but the code still seems
>>>> wrong to me.
>>> I ran parfait with the fix and the warning was gone.
>>>
>>> I've an updated webrev at:
>>>      http://cr.openjdk.java.net/~ccheung/8006001/webrev/
>>>
>>> Will submit a jprt test run soon.
>>>
>>> Calvin
>>>>
>>>> David
>>>> -----
>>>>
>>>>> webrev: http://http://cr.openjdk.java.net/~ccheung/8006001/
>>>>> <http://cr.openjdk.java.net/%7Eccheung/8006001/>
>>>>> bug: https://jbs.oracle.com/bugs/browse/JDK-8006001
>>>>>
>>>>> Tests:
>>>>>      JPRT
>>>>>      parfait
>>>>>
>>>>> thanks,
>>>>> Calvin
>>>