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

[David Holmes]

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