RFR [8022584] Memory leak in some NetworkInterface methods
Ivan Gerasimov
ivan.gerasimov at oracle.com
Mon Aug 12 05:33:48 PDT 2013
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