RFR [8022584] Memory leak in some NetworkInterface methods

[David Holmes]

Sorry I messed this up. The JNI book says GetStringUTFChars will return 
NULL and post OOME but the JNI spec (latest version 6.0) does not - it 
only says it will return NULL on failure.

So your previous version was the more correct. Given we just failed to 
allocate C-heap I think we are on thin ice anyway, but better to at 
least attempt to do the right thing.

FYI I filed 8022683 to fix GetStringUTFChars.

David
-----

On 9/08/2013 3:21 PM, David Holmes wrote:
> 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
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>>
>>>>
>>>
>>>
>>