RFR [8022584] Memory leak in some NetworkInterface methods
Ivan Gerasimov
ivan.gerasimov at oracle.com
Mon Aug 12 05:44:26 PDT 2013
Thank you Chris!
On 12.08.2013 16:43, Chris Hegarty wrote:
> Thank you Ivan. This looks good to me.
>
> -Chris.
>
> P.S. I will give others a chance to comment. If no objections, I will
> push this tomorrow for you.
>
> On 12/08/2013 13:33, Ivan Gerasimov wrote:
>> David, Chris,
>>
>> I reverted back NULL-checking.
>> Now the change consists of one line removal and a regression test.
>>
>> Webrev: http://cr.openjdk.java.net/~igerasim/8022584/6/webrev/
>> Hg export:
>> http://cr.openjdk.java.net/~igerasim/2commit/8022584-jdk8-Memleak-in-NetworkInterface.patch
>>
>>
>>
>> Sincerely yours,
>> Ivan
>>
>> On 09.08.2013 16:18, David Holmes wrote:
>>> Hi Chris,
>>>
>>> On 9/08/2013 8:36 PM, Chris Hegarty wrote:
>>>> Firstly, I think the memory leak issue should be moved forward
>>>> separately to this cleanup effort. They are unrelated, and I'm
>>>> starting
>>>> to get the feeling that this could take some time to reach conclusion.
>>>> It seems reasonable to separate the issues.
>>>
>>> I agree. I'm sure when Alan suggested to check the return he didn't
>>> expect it to unravel like this :) As we know hotspot will never
>>> actually return NULL there is no urgency to add this in.
>>>
>>>> On 09/08/2013 10:27, Ivan Gerasimov wrote:
>>>>> 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.
>>>>
>>>> JNU_ThrowOutOfMemoryError is simply a convenience wrapper for
>>>> JNU_ThrowByName(env, "java/lang/OutOfMemoryError", msg);
>>>>
>>>> ...and JNU_ThrowByName [1] is defined as...
>>>>
>>>> JNU_ThrowByName(JNIEnv *env, const char *name, const char *msg) {
>>>> class cls = (*env)->FindClass(env, name);
>>>> if (cls != 0) /* Otherwise an exception has already been thrown */
>>>> (*env)->ThrowNew(env, cls, msg);
>>>> }
>>>> }
>>>>
>>>> Neither FindClass or ThrowNew is safe to call if there is a pending
>>>> exception [1].
>>>
>>> Right - we have to check for a pending exception before trying to
>>> throw one.
>>>
>>>> Now the issue comes down to; could there ever be a pending
>>>> exception if
>>>> GetStringUTFChars returns NULL? The latest specs doesn't indicate that
>>>> there could be, but every copy of "The Java Native Interface
>>>> Programmer's Guide and Specification" I can find does. There also
>>>> appears to be an assumption of this if you look at the usages in the
>>>> JDK.
>>>
>>> AFAIK there is only one version of the JNI spec book and it never got
>>> updated. The official spec says no throw, but when people have the
>>> book on their bookshelf that is what they tend to rely on. I looked at
>>> some of the usages and they seem exception agnostic - many of them
>>> don't even check for NULL :(
>>>
>>> The implementation as it stands will not throw and will not return
>>> NULL.
>>>
>>>> I would really like to get a definitive answer on the JNI
>>>> specification
>>>> for GetStringUTFChars before making any changes here.
>>>
>>> The JNI spec (as opposed to the book) is definitive. If we don't like
>>> what is there then it requires a spec change.
>>>
>>> I can't find any reference to this particular issue being raised
>>> before.
>>>
>>> Cheers,
>>> David
>>>
>>>> -Chris.
>>>>
>>>> [1]
>>>> http://hg.openjdk.java.net/jdk8/tl/jdk/file/54f0ccdd9ad7/src/share/native/common/jni_util.c
>>>>
>>>>
>>>>
>>>> [2]
>>>> http://docs.oracle.com/javase/6/docs/technotes/guides/jni/spec/design.html#wp17626
>>>>
>>>>
>>>>
>>>>
>>>>>
>>>>> 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