RFR [8022584] Memory leak in some NetworkInterface methods
Ivan Gerasimov
ivan.gerasimov at oracle.com
Fri Aug 9 02:27:15 PDT 2013
Chris,
I would use this
if ((name_utf = (*env)->GetStringUTFChars(env, name, &isCopy)) == NULL) {
JNU_ThrowOutOfMemoryError(env, "GetStringUTFChars failed");
return NULL/JNI_False/-1;
}
If I understand it correctly, JNU_ThrowOutOfMemoryError throws an
exception only if it hasn't been already thrown.
Sincerely yours,
Ivan
On 09.08.2013 11:25, Chris Hegarty wrote:
> On 09/08/2013 06:47, David Holmes wrote:
>> 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.
>
> This is indeed strange. Most usages of this function in the jdk expect
> the former. If this is not the case, then we may need to do an audit
> of all usages.
>
>> 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.
>
> I'm not sure what the right thing to do here is? It seems a little
> unwieldy!
>
> if ((name_utf = (*env)->GetStringUTFChars(env, name, &isCopy)) ==
> NULL) {
> if ((*env)->ExceptionOccurred(env)) {
> return NULL/JNI_False/-1;
> } else {
> throwException(env, "java/lang/InternalError",
> "GetStringUTFChars failed");
> return NULL/JNI_False/-1;
> }
>
> Given we have no idea why GetStringUTFChars may have failed, what
> exception do we throw?
>
> Also worth noting is that this bug fix has moved away from the
> original problem (memory leak), and is now focused on code cleanup.
>
> If we cannot get agreement on the cleanup, or it looks like more
> clarification is needed around the cleanup effort, then I would like
> to suggest that we proceed with the original fix for the memory leak
> and separate out the cleanup effort.
>
> -Chris.
>
>> 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
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>>
>>>>
>
>
More information about the net-dev
mailing list