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

[Zhengyu Gu]

Good to me.

-Zhengyu

On 4/5/2013 12:36 PM, Calvin Cheung wrote:
> 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
>>>>
>