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

[Calvin Cheung]

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

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