RFR [8022584] Memory leak in some NetworkInterface methods

[David Holmes]

Thumbs up!

Thanks,
David

On 9/08/2013 8:19 AM, Ivan Gerasimov wrote:
> 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
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>
>>>>>>
>>>>>
>>>>
>>>>
>>>
>>
>>
>