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

[David Holmes]

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
>