Re: RFR [8022584] Memory leak in some NetworkInterface methods
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
hg: jdk8/tl/langtools: 8019486: javac, generates erroneous LVT for a test case with lambda code
Changeset: b8610a65fbf9 Author:vromero Date: 2013-08-08 11:49 +0100 URL: http://hg.openjdk.java.net/jdk8/tl/langtools/rev/b8610a65fbf9 8019486: javac, generates erroneous LVT for a test case with lambda code Reviewed-by: mcimadamore ! src/share/classes/com/sun/tools/javac/comp/LambdaToMethod.java + test/tools/javac/T8019486/WrongLVTForLambdaTest.java
hg: jdk8/tl/jdk: 8016594: Native Windows ccache still reads DES tickets
Changeset: b7d594716f86 Author:weijun Date: 2013-08-08 21:13 +0800 URL: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/b7d594716f86 8016594: Native Windows ccache still reads DES tickets Reviewed-by: dsamersoff, xuelei ! src/share/classes/sun/security/krb5/Credentials.java ! src/share/native/sun/security/krb5/nativeccache.c ! src/windows/native/sun/security/krb5/NativeCreds.c
Re: RFR [8022584] Memory leak in some NetworkInterface methods
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
Re: RFR [8022584] Memory leak in some NetworkInterface methods
The patch looks good to me. I guess a regression test isn't feasible. So, the bug will be tagged noreg-hard Michael On 08/08/13 14:39, Ivan Gerasimov wrote: Thanks, David I've updated the webrev http://cr.openjdk.java.net/~igerasim/8022584/2/webrev/. On 08.08.2013 9:01, 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. It seemed to me that, JNU_ThrowOutOfMemoryError only throws an exception in a case one is not pending. But I don't mind to remove it, relaying on the correct implementation of GetStringUTFChars. Sincerely yours, Ivan 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
Re: RFR [8022584] Memory leak in some NetworkInterface methods
Thank you Michael! I'm working on the test. Chris, if it's not too late, I would like to include a regtest into the change. It will be ready in a few minutes and I'll send an updated webrev. Thanks, Ivan On 08.08.2013 17:51, Michael McMahon wrote: The patch looks good to me. I guess a regression test isn't feasible. So, the bug will be tagged noreg-hard Michael On 08/08/13 14:39, Ivan Gerasimov wrote: Thanks, David I've updated the webrev http://cr.openjdk.java.net/~igerasim/8022584/2/webrev/. On 08.08.2013 9:01, 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. It seemed to me that, JNU_ThrowOutOfMemoryError only throws an exception in a case one is not pending. But I don't mind to remove it, relaying on the correct implementation of GetStringUTFChars. Sincerely yours, Ivan 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
Re: RFR [8022584] Memory leak in some NetworkInterface methods
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
Re: RFR [8022584] Memory leak in some NetworkInterface methods
Main fix looks good to me. Regression test may need some tweaking eg I think othervm will be needed. 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.) 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
Re: RFR [8022584] Memory leak in some NetworkInterface methods
Yes, definitely othervm would be required for the test. Also, why skip other OS'es? The fix is only for Linux, but it might catch future leaks on Windows. The trouble with tests like this, is they sometimes don't age well. Future changes in OS kernel behavior could cause problems but I guess 32MB is a fairly large difference. So, it should be okay Michael On 08/08/13 22:15, David Holmes wrote: Main fix looks good to me. Regression test may need some tweaking eg I think othervm will be needed. 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.) 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
Re: RFR [8022584] Memory leak in some NetworkInterface methods
Michael, The test uses /proc/self/stat file to detect changes in virtual memory usage. This approach is specific for Linux. That's why I included the check of OS in the test. Sincerely yours, Ivan On 09.08.2013 1:38, Michael McMahon wrote: Yes, definitely othervm would be required for the test. Also, why skip other OS'es? The fix is only for Linux, but it might catch future leaks on Windows. The trouble with tests like this, is they sometimes don't age well. Future changes in OS kernel behavior could cause problems but I guess 32MB is a fairly large difference. So, it should be okay Michael On 08/08/13 22:15, David Holmes wrote: Main fix looks good to me. Regression test may need some tweaking eg I think othervm will be needed. 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.) 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
Re: RFR [8022584] Memory leak in some NetworkInterface methods
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
Re: RFR [8022584] Memory leak in some NetworkInterface methods
Ivan, Right, it's not worth trying to do the equivalent, whatever it is, for Windows. The test is fine with me. Thanks Michael On 08/08/13 23:15, Ivan Gerasimov wrote: Michael, The test uses /proc/self/stat file to detect changes in virtual memory usage. This approach is specific for Linux. That's why I included the check of OS in the test. Sincerely yours, Ivan On 09.08.2013 1:38, Michael McMahon wrote: Yes, definitely othervm would be required for the test. Also, why skip other OS'es? The fix is only for Linux, but it might catch future leaks on Windows. The trouble with tests like this, is they sometimes don't age well. Future changes in OS kernel behavior could cause problems but I guess 32MB is a fairly large difference. So, it should be okay Michael On 08/08/13 22:15, David Holmes wrote: Main fix looks good to me. Regression test may need some tweaking eg I think othervm will be needed. 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.) 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
Re: RFR [8022584] Memory leak in some NetworkInterface methods
Thank you Michael! On 09.08.2013 2:19, Michael McMahon wrote: Ivan, Right, it's not worth trying to do the equivalent, whatever it is, for Windows. The test is fine with me. Thanks Michael On 08/08/13 23:15, Ivan Gerasimov wrote: Michael, The test uses /proc/self/stat file to detect changes in virtual memory usage. This approach is specific for Linux. That's why I included the check of OS in the test. Sincerely yours, Ivan On 09.08.2013 1:38, Michael McMahon wrote: Yes, definitely othervm would be required for the test. Also, why skip other OS'es? The fix is only for Linux, but it might catch future leaks on Windows. The trouble with tests like this, is they sometimes don't age well. Future changes in OS kernel behavior could cause problems but I guess 32MB is a fairly large difference. So, it should be okay Michael On 08/08/13 22:15, David Holmes wrote: Main fix looks good to me. Regression test may need some tweaking eg I think othervm will be needed. 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.) 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
Re: Code review request, 8020842 IDN do not throw IAE when hostname ends with a trailing dot
Ping. Thanks, Xuelei On 8/7/2013 11:17 PM, Xuelei Fan wrote: Please review the new update: http://cr.openjdk.java.net./~xuelei/8020842/webrev.01/ With this update, com. is valid (return com.); . and example..com are invalid. And IAE will be thrown for invalid IDN. Thanks, Xuelei On 8/7/2013 10:18 PM, Michael McMahon wrote: On 07/08/13 15:13, Xuelei Fan wrote: On 8/7/2013 10:05 PM, Michael McMahon wrote: Resolvers seem to accept queries using trailing dots. eg nslookup www.oracle.com. or InetAddress.getByName(www.oracle.com.); The part of RFC3490 quoted below seems to me to be saying that the empty label implied by the trailing dot is not regarded as a label so that you don't end up calling toAscii() or toUnicode() with an empty string. I don't think it's saying the trailing dot can't be there. It makes sense. What's your preference to return for IDN.toASCII(www.oracle.com.), www.oracle.com. or www.oracle.com? The current returned value is www.oracle.com. I would like to reserve the behavior in this update. My opinion is to keep it as at present ie. www.oracle.com. Michael I think we are on same page soon. Thanks, Xuelei Michael On 07/08/13 13:44, Xuelei Fan wrote: On 8/7/2013 12:06 AM, Matthew Hall wrote: Trailing dots are allowed in plain DNS (thus almost surely in IDN), and the single dot represents the root zone. So you have to be careful making this sort of change to check the DNS RFCs first. That's the first question we need to answer, whether IDN allow tailling dots (com.), zero-length root label (.), and zero-length label (, for example example..com)? Per the specification of IDN.toASCII(): === ToASCII operation can fail. ToASCII fails if any step of it fails. If ToASCII operation fails, an IllegalArgumentException will be thrown. In this case, the input string should not be used in an internationalized domain name. A label is an individual part of a domain name. The original ToASCII operation, as defined in RFC 3490, only operates on a single label. This method can handle both label and entire domain name, by assuming that labels in a domain name are always separated by dots. ... Throws IllegalArgumentException - if the input string doesn't conform to RFC 3490 specification Per the specification of RFC 3490: == [section 2] A label is an individual part of a domain name. Labels are usually shown separated by dots; for example, the domain name www.example.com is composed of three labels: www, example, and com. (The zero-length root label described in [STD13], which can be explicit as in www.example.com. or implicit as in www.example.com, is not considered a label in this specification.) An internationalized label is a label to which the ToASCII operation (see section 4) can be applied without failing (with the UseSTD3ASCIIRules flag unset). ... Although most Unicode characters can appear in internationalized labels, ToASCII will fail for some input strings, and such strings are not valid internationalized labels. An internationalized domain name (IDN) is a domain name in which every label is an internationalized label. [Section 4.1] ToASCII consists of the following steps: ... 8. Verify that the number of code points is in the range 1 to 63 inclusive. Here are the questions: 1. whether example..com is an valid IDN? As dot is used as label separators, there are three labels, example, , com. Per RFC 3490, is not a valid label. Hence, example..com is not a valid IDN. We need to address the issue in IDN. 2. whether xyz. is an valid IDN? It's an gray area, I think. We can treat the trailing . as root label, or a label separator. If the trailing . is treated as label separator, xyz. is invalid per RFC 3490. if the trailing . is treated as root label, what's the expected return value of IDN.toASCII(xyz.)? I think the return value can be either xyz. or xyz. The current implementation returns xyz. We may need not to update the implementation if tailing . is treated as root label. 3. whether . is an valid IDN? It's an gray area again, I think. As above, if the trailing . is treated as root label, I think the return value can be either . or . The current implementation throws a StringIndexOutOfBoundsException. However, what empty domain name () really means? I would prefer to return . for . instead. We need to address the issue in IDN. Here comes the solution, the IDN.toASCII() returns: 1. . for .; 2. xyz for xyz.; 3. IAE for example..com. Does it make sense? Thanks, Xuelei On 8/7/2013 1:35 AM, Michael McMahon wrote: I don't really understand the reason for the restriction in SNIHostName But, I guess that is where it should be enforced if it is required. Michael. On 06/08/13 17:43,
Re: Code review request, 8020842 IDN do not throw IAE when hostname ends with a trailing dot
I tried nslookup. Those with .. inside are illegal, $ nslookup com.. nslookup: 'com..' is not a legal name (empty label) but $ nslookup . Server: 192.168.10.1 Address:192.168.10.1#53 Non-authoritative answer: *** Can't find .: No answer Also, since this bug was originally about SNIHostName, do you need to add some extra restriction there to reject oracle.com. things? Thanks Max On 8/9/13 8:41 AM, Xuelei Fan wrote: Ping. Thanks, Xuelei On 8/7/2013 11:17 PM, Xuelei Fan wrote: Please review the new update: http://cr.openjdk.java.net./~xuelei/8020842/webrev.01/ With this update, com. is valid (return com.); . and example..com are invalid. And IAE will be thrown for invalid IDN. Thanks, Xuelei
Re: Code review request, 8020842 IDN do not throw IAE when hostname ends with a trailing dot
On 8/9/2013 9:22 AM, Weijun Wang wrote: I tried nslookup. Those with .. inside are illegal, $ nslookup com.. nslookup: 'com..' is not a legal name (empty label) but $ nslookup . Server:192.168.10.1 Address:192.168.10.1#53 Non-authoritative answer: *** Can't find .: No answer Thanks for the testing. The behaviors are the same as this fix now. Learn something new today to use nslookup. Also, since this bug was originally about SNIHostName, do you need to add some extra restriction there to reject oracle.com. things? No, we cannot restrict the format of IDN in SNIHostName more than in IDN. However, we may need to rethink about the comparing of two IDN, for example, example.com. should equal to example.com. I want to consider it in another bug. Can I push the changeset? Thanks, Xuelei Thanks Max On 8/9/13 8:41 AM, Xuelei Fan wrote: Ping. Thanks, Xuelei On 8/7/2013 11:17 PM, Xuelei Fan wrote: Please review the new update: http://cr.openjdk.java.net./~xuelei/8020842/webrev.01/ With this update, com. is valid (return com.); . and example..com are invalid. And IAE will be thrown for invalid IDN. Thanks, Xuelei
Re: Code review request, 8020842 IDN do not throw IAE when hostname ends with a trailing dot
On 8/9/13 9:37 AM, Xuelei Fan wrote: On 8/9/2013 9:22 AM, Weijun Wang wrote: I tried nslookup. Those with .. inside are illegal, $ nslookup com.. nslookup: 'com..' is not a legal name (empty label) but $ nslookup . Server:192.168.10.1 Address:192.168.10.1#53 Non-authoritative answer: *** Can't find .: No answer Thanks for the testing. The behaviors are the same as this fix now. No exactly. It seems nslookup still regards . legal but just cannot find an IP for it. Learn something new today to use nslookup. Also, since this bug was originally about SNIHostName, do you need to add some extra restriction there to reject oracle.com. things? No, we cannot restrict the format of IDN in SNIHostName more than in IDN. However, we may need to rethink about the comparing of two IDN, for example, example.com. should equal to example.com. I want to consider it in another bug. Not sure. Does the spec say IDN and SNIHostName are equivalent sets? And it's not one is another's subset? Can I push the changeset? I think it's better to ask someone in the networking team to make the suggestion. From what I read Michael in this thread, he does not seem totally agreed with your code changes (at least not the 00 version). Thanks Max Thanks, Xuelei Thanks Max On 8/9/13 8:41 AM, Xuelei Fan wrote: Ping. Thanks, Xuelei On 8/7/2013 11:17 PM, Xuelei Fan wrote: Please review the new update: http://cr.openjdk.java.net./~xuelei/8020842/webrev.01/ With this update, com. is valid (return com.); . and example..com are invalid. And IAE will be thrown for invalid IDN. Thanks, Xuelei
Re: Code review request, 8020842 IDN do not throw IAE when hostname ends with a trailing dot
On 8/9/2013 10:14 AM, Weijun Wang wrote: On 8/9/13 9:37 AM, Xuelei Fan wrote: On 8/9/2013 9:22 AM, Weijun Wang wrote: I tried nslookup. Those with .. inside are illegal, $ nslookup com.. nslookup: 'com..' is not a legal name (empty label) but $ nslookup . Server:192.168.10.1 Address:192.168.10.1#53 Non-authoritative answer: *** Can't find .: No answer Thanks for the testing. The behaviors are the same as this fix now. No exactly. It seems nslookup still regards . legal but just cannot find an IP for it. I'm not sure whether a root domain name can be stand alone. Root label is not considered as a label in IDN. I think it is safe to regard that . is not a valid IDN as it contains no label. Anyway, it is a corner case. There are many online IDN conversion web services, some of them can convert ., some of the cannot. In the present implementation, we cannot recognize ., and IDN.toASCII(.) throws StringIndexOutOfBoundsException. With this fix, I was wondering IAE is a better exception for IDN.toASCII(.). Learn something new today to use nslookup. Also, since this bug was originally about SNIHostName, do you need to add some extra restriction there to reject oracle.com. things? No, we cannot restrict the format of IDN in SNIHostName more than in IDN. However, we may need to rethink about the comparing of two IDN, for example, example.com. should equal to example.com. I want to consider it in another bug. Not sure. Does the spec say IDN and SNIHostName are equivalent sets? And it's not one is another's subset? Per TLS specification, host name in SNI is an IDN. The spec of SNIHostname says, hostname is not a valid Internationalized Domain Name (IDN) compliant with the RFC 3490 specification. The spec in SNIHostName has the same means as IDN. I won't want to add additional restrict beyond the specification of an IDN. Xuelei Can I push the changeset? I think it's better to ask someone in the networking team to make the suggestion. From what I read Michael in this thread, he does not seem totally agreed with your code changes (at least not the 00 version). Thanks Max Thanks, Xuelei Thanks Max On 8/9/13 8:41 AM, Xuelei Fan wrote: Ping. Thanks, Xuelei On 8/7/2013 11:17 PM, Xuelei Fan wrote: Please review the new update: http://cr.openjdk.java.net./~xuelei/8020842/webrev.01/ With this update, com. is valid (return com.); . and example..com are invalid. And IAE will be thrown for invalid IDN. Thanks, Xuelei
Re: Code review request, 8020842 IDN do not throw IAE when hostname ends with a trailing dot
But, DNS considers . as the valid root zone... -- Sent from my mobile device. Xuelei Fan xuelei@oracle.com wrote: On 8/9/2013 10:14 AM, Weijun Wang wrote: On 8/9/13 9:37 AM, Xuelei Fan wrote: On 8/9/2013 9:22 AM, Weijun Wang wrote: I tried nslookup. Those with .. inside are illegal, $ nslookup com.. nslookup: 'com..' is not a legal name (empty label) but $ nslookup . Server:192.168.10.1 Address:192.168.10.1#53 Non-authoritative answer: *** Can't find .: No answer Thanks for the testing. The behaviors are the same as this fix now. No exactly. It seems nslookup still regards . legal but just cannot find an IP for it. I'm not sure whether a root domain name can be stand alone. Root label is not considered as a label in IDN. I think it is safe to regard that . is not a valid IDN as it contains no label. Anyway, it is a corner case. There are many online IDN conversion web services, some of them can convert ., some of the cannot. In the present implementation, we cannot recognize ., and IDN.toASCII(.) throws StringIndexOutOfBoundsException. With this fix, I was wondering IAE is a better exception for IDN.toASCII(.). Learn something new today to use nslookup. Also, since this bug was originally about SNIHostName, do you need to add some extra restriction there to reject oracle.com. things? No, we cannot restrict the format of IDN in SNIHostName more than in IDN. However, we may need to rethink about the comparing of two IDN, for example, example.com. should equal to example.com. I want to consider it in another bug. Not sure. Does the spec say IDN and SNIHostName are equivalent sets? And it's not one is another's subset? Per TLS specification, host name in SNI is an IDN. The spec of SNIHostname says, hostname is not a valid Internationalized Domain Name (IDN) compliant with the RFC 3490 specification. The spec in SNIHostName has the same means as IDN. I won't want to add additional restrict beyond the specification of an IDN. Xuelei Can I push the changeset? I think it's better to ask someone in the networking team to make the suggestion. From what I read Michael in this thread, he does not seem totally agreed with your code changes (at least not the 00 version). Thanks Max Thanks, Xuelei Thanks Max On 8/9/13 8:41 AM, Xuelei Fan wrote: Ping. Thanks, Xuelei On 8/7/2013 11:17 PM, Xuelei Fan wrote: Please review the new update: http://cr.openjdk.java.net./~xuelei/8020842/webrev.01/ With this update, com. is valid (return com.); . and example..com are invalid. And IAE will be thrown for invalid IDN. Thanks, Xuelei
hg: jdk8/tl/jdk: 8021788: JarInputStream doesn't provide certificates for some file under META-INF
Changeset: 758e3117899c Author:weijun Date: 2013-08-09 11:41 +0800 URL: http://hg.openjdk.java.net/jdk8/tl/jdk/rev/758e3117899c 8021788: JarInputStream doesn't provide certificates for some file under META-INF Reviewed-by: chegar, sherman ! src/share/classes/java/util/jar/JarVerifier.java + test/java/util/jar/JarInputStream/ExtraFileInMetaInf.java
Re: Code review request, 8020842 IDN do not throw IAE when hostname ends with a trailing dot
On 8/9/2013 11:24 AM, Matthew Hall wrote: But, DNS considers . as the valid root zone... Good! Looks like that IDN.toASCII(.) should returns ., so that a general domain name can always use IDN.toASCII() conversion instead of throwing runtime exception. Xuelei
Re: Code review request, 8020842 IDN do not throw IAE when hostname ends with a trailing dot
Thanks for your feedback and suggestions. Here is the new webrev: http://cr.openjdk.java.net/~xuelei/8020842/webrev.02/ . is regarded as valid IDN in this update. Thanks, Xuelei On 8/9/2013 10:50 AM, Xuelei Fan wrote: On 8/9/2013 10:14 AM, Weijun Wang wrote: On 8/9/13 9:37 AM, Xuelei Fan wrote: On 8/9/2013 9:22 AM, Weijun Wang wrote: I tried nslookup. Those with .. inside are illegal, $ nslookup com.. nslookup: 'com..' is not a legal name (empty label) but $ nslookup . Server:192.168.10.1 Address:192.168.10.1#53 Non-authoritative answer: *** Can't find .: No answer Thanks for the testing. The behaviors are the same as this fix now. No exactly. It seems nslookup still regards . legal but just cannot find an IP for it. I'm not sure whether a root domain name can be stand alone. Root label is not considered as a label in IDN. I think it is safe to regard that . is not a valid IDN as it contains no label. Anyway, it is a corner case. There are many online IDN conversion web services, some of them can convert ., some of the cannot. In the present implementation, we cannot recognize ., and IDN.toASCII(.) throws StringIndexOutOfBoundsException. With this fix, I was wondering IAE is a better exception for IDN.toASCII(.). Learn something new today to use nslookup. Also, since this bug was originally about SNIHostName, do you need to add some extra restriction there to reject oracle.com. things? No, we cannot restrict the format of IDN in SNIHostName more than in IDN. However, we may need to rethink about the comparing of two IDN, for example, example.com. should equal to example.com. I want to consider it in another bug. Not sure. Does the spec say IDN and SNIHostName are equivalent sets? And it's not one is another's subset? Per TLS specification, host name in SNI is an IDN. The spec of SNIHostname says, hostname is not a valid Internationalized Domain Name (IDN) compliant with the RFC 3490 specification. The spec in SNIHostName has the same means as IDN. I won't want to add additional restrict beyond the specification of an IDN. Xuelei Can I push the changeset? I think it's better to ask someone in the networking team to make the suggestion. From what I read Michael in this thread, he does not seem totally agreed with your code changes (at least not the 00 version). Thanks Max Thanks, Xuelei Thanks Max On 8/9/13 8:41 AM, Xuelei Fan wrote: Ping. Thanks, Xuelei On 8/7/2013 11:17 PM, Xuelei Fan wrote: Please review the new update: http://cr.openjdk.java.net./~xuelei/8020842/webrev.01/ With this update, com. is valid (return com.); . and example..com are invalid. And IAE will be thrown for invalid IDN. Thanks, Xuelei
Re: RFR [8022584] Memory leak in some NetworkInterface methods
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
Re: RFR [8022584] Memory leak in some NetworkInterface methods
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. 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. 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
