RFR [8022584] Memory leak in some NetworkInterface methods

[Ivan Gerasimov]

Thanks David!

On 09.08.2013 1:15, David Holmes wrote:
> Main fix looks good to me.
>
> Regression test may need some tweaking eg I think othervm will be needed.
>
Yes, it's a good point.
Since there may be a memory leak in the test, it'd better not interfere 
with other tests in jtreg.

> Also this:
>
>  System.out.println("WARNING: Cannot perform memory leak detection on 
> this OS");
>
> should probably just say something like "Test skipped on this OS" - 
> there are other tests that do this so just check if there is common 
> terminology. (In the future we may have keyword tags to flag this as 
> linux only etc.)
>
OK, I changed the phrase to "Test only runs on Linux".

Updated webrev is here:
http://cr.openjdk.java.net/~igerasim/8022584/4/webrev/

Updated export is at the same place:
http://cr.openjdk.java.net/~igerasim/2commit/8022584-jdk8-Memleak-in-NetworkInterface.patch

Sincerely yours,
Ivan


> Thanks,
> David
>
> On 9/08/2013 4:41 AM, Ivan Gerasimov wrote:
>> Chris, if it's not too late, I'd like to include a regtest in the fix.
>>
>> Here's webrev that includes the test:
>> http://cr.openjdk.java.net/~igerasim/8022584/3/webrev/
>>
>> The test gets past with the fixed jdk8 and fails with jdk8-b101 and jdk7
>> as expected.
>>
>> Sincerely yours,
>> Ivan
>>
>> On 08.08.2013 17:50, Chris Hegarty wrote:
>>> Looks good to me. Thanks Ivan.
>>>
>>> -Chris.
>>>
>>> On 08/08/2013 02:33 PM, Ivan Gerasimov wrote:
>>>> Hello Chris!
>>>>
>>>> Here's the update:
>>>> http://cr.openjdk.java.net/~igerasim/8022584/2/webrev/
>>>>
>>>> Thanks for offering the sponsorship!
>>>> Here's the hg-export
>>>> http://cr.openjdk.java.net/~igerasim/2commit/8022584-jdk8-Memleak-in-NetworkInterface.patch 
>>>>
>>>>
>>>>
>>>>
>>>> Sincerely yours,
>>>> Ivan
>>>>
>>>> On 08.08.2013 12:43, Chris Hegarty wrote:
>>>>> Thanks for taking this Ivan.
>>>>>
>>>>> Can you please make the changes suggested by both David and Alan (
>>>>> simply return NULL/-1/JNI_FALSE, as appropriate, if GetStringUTFChars
>>>>> fails ( returns NULL ), then I will sponsor this change into jdk8 for
>>>>> you.
>>>>>
>>>>> Please post an update webrev to cr.openjdk.java.net.
>>>>>
>>>>> -Chris.
>>>>>
>>>>> On 08/08/2013 06:01 AM, David Holmes wrote:
>>>>>> Ivan,
>>>>>>
>>>>>> On 8/08/2013 2:05 PM, Ivan Gerasimov wrote:
>>>>>>> David, Alan,
>>>>>>>
>>>>>>> I added checking for NULL  results and throwing OOMException if
>>>>>>> necessary.
>>>>>>
>>>>>> You don't need to throw it yourself:
>>>>>>
>>>>>>    JNU_ThrowOutOfMemoryError(env, NULL);
>>>>>>
>>>>>> Assuming a correct VM implementation if NULL is returned then an 
>>>>>> OOME
>>>>>> should already be pending and will be thrown as soon as your native
>>>>>> code
>>>>>> returns to Java.
>>>>>>
>>>>>> Thanks,
>>>>>> David
>>>>>>
>>>>>>> I've also added some spaces along the code to improve indentation.
>>>>>>>
>>>>>>> Would you please review the updated webrev?
>>>>>>> http://washi.ru.oracle.com/~igerasim/webrevs/8022584/1/webrev/
>>>>>>>
>>>>>>> Sincerely yours,
>>>>>>> Ivan
>>>>>>>
>>>>>>>
>>>>>>> On 08.08.2013 5:35, David Holmes wrote:
>>>>>>>> On 8/08/2013 11:20 AM, Ivan Gerasimov wrote:
>>>>>>>>> Thanks Alan!
>>>>>>>>>
>>>>>>>>> I've checked that and it turns out that GetStringUTFChars cannot
>>>>>>>>> return
>>>>>>>>> NULL.
>>>>>>>>> For allocation of the result string it calls AllocateHeap() with
>>>>>>>>> the
>>>>>>>>> default EXIT_OOM fail strategy.
>>>>>>>>> Thus, in case of being unable to allocate memory it simply stops
>>>>>>>>> VM.
>>>>>>>>
>>>>>>>> Ouch! That is a hotspot bug. GetStringUTFChars is supposed to 
>>>>>>>> throw
>>>>>>>> OutOfMemoryError if it has memory issues, not abort the VM!
>>>>>>>>
>>>>>>>> I recommend that you check for NULL and/or a pending exception.
>>>>>>>>
>>>>>>>> David
>>>>>>>>
>>>>>>>>> Sincerely yours,
>>>>>>>>> Ivan
>>>>>>>>>
>>>>>>>>> On 08.08.2013 5:05, Alan Bateman wrote:
>>>>>>>>>> (cc'ing net-dev).
>>>>>>>>>>
>>>>>>>>>> The change looks okay to me. One suggestion (while you are
>>>>>>>>>> there) is
>>>>>>>>>> to check the return from  GetStringUTFChars so that the name
>>>>>>>>>> returns
>>>>>>>>>> when it fails with NULL.
>>>>>>>>>>
>>>>>>>>>> -Alan.
>>>>>>>>>>
>>>>>>>>>> On 07/08/2013 17:46, Ivan Gerasimov wrote:
>>>>>>>>>>> Hello everybody!
>>>>>>>>>>>
>>>>>>>>>>> Some methods of NetworkInterface class were reported to leak
>>>>>>>>>>> memory.
>>>>>>>>>>> http://bugs.sun.com/view_bug.do?bug_id=JDK-8022584 (not yet
>>>>>>>>>>> available.)
>>>>>>>>>>>
>>>>>>>>>>> Examples are isUp() and isLoopback().
>>>>>>>>>>>
>>>>>>>>>>> Affected versions of jdk are 6, 7 and 8
>>>>>>>>>>>
>>>>>>>>>>> Would you please review a simple fix that removes the 
>>>>>>>>>>> unnecessary
>>>>>>>>>>> allocation?
>>>>>>>>>>> http://cr.openjdk.java.net/~igerasim/8022584/0/webrev/
>>>>>>>>>>>
>>>>>>>>>>> Sincerely yours,
>>>>>>>>>>> Ivan
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>
>>>>>
>>>>
>>>
>>>
>>
>
>