RFR [8022584] Memory leak in some NetworkInterface methods
Chris Hegarty
chris.hegarty at oracle.com
Fri Aug 9 03:36:22 PDT 2013
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.
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].
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.
I would really like to get a definitive answer on the JNI specification
for GetStringUTFChars before making any changes here.
-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