RFR [8022584] Memory leak in some NetworkInterface methods
David Holmes
david.holmes at oracle.com
Mon Aug 12 22:33:04 PDT 2013
Thanks Ivan.
David
On 12/08/2013 10:33 PM, 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