Re: RFR 6642881: Improve performance of Class.getClassLoader()
On 24/06/2014 00:45, Coleen Phillimore wrote: Please review a change to the JDK code for adding classLoader field to the instances of java/lang/Class. This change restricts reflection from changing access to the classLoader field. In the spec, AccessibleObject.setAccessible() may throw SecurityException if the accessibility of an object may not be changed: http://docs.oracle.com/javase/7/docs/api/java/lang/reflect/AccessibleObject.html#setAccessible(boolean) Only AccessibleObject.java has changes from the previous version of this change. open webrev at http://cr.openjdk.java.net/~coleenp/6642881_jdk_4/ bug link https://bugs.openjdk.java.net/browse/JDK-6642881 Having setAccessible throw SecurityException when there isn't a security manager set is a bit odd but as you point out, setAccessible is specified to allow that. The changes looks okay, although I don't think strictly necessary. -Alan.
Re: RFR 8035490: move xml jaxp unit tests in bugxxx dir into jaxp repo
On 24/06/2014 01:14, huizhe wang wrote: : One thing that concerns me a bit is that none of the .java files that I looked at have any comments to say what the test does. Is there anything that could be brought over from the original issue in JIRA to explain what each of these tests is about? It would have been nice if they had comments. But adding comments would be too much work, unnecessary I would think. These tests serve their purpose, that is, preventing regressions. In case any fails, its bugid would allow us to easily find out what it was testing. If there is a test failure then whoever runs into it will need to decypher what the test is about. I would think that it would at least be helpful to have a short summary at the top to give some indication as to what it is doing. Are we even sure that all the issues are accessible to all in JIRA? -Alan
Re: RFR: 8047795: Collections.checkedList checking bypassed by List.replaceAll
On Jun 24, 2014, at 2:42 AM, Mike Duigou mike.dui...@oracle.com wrote: Hello all; This changeset corrects a reported problem with the lists returned by Collections.checkedList(). Since Java 8 the replaceAll() method on checked lists has erroneously allowed the operator providing replacements to provide illegal replacement values which are then stored, unchecked into the wrapped list. This changeset adds a check on the proposed replacement value and throws a ClassCastException if the replacement value is incompatible with the list. That seems like a reasonable approach and it's in sync with the behaviour of Map.replaceAll. Additionally the javadoc is updated to inform users that a ClassCastException may result if the proposed replacement is unacceptable. No users will see the JavaDoc on Collections.CheckedList since it is package private, plus i think it redundant. Any such associated documentation would need to be on the public static method, and i am not sure we really need to say anything more than what is already said: 3388 * Any attempt to insert an element of the wrong type will result in 3389 * an immediate {@link ClassCastException}. Assuming a list contains Note that this changeset takes the strategy of failing when the illegal value is encountered. Replacements of earlier items in the list are retained. jbsbug: https://bugs.openjdk.java.net/browse/JDK-8047795 webrev: http://cr.openjdk.java.net/~mduigou/JDK-8047795/0/webrev/ Are there existing tests for the checked Map.replaceAll (for keys and values)? Paul. This change will be backported to Java 8. Mike
A question about Depth of container annotations
Hi All, I have a question about repeated annotation depth. If this is not the proper mailing list group, please tell me where I should send it to. My question will be about the depth of container annotations. For instance, assume there are 3 annotations. - RepeatedAnn - ContainerAnn - ContainerContainerAnn So, ContainerContainerAnn can have ContainerAnn and that can also have RepeatbleAnn in it. In this case, get*AnnotationsByType(RepeatableAnn) APIs wont return repeteableAnns in depth 2. Java docs don't talk about the depth. I wonder if the get*AnnotationsByType api should return the annotations of all depth? If we have below annotations: @Retention(RetentionPolicy.RUNTIME) @Repeatable(ContainerAnn.class) @interface RepeatableAnn { String value(); } @Inherited @Retention(RetentionPolicy.RUNTIME) @Repeatable(ContainerContainerAnn.class) @interface ContainerAnn { RepeatableAnn[] value(); } @Inherited @Retention(RetentionPolicy.RUNTIME) @interface ContainerContainerAnn { ContainerAnn[] value(); } And the main class is annotated by : @ContainerAnn({@RepeatableAnn(Parent-3)}) @ContainerAnn({@RepeatableAnn(Parent-4)}) public class Test { ... when we call getAnnotationsByType(RepeatableAnn.class) on this class, we get nothing because RepeatableAnn is contained by ContainerAnn which is also contained by ContainerContainerAnn. In other words, RepeatableAnn is in depth 2 and get*AnnotationsByType APIs only searches depth 1. The test results are: /home/deven/jdk8_20/jdk1.8.0_20/bin$ java repeated.Test CLASS = repeated.Test getDeclaredAnnotationsByType(RepeatableAnn.class) getDeclaredAnnotationsByType(ContainerAnn.class) @repeated.ContainerAnn(value=[@repeated.RepeatableAnn(value=Parent-3)]) @repeated.ContainerAnn(value=[@repeated.RepeatableAnn(value=Parent-4)]) getDeclaredAnnotationsByType(ContainerContainerAnn.class) @repeated.ContainerContainerAnn(value=[@repeated.ContainerAnn(value=[@repeated.RepeatableAnn(value=Parent-3)]), @repeated.ContainerAnn(value=[@repeated.RepeatableAnn(value=Parent-4)])]) getAnnotationsByType(RepeatableAnn.class) getAnnotationsByType(ContainerAnn.class) @repeated.ContainerAnn(value=[@repeated.RepeatableAnn(value=Parent-3)]) @repeated.ContainerAnn(value=[@repeated.RepeatableAnn(value=Parent-4)]) getAnnotationsByType(ContainerContainerAnn.class) @repeated.ContainerContainerAnn(value=[@repeated.ContainerAnn(value=[@repeated.RepeatableAnn(value=Parent-3)]), @repeated.ContainerAnn(value=[@repeated.RepeatableAnn(value=Parent-4)])])
Re: RFR 6642881: Improve performance of Class.getClassLoader()
On 06/24/2014 01:45 AM, Coleen Phillimore wrote: Please review a change to the JDK code for adding classLoader field to the instances of java/lang/Class. This change restricts reflection from changing access to the classLoader field. In the spec, AccessibleObject.setAccessible() may throw SecurityException if the accessibility of an object may not be changed: http://docs.oracle.com/javase/7/docs/api/java/lang/reflect/AccessibleObject.html#setAccessible(boolean) Only AccessibleObject.java has changes from the previous version of this change. open webrev at http://cr.openjdk.java.net/~coleenp/6642881_jdk_4/ bug link https://bugs.openjdk.java.net/browse/JDK-6642881 Thanks, Coleen Hi Coleen, So hackers are prevented from hacking... Out of curiosity, what would happen if someone changed the classLoader field of some Class object? I guess VM still has it's own notion of the class' class loader, right? Only the code that directly uses the Class.getClassLoader() (and Unsafe.defineClass0) methods would be affected... While we're at it, does this change in any way affect the GC logic? Will GC now navigate the classLoader field during marking but previously didn't ? Will this have any GC performance penalty ? Regards, Peter On 6/19/14, 11:01 PM, David Holmes wrote: On 20/06/2014 6:53 AM, Joel Borggrén-Franck wrote: Hi Mandy, On 19 jun 2014, at 22:34, Mandy Chung mandy.ch...@oracle.com wrote: On 6/19/14 12:34 PM, Joel Borggrén-Franck wrote: Hi, On 19 jun 2014, at 20:46, Coleen Phillimore coleen.phillim...@oracle.com wrote: On 6/17/14, 12:38 AM, Joel Borggrén-Franck wrote: Have you considered hiding the Class.classLoader field from reflection? I’m not sure it is necessary but I’m not to keen on the idea of people poking at this field with Unsafe (which should go away in 9 but …). I don't know how to hide the field from reflection. It's a private final field so you need to get priviledges to make it setAccessible. If you mean injecting it on the JVM side, the reason for this change is so that the JIT compilers can inline this call and not have to call into the JVM to get the class loader. There is sun.reflect.Reflection.registerFieldsToFilter() that hides a field from being found using reflection. It might very well be the case that private and final is enough, I haven’t thought this through 100%. On the other hand, is there a reason to give users access through the field instead of having to use Class.getClassLoader()? There are many getter methods that returns a private final field. I'm not sure if hiding the field is necessary nor a good precedence to set. Accessing a private field requires accessDeclaredMember permission although it's a different security check (vs getClassLoader permission). Arguably before this new classLoader field, one could call Class.getClassLoader0() via reflection to get a hold of class loader. Perhaps you are concerned that the accessDeclaredMember permission is too coarse-grained? I think the security team is looking into the improvement in that regards. I think I’m a bit worried about two things, first as you wrote, “accessDeclaredMember” isn’t the same as “getClassLoader”, but since you could get the class loader through getClassLoader0() that shouldn’t be a new issue. The second thing is that IIRC there are ways to set a final field after initialization. I’m not sure we need to care about that either if you need Unsafe to do it. Normal reflection can set a final field if you can successfully call setAccessible(true) on the Field object. David - cheers /Joel
Re: RFR 6642881: Improve performance of Class.getClassLoader()
Hi Coleen, It seems that there's still a reference to JVM_GetClassLoader in file jdk/src/share/native/common/check_code.c. The code looks like dead code, but it would be nice to clean it up. Thanks, Fred On 06/24/2014 01:45 AM, Coleen Phillimore wrote: Please review a change to the JDK code for adding classLoader field to the instances of java/lang/Class. This change restricts reflection from changing access to the classLoader field. In the spec, AccessibleObject.setAccessible() may throw SecurityException if the accessibility of an object may not be changed: http://docs.oracle.com/javase/7/docs/api/java/lang/reflect/AccessibleObject.html#setAccessible(boolean) Only AccessibleObject.java has changes from the previous version of this change. open webrev at http://cr.openjdk.java.net/~coleenp/6642881_jdk_4/ bug link https://bugs.openjdk.java.net/browse/JDK-6642881 Thanks, Coleen On 6/19/14, 11:01 PM, David Holmes wrote: On 20/06/2014 6:53 AM, Joel Borggrén-Franck wrote: Hi Mandy, On 19 jun 2014, at 22:34, Mandy Chung mandy.ch...@oracle.com wrote: On 6/19/14 12:34 PM, Joel Borggrén-Franck wrote: Hi, On 19 jun 2014, at 20:46, Coleen Phillimore coleen.phillim...@oracle.com wrote: On 6/17/14, 12:38 AM, Joel Borggrén-Franck wrote: Have you considered hiding the Class.classLoader field from reflection? I’m not sure it is necessary but I’m not to keen on the idea of people poking at this field with Unsafe (which should go away in 9 but …). I don't know how to hide the field from reflection. It's a private final field so you need to get priviledges to make it setAccessible. If you mean injecting it on the JVM side, the reason for this change is so that the JIT compilers can inline this call and not have to call into the JVM to get the class loader. There is sun.reflect.Reflection.registerFieldsToFilter() that hides a field from being found using reflection. It might very well be the case that private and final is enough, I haven’t thought this through 100%. On the other hand, is there a reason to give users access through the field instead of having to use Class.getClassLoader()? There are many getter methods that returns a private final field. I'm not sure if hiding the field is necessary nor a good precedence to set. Accessing a private field requires accessDeclaredMember permission although it's a different security check (vs getClassLoader permission). Arguably before this new classLoader field, one could call Class.getClassLoader0() via reflection to get a hold of class loader. Perhaps you are concerned that the accessDeclaredMember permission is too coarse-grained? I think the security team is looking into the improvement in that regards. I think I’m a bit worried about two things, first as you wrote, “accessDeclaredMember” isn’t the same as “getClassLoader”, but since you could get the class loader through getClassLoader0() that shouldn’t be a new issue. The second thing is that IIRC there are ways to set a final field after initialization. I’m not sure we need to care about that either if you need Unsafe to do it. Normal reflection can set a final field if you can successfully call setAccessible(true) on the Field object. David - cheers /Joel -- Frederic Parain - Oracle Grenoble Engineering Center - France Phone: +33 4 76 18 81 17 Email: frederic.par...@oracle.com
Re: RFR: 8047795: Collections.checkedList checking bypassed by List.replaceAll
Looks good to me Mike. I agree with Paul’s comment that the javadoc change will never be seen in the public docs, but I still think it is a reasonable addition for future maintainers. Trivially, you should probably add @SuppressWarnings(unchecked”) to typeCheck(Object). -Chris. On 24 Jun 2014, at 01:42, Mike Duigou mike.dui...@oracle.com wrote: Hello all; This changeset corrects a reported problem with the lists returned by Collections.checkedList(). Since Java 8 the replaceAll() method on checked lists has erroneously allowed the operator providing replacements to provide illegal replacement values which are then stored, unchecked into the wrapped list. This changeset adds a check on the proposed replacement value and throws a ClassCastException if the replacement value is incompatible with the list. Additionally the javadoc is updated to inform users that a ClassCastException may result if the proposed replacement is unacceptable. Note that this changeset takes the strategy of failing when the illegal value is encountered. Replacements of earlier items in the list are retained. jbsbug: https://bugs.openjdk.java.net/browse/JDK-8047795 webrev: http://cr.openjdk.java.net/~mduigou/JDK-8047795/0/webrev/ This change will be backported to Java 8. Mike
Re: Ready for Review : 8042469 : Launcher changes for native memory tracking scalability enhancement
Neil, Some nits: TestSpecialArgs.java: extra space 105 for ( String line : tr.testOutput) { This is very C'ish, I suggest. -106 int index; -107 if ((index = line.indexOf(envVarPidString)) = 0) { +106 int index = line.indexOf(envVarPidString); +107 if (index = 0) { Needs space -146 launcherPid = line.substring(sindex,tindex); +146 launcherPid = line.substring(sindex, tindex); Needs space -168 if (!tr.contains(NativeMemoryTracking: got value +NMT_Option_Value)) { +168 if (!tr.contains(NativeMemoryTracking: got value + NMT_Option_Value)) { Thanks Kumar On 6/23/2014 10:26 AM, Neil Toda wrote: I've spun a new webrev based on the comments for webrev-03: http://cr.openjdk.java.net/~ntoda/8042469/webrev-04/ Changes are: 1) java.c a) add free(envName) line 1063 b) change from malloc() to JLI_MemAlloc() @ lines 1048 and 1056 Thanks -neil On 6/20/2014 4:45 PM, Kumar Srinivasan wrote: Neil, Generally looks good, yes JLI_* functions must be used, I missed that one. Are you going to post another iteration ? Kumar On 6/20/2014 4:27 PM, Neil Toda wrote: Thanks Joe. It would have checked for NULL for me. I'll use the JLI wrapper. -neil On 6/20/2014 4:04 PM, Joe Darcy wrote: Memory allocation in the launcher should use one of the JLI_MemAlloc wrappers, if possible. -Joe On 06/20/2014 09:50 AM, Neil Toda wrote: They should complain. Thanks Zhengyu. I'll make sure these are non-null. -neil On 6/20/2014 5:01 AM, Zhengyu Gu wrote: Neil, Thanks for quick implementation. java.c: Did not check return values of malloc(), I wonder if source code analyzers will complain. -Zhengyu On 6/19/2014 8:29 PM, Neil Toda wrote: Launcher support for modified Native Memory Tracking mechanism in JVM in JDK9. Webrev : http://cr.openjdk.java.net/~ntoda/8042469/webrev-03/ bug : https://bugs.openjdk.java.net/browse/JDK-8042469 CCC : http://ccc.us.oracle.com/8042469 Thanks. -neil
Re: RFR 6642881: Improve performance of Class.getClassLoader()
Hi Peter, On 6/24/14, 4:23 AM, Peter Levart wrote: On 06/24/2014 01:45 AM, Coleen Phillimore wrote: Please review a change to the JDK code for adding classLoader field to the instances of java/lang/Class. This change restricts reflection from changing access to the classLoader field. In the spec, AccessibleObject.setAccessible() may throw SecurityException if the accessibility of an object may not be changed: http://docs.oracle.com/javase/7/docs/api/java/lang/reflect/AccessibleObject.html#setAccessible(boolean) Only AccessibleObject.java has changes from the previous version of this change. open webrev at http://cr.openjdk.java.net/~coleenp/6642881_jdk_4/ bug link https://bugs.openjdk.java.net/browse/JDK-6642881 Thanks, Coleen Hi Coleen, So hackers are prevented from hacking... Out of curiosity, what would happen if someone changed the classLoader field of some Class object? I guess VM still has it's own notion of the class' class loader, right? Only the code that directly uses the Class.getClassLoader() (and Unsafe.defineClass0) methods would be affected... True, Class.getClassLoader() calls would be affected but we may use this call for security checks. I'm not really an expert on this, but we thought it wouldn't be safe to allow user access to classLoader. While we're at it, does this change in any way affect the GC logic? Will GC now navigate the classLoader field during marking but previously didn't ? Will this have any GC performance penalty ? I actually ran this through our performance testing and got a good improvement in one of the scimark benchmarks for no reason I could explain (and lost the results), but none of the other tests were affected. GC would have to mark this new field for full collections but not young collections because it's only set during initialization. I wouldn't expect this field to have any negative performance for GC. Thanks, Coleen Regards, Peter On 6/19/14, 11:01 PM, David Holmes wrote: On 20/06/2014 6:53 AM, Joel Borggrén-Franck wrote: Hi Mandy, On 19 jun 2014, at 22:34, Mandy Chung mandy.ch...@oracle.com wrote: On 6/19/14 12:34 PM, Joel Borggrén-Franck wrote: Hi, On 19 jun 2014, at 20:46, Coleen Phillimore coleen.phillim...@oracle.com wrote: On 6/17/14, 12:38 AM, Joel Borggrén-Franck wrote: Have you considered hiding the Class.classLoader field from reflection? I’m not sure it is necessary but I’m not to keen on the idea of people poking at this field with Unsafe (which should go away in 9 but …). I don't know how to hide the field from reflection. It's a private final field so you need to get priviledges to make it setAccessible. If you mean injecting it on the JVM side, the reason for this change is so that the JIT compilers can inline this call and not have to call into the JVM to get the class loader. There is sun.reflect.Reflection.registerFieldsToFilter() that hides a field from being found using reflection. It might very well be the case that private and final is enough, I haven’t thought this through 100%. On the other hand, is there a reason to give users access through the field instead of having to use Class.getClassLoader()? There are many getter methods that returns a private final field. I'm not sure if hiding the field is necessary nor a good precedence to set. Accessing a private field requires accessDeclaredMember permission although it's a different security check (vs getClassLoader permission). Arguably before this new classLoader field, one could call Class.getClassLoader0() via reflection to get a hold of class loader. Perhaps you are concerned that the accessDeclaredMember permission is too coarse-grained? I think the security team is looking into the improvement in that regards. I think I’m a bit worried about two things, first as you wrote, “accessDeclaredMember” isn’t the same as “getClassLoader”, but since you could get the class loader through getClassLoader0() that shouldn’t be a new issue. The second thing is that IIRC there are ways to set a final field after initialization. I’m not sure we need to care about that either if you need Unsafe to do it. Normal reflection can set a final field if you can successfully call setAccessible(true) on the Field object. David - cheers /Joel
Re: RFR 6642881: Improve performance of Class.getClassLoader()
Fred, Thank you for finding this. Yes, I meant to clean this up with the bug to remove JVM_GetClassLoader but I should remove this with this change instead, since the other change will be in hotspot only. Yes, it's dead code. Thanks! Coleen On 6/24/14, 4:41 AM, Frederic Parain wrote: Hi Coleen, It seems that there's still a reference to JVM_GetClassLoader in file jdk/src/share/native/common/check_code.c. The code looks like dead code, but it would be nice to clean it up. Thanks, Fred On 06/24/2014 01:45 AM, Coleen Phillimore wrote: Please review a change to the JDK code for adding classLoader field to the instances of java/lang/Class. This change restricts reflection from changing access to the classLoader field. In the spec, AccessibleObject.setAccessible() may throw SecurityException if the accessibility of an object may not be changed: http://docs.oracle.com/javase/7/docs/api/java/lang/reflect/AccessibleObject.html#setAccessible(boolean) Only AccessibleObject.java has changes from the previous version of this change. open webrev at http://cr.openjdk.java.net/~coleenp/6642881_jdk_4/ bug link https://bugs.openjdk.java.net/browse/JDK-6642881 Thanks, Coleen On 6/19/14, 11:01 PM, David Holmes wrote: On 20/06/2014 6:53 AM, Joel Borggrén-Franck wrote: Hi Mandy, On 19 jun 2014, at 22:34, Mandy Chung mandy.ch...@oracle.com wrote: On 6/19/14 12:34 PM, Joel Borggrén-Franck wrote: Hi, On 19 jun 2014, at 20:46, Coleen Phillimore coleen.phillim...@oracle.com wrote: On 6/17/14, 12:38 AM, Joel Borggrén-Franck wrote: Have you considered hiding the Class.classLoader field from reflection? I’m not sure it is necessary but I’m not to keen on the idea of people poking at this field with Unsafe (which should go away in 9 but …). I don't know how to hide the field from reflection. It's a private final field so you need to get priviledges to make it setAccessible. If you mean injecting it on the JVM side, the reason for this change is so that the JIT compilers can inline this call and not have to call into the JVM to get the class loader. There is sun.reflect.Reflection.registerFieldsToFilter() that hides a field from being found using reflection. It might very well be the case that private and final is enough, I haven’t thought this through 100%. On the other hand, is there a reason to give users access through the field instead of having to use Class.getClassLoader()? There are many getter methods that returns a private final field. I'm not sure if hiding the field is necessary nor a good precedence to set. Accessing a private field requires accessDeclaredMember permission although it's a different security check (vs getClassLoader permission). Arguably before this new classLoader field, one could call Class.getClassLoader0() via reflection to get a hold of class loader. Perhaps you are concerned that the accessDeclaredMember permission is too coarse-grained? I think the security team is looking into the improvement in that regards. I think I’m a bit worried about two things, first as you wrote, “accessDeclaredMember” isn’t the same as “getClassLoader”, but since you could get the class loader through getClassLoader0() that shouldn’t be a new issue. The second thing is that IIRC there are ways to set a final field after initialization. I’m not sure we need to care about that either if you need Unsafe to do it. Normal reflection can set a final field if you can successfully call setAccessible(true) on the Field object. David - cheers /Joel
Re: RFR: JDK-8044629: (reflect) Constructor.getAnnotatedReceiverType() returns wrong value
On Jun 17, 2014, at 6:52 PM, Joel Borggrén-Franck joel.fra...@oracle.com wrote: Hi, Can I get a review for this fix and javadoc clarification for https://bugs.openjdk.java.net/browse/JDK-8044629 The problem is with potentially annotated receiver parameters, they only exist for inner class constructors, so this fix makes sure that a ctor is for an inner class or returns null (as for a static method) otherwise. CCC has been approved. Webrev: http://cr.openjdk.java.net/~jfranck/8044629/webrev.00/ +1 I never quite realised just how convoluted it was to determine that a class is an inner class. Paul.
Re: ThreadLocalRandom clinit troubles
Hi Martin, On 06/22/2014 07:12 PM, Martin Buchholz wrote: We know that loading the networking machinery is problematic. On Linux we would be content to hard-code a read from /dev/urandom, which is safer and strictly more random than the existing network hardware determination, but y'all will reject that as too system-dependent (insufficient machinery!). H maybe not as long as we code up a good fallback ... I learned that SecureRandom by default on Unix uses /dev/random for seed bytes and /dev/urandom for nextBytes. Here's my proposal, that in the default case on Unix doesn't load any machinery, and as a fallback loads the SecureRandom machinery instead of the network machinery, while maintaining the ultra-secure behavior of the java.util.secureRandomSeed system property: private static long initialSeed() { byte[] seedBytes = initialSeedBytes(); long s = (long)(seedBytes[0]) 0xffL; for (int i = 1; i seedBytes.length; ++i) s = (s 8) | ((long)(seedBytes[i]) 0xffL); return s ^ mix64(System.currentTimeMillis()) ^ mix64(System.nanoTime()); } private static byte[] initialSeedBytes() { String pp = java.security.AccessController.doPrivileged( new sun.security.action.GetPropertyAction( java.util.secureRandomSeed)); boolean secureRandomSeed = (pp != null pp.equalsIgnoreCase(true)); if (secureRandomSeed) return java.security.SecureRandom.getSeed(8); final byte[] seedBytes = new byte[8]; File seedSource = new File(/dev/urandom); if (seedSource.exists()) { try (FileInputStream stream = new FileInputStream(seedSource)) { if (stream.read(seedBytes) == 8) return seedBytes; } catch (IOException ignore) { } } new java.security.SecureRandom().nextBytes(seedBytes); return seedBytes; } So on platforms lacking /dev/urandom special file (Windows only?), the fall-back would be to use SecureRandom.nextBytes() whatever default SecureRandom PRNG is configured to be on such platform. This might be a user-supplied SecureRandom PRNG. The user might want it's own default SecureRandom but not use it for TLR's seed computation (unless requested by java.util.secureRandomSeed system property). I would rather use SecureRandom.generateSeed() instance method instead of SecureRandom.nextBytes(). Why? Because every SecureRandom instance has to initialize it's seed 1st before getBytes() can provide the next random bytes from the PRNG. Since we only need the 1st 8 bytes from the SecureRandom instance to initialize TLR's seeder, we might as well directly call the SecureRandom.generateSeed() method. That's one reason. The other is the peculiar initialization of default SecureRandom algorithm on Windows (see below)... Even if user does not provide it's own default SecureRandom PRNG, there are basically two algorithms that are used by default on OpenJDK: On Solaris/Linux/Mac/AIX: the NativePRNG algorithm from SUN provider (implemented by sun.security.provider.NativePRNG) which uses /dev/random for getBytes() and /dev/urandom (or whatever is configured with java.security.egd or securerandom.source system properties) for generateSeed(), and On Windows: the SHA1PRNG algorithm from SUN provider (implemented by sun.security.provider.SecureRandom) which uses SHA1 message digest for generating random numbers with seed computed by gathering system entropy... The most problematic one is the default on Windows platform (the platform that does not have the /dev/urandom special file and would be used as a fall-back by your proposal) - sun.security.provider.SecureRandom. This one seeds itself by constructing an instance of itself with the result returned from SeedGenerator.getSystemEntropy() method. This method, among other things, uses networking code to gather system entropy: ... md.update (InetAddress.getLocalHost().toString().getBytes()); ... This is problematic since it not only initializes NameService providers but also uses them to resolve local host name. This can block for several seconds on unusual configurations and was the main motivation to replace similar code in TLR with code that uses NetworkInterface.getHardwareAddress() instead. Using SecureRandom.generateSeed() instead of SecureRandom.getBytes() does not invoke SeedGenerator.getSystemEntropy(), but uses just SeedGenerator.generateSeed(), which by default on Windows uses ThreadedSeedGenerator.getSeedBytes()... I showed how we could suppress NameService providers initialization while still using NetworkInterface.getHardwareAddress() for TLR's seeder initialization. If that's not enough and we would like to get-away without using networking code at all, then I propose the following:
Re: RFR 6642881: Improve performance of Class.getClassLoader()
On 6/24/14, 4:41 AM, Frederic Parain wrote: Hi Coleen, It seems that there's still a reference to JVM_GetClassLoader in file jdk/src/share/native/common/check_code.c. The code looks like dead code, but it would be nice to clean it up. I removed this code. There are no other instances of the macro BROKEN_JAVAC. I update the copyrights when I commit the changeset. http://cr.openjdk.java.net/~coleenp/6642881_jdk_5/ Thanks! Coleen Thanks, Fred On 06/24/2014 01:45 AM, Coleen Phillimore wrote: Please review a change to the JDK code for adding classLoader field to the instances of java/lang/Class. This change restricts reflection from changing access to the classLoader field. In the spec, AccessibleObject.setAccessible() may throw SecurityException if the accessibility of an object may not be changed: http://docs.oracle.com/javase/7/docs/api/java/lang/reflect/AccessibleObject.html#setAccessible(boolean) Only AccessibleObject.java has changes from the previous version of this change. open webrev at http://cr.openjdk.java.net/~coleenp/6642881_jdk_4/ bug link https://bugs.openjdk.java.net/browse/JDK-6642881 Thanks, Coleen On 6/19/14, 11:01 PM, David Holmes wrote: On 20/06/2014 6:53 AM, Joel Borggrén-Franck wrote: Hi Mandy, On 19 jun 2014, at 22:34, Mandy Chung mandy.ch...@oracle.com wrote: On 6/19/14 12:34 PM, Joel Borggrén-Franck wrote: Hi, On 19 jun 2014, at 20:46, Coleen Phillimore coleen.phillim...@oracle.com wrote: On 6/17/14, 12:38 AM, Joel Borggrén-Franck wrote: Have you considered hiding the Class.classLoader field from reflection? I’m not sure it is necessary but I’m not to keen on the idea of people poking at this field with Unsafe (which should go away in 9 but …). I don't know how to hide the field from reflection. It's a private final field so you need to get priviledges to make it setAccessible. If you mean injecting it on the JVM side, the reason for this change is so that the JIT compilers can inline this call and not have to call into the JVM to get the class loader. There is sun.reflect.Reflection.registerFieldsToFilter() that hides a field from being found using reflection. It might very well be the case that private and final is enough, I haven’t thought this through 100%. On the other hand, is there a reason to give users access through the field instead of having to use Class.getClassLoader()? There are many getter methods that returns a private final field. I'm not sure if hiding the field is necessary nor a good precedence to set. Accessing a private field requires accessDeclaredMember permission although it's a different security check (vs getClassLoader permission). Arguably before this new classLoader field, one could call Class.getClassLoader0() via reflection to get a hold of class loader. Perhaps you are concerned that the accessDeclaredMember permission is too coarse-grained? I think the security team is looking into the improvement in that regards. I think I’m a bit worried about two things, first as you wrote, “accessDeclaredMember” isn’t the same as “getClassLoader”, but since you could get the class loader through getClassLoader0() that shouldn’t be a new issue. The second thing is that IIRC there are ways to set a final field after initialization. I’m not sure we need to care about that either if you need Unsafe to do it. Normal reflection can set a final field if you can successfully call setAccessible(true) on the Field object. David - cheers /Joel
Re: Review request for 8047904: Runtime.loadLibrary throws SecurityException when security manager is installed
On Jun 23, 2014, at 10:23 PM, Mandy Chung mandy.ch...@oracle.com wrote: The loadLibrary implementation is missing to wrap the call to File.getCanonicalPath method with doPrivileged block. Webrev: http://cr.openjdk.java.net/~mchung/jdk9/webrevs/8047904/webrev.00/ Looks ok to me. Paul.
Re: RFR 8035490: move xml jaxp unit tests in bugxxx dir into jaxp repo
On 6/24/2014 12:19 AM, Alan Bateman wrote: On 24/06/2014 01:14, huizhe wang wrote: : One thing that concerns me a bit is that none of the .java files that I looked at have any comments to say what the test does. Is there anything that could be brought over from the original issue in JIRA to explain what each of these tests is about? It would have been nice if they had comments. But adding comments would be too much work, unnecessary I would think. These tests serve their purpose, that is, preventing regressions. In case any fails, its bugid would allow us to easily find out what it was testing. If there is a test failure then whoever runs into it will need to decypher what the test is about. I would think that it would at least be helpful to have a short summary at the top to give some indication as to what it is doing. Are we even sure that all the issues are accessible to all in JIRA? If there is a test failure, we can narrow it down to the cause pretty quickly since we have a context, that is, the current patch. Rather than some comments, we generally rely on the test code itself to identify the culprit or browse the original bug report to understand the whole story (when change happened, the whole history and etc.). Furthermore, they don't really fail often, in fact, some of the tests have never failed because of the nature of related fixes. For someone like Patrick who doesn't have knowledge of the original fixes or bug reports to go into them and try to figure out a proper write-up, it may not be an easy task. There are hundreds of tests, plus the function tests that did not have comments either. I'm just not sure it's worth it to spend all that time (and then, not used in most of the cases :-) ). -Joe -Alan
RE: Zombie FileHandler locks can exhaust all available log file locks.
Daniel, With regard to JDK-4420020, in the original webrev Jim was incorrectly using java.io.File.canWrite() but that webrev was replaced by the current version. The NIO.2 code performs the effective access checks correctly. With regard to JDK-6244047 my concern was that checking the permissions and preconditions up front is a small 'TOCTOU' race and redundant because the next step after that is to open the FileChannel. I would assume both CREATE or CREATE_NEW plus WRITE would perform the effective access checks on open. The use of delete on exit is a deal breaker because you can't undo a registration on close of the FileHandler which can trigger an OOME. See https://bugs.openjdk.java.net/browse/JDK-4872014. I've used custom proxy handlers that generate a file name based off of the LogRecord which results in generating lots of file names and opening and close the FileHandler on demand. If the behavior is reverted to use CREATE instead of CREATE_NEW then we have to deal with JDK-8031438 because any user code can lock a file ahead of opening the FileHandler. Jason Date: Mon, 23 Jun 2014 17:13:04 +0200 From: daniel.fu...@oracle.com To: alan.bate...@oracle.com; jason_mehr...@hotmail.com; core-libs-dev@openjdk.java.net Subject: Re: Zombie FileHandler locks can exhaust all available log file locks. On 6/23/14 4:53 PM, Alan Bateman wrote: On 23/06/2014 10:48, Daniel Fuchs wrote: : All in all - I feel our best options would be to revert to using CREATE, or use CREATE_NEW + DELETE_ON_CLOSE, or not do anything and live with the issue. Hopefully some nio experts will chime in ;-) The logging use of FileLock is a bit unusual but looking at it now then I don't see the reason why it needs to use CREATE_NEW. OK - thanks - in the discussion that ended up in the patch where CREATE_NEW was introduced Jason (I think it was Jason) pointed at https://bugs.openjdk.java.net/browse/JDK-4420020 I'm guessing here that maybe it would be wrong to use an already existing lock file if you don't have the rights to write to its directory - and that using CREATE_NEW ensures that you have all necessary access rights all along the path. I don't think DELETE_ON_CLOSE is suitable here as that work is for transient work files which isn't the case here. Instead it could use deleteOnExit to have some chance of removing it on a graceful exit. Right - I suggested a patch in another mail where I just use that. BTW: Do you know why locks is Map? Would a Set do? It certainly looks as if we could use a simple HashSet here. Thanks Alan! -- daniel -Alan.
Re: Zombie FileHandler locks can exhaust all available log file locks.
Hi Jason, Thanks for the feedback! On 6/24/14 7:08 PM, Jason Mehrens wrote: Daniel, With regard to JDK-4420020, in the original webrev Jim was incorrectly using java.io.File.canWrite() but that webrev was replaced by the current version. The NIO.2 code performs the effective access checks correctly. Thanks. With regard to JDK-6244047 my concern was that checking the permissions and preconditions up front is a small 'TOCTOU' race and redundant because the next step after that is to open the FileChannel. I would assume both CREATE or CREATE_NEW plus WRITE would perform the effective access checks on open. OK. The use of delete on exit is a deal breaker because you can't undo a registration on close of the FileHandler which can trigger an OOME. See https://bugs.openjdk.java.net/browse/JDK-4872014. I've used custom proxy handlers that generate a file name based off of the LogRecord which results in generating lots of file names and opening and close the FileHandler on demand. ah. h. I didn't think there would be that many FileHandlers... Well - I guess that if we find a way to reuse the zombie lock files then we don't really need the delete on exit. Someone creating a FileHandler programmatically should be responsible for closing it - so if an application creates FileHandlers without closing them then it's a bug in the application. If the behavior is reverted to use CREATE instead of CREATE_NEW then we have to deal with JDK-8031438 because any user code can lock a file ahead of opening the FileHandler. Yes - I discovered that while writing a test for my suggested fix ;-) Catching OverlappingFileLockException in FileHandler.openFiles and setting available to false when it's thrown fixes the issue. So let's just remove the deleteOnExit add the catch for OverlappingFileLockException to my patch and we should be good. On the other hand we could just use replace CREATE_NEW by CREATE but I have some misgivings about the part that sets available to true when tryLock throws an IOException. I'm not sure we should be doing that if we haven't actually created the lock file. So I think I'd prefer keeping the two steps behavior - first try CREATE_NEW - then attempt to use CREATE if CREATE_NEW failed... I'm not sure CREATE will check the access rights for writing in the directory if the file already exists - so checking the directory for write access in that case is probably the right thing to do. Would you agree? -- daniel Jason Date: Mon, 23 Jun 2014 17:13:04 +0200 From: daniel.fu...@oracle.com To: alan.bate...@oracle.com; jason_mehr...@hotmail.com; core-libs-dev@openjdk.java.net Subject: Re: Zombie FileHandler locks can exhaust all available log file locks. On 6/23/14 4:53 PM, Alan Bateman wrote: On 23/06/2014 10:48, Daniel Fuchs wrote: : All in all - I feel our best options would be to revert to using CREATE, or use CREATE_NEW + DELETE_ON_CLOSE, or not do anything and live with the issue. Hopefully some nio experts will chime in ;-) The logging use of FileLock is a bit unusual but looking at it now then I don't see the reason why it needs to use CREATE_NEW. OK - thanks - in the discussion that ended up in the patch where CREATE_NEW was introduced Jason (I think it was Jason) pointed at https://bugs.openjdk.java.net/browse/JDK-4420020 I'm guessing here that maybe it would be wrong to use an already existing lock file if you don't have the rights to write to its directory - and that using CREATE_NEW ensures that you have all necessary access rights all along the path. I don't think DELETE_ON_CLOSE is suitable here as that work is for transient work files which isn't the case here. Instead it could use deleteOnExit to have some chance of removing it on a graceful exit. Right - I suggested a patch in another mail where I just use that. BTW: Do you know why locks is Map? Would a Set do? It certainly looks as if we could use a simple HashSet here. Thanks Alan! -- daniel -Alan.
Re: JDK 9 RFR of JDK-8048014: Update java.lang.SafeVararags for private methods
Will there be another ticket to update the section (9.6.4.7.) in JLS 9? Cheers, Paul On Tue, Jun 24, 2014 at 12:27 PM, Lance Andersen lance.ander...@oracle.com wrote: +1 On Jun 24, 2014, at 1:13 PM, Joe Darcy joe.da...@oracle.com wrote: Hello, Please review the libraries update portion of allowing @SafeVarargs on private instance methods: JDK-8048014: Update java.lang.SafeVararags for private methods The patch is diff -r 6c26f18d9bc0 src/share/classes/java/lang/SafeVarargs.java --- a/src/share/classes/java/lang/SafeVarargs.javaMon Jun 23 12:12:30 2014 -0700 +++ b/src/share/classes/java/lang/SafeVarargs.javaTue Jun 24 10:03:21 2014 -0700 @@ -1,5 +1,5 @@ /* - * Copyright (c) 2010, 2013, Oracle and/or its affiliates. All rights reserved. + * Copyright (c) 2010, 2014, Oracle and/or its affiliates. All rights reserved. * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. * * This code is free software; you can redistribute it and/or modify it @@ -45,7 +45,7 @@ * li the declaration is a fixed arity method or constructor * * li the declaration is a variable arity method that is neither - * {@code static} nor {@code final}. + * {@code static} nor {@code final} nor {@code private}. * * /ul * The bulk of the change to allow @SafeVarargs on private methods, including the tests, are over in the langtools repo with javac updates. The javac changes have been reviewed and approved on compiler-dev: http://mail.openjdk.java.net/pipermail/compiler-dev/2014-June/008855.html Thanks, -Joe Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037 Oracle Java Engineering 1 Network Drive Burlington, MA 01803 lance.ander...@oracle.com
Re: JDK 9 RFR of JDK-8048014: Update java.lang.SafeVararags for private methods
On 06/24/2014 10:43 AM, Paul Benedict wrote: Will there be another ticket to update the section (9.6.4.7.) in JLS 9? JDK-8047159: 9.6.4.7: Allow @SafeVarargs on private methods https://bugs.openjdk.java.net/browse/JDK-8047159 Cheers, -Joe Cheers, Paul On Tue, Jun 24, 2014 at 12:27 PM, Lance Andersen lance.ander...@oracle.com mailto:lance.ander...@oracle.com wrote: +1 On Jun 24, 2014, at 1:13 PM, Joe Darcy joe.da...@oracle.com mailto:joe.da...@oracle.com wrote: Hello, Please review the libraries update portion of allowing @SafeVarargs on private instance methods: JDK-8048014: Update java.lang.SafeVararags for private methods The patch is diff -r 6c26f18d9bc0 src/share/classes/java/lang/SafeVarargs.java --- a/src/share/classes/java/lang/SafeVarargs.javaMon Jun 23 12:12:30 2014 -0700 +++ b/src/share/classes/java/lang/SafeVarargs.javaTue Jun 24 10:03:21 2014 -0700 @@ -1,5 +1,5 @@ /* - * Copyright (c) 2010, 2013, Oracle and/or its affiliates. All rights reserved. + * Copyright (c) 2010, 2014, Oracle and/or its affiliates. All rights reserved. * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. * * This code is free software; you can redistribute it and/or modify it @@ -45,7 +45,7 @@ * li the declaration is a fixed arity method or constructor * * li the declaration is a variable arity method that is neither - * {@code static} nor {@code final}. + * {@code static} nor {@code final} nor {@code private}. * * /ul * The bulk of the change to allow @SafeVarargs on private methods, including the tests, are over in the langtools repo with javac updates. The javac changes have been reviewed and approved on compiler-dev: http://mail.openjdk.java.net/pipermail/compiler-dev/2014-June/008855.html Thanks, -Joe Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037 Oracle Java Engineering 1 Network Drive Burlington, MA 01803 lance.ander...@oracle.com mailto:lance.ander...@oracle.com
Re: RFR: 8047795: Collections.checkedList checking bypassed by List.replaceAll
On Jun 24 2014, at 01:46 , Chris Hegarty chris.hega...@oracle.com wrote: Looks good to me Mike. I agree with Paul’s comment that the javadoc change will never be seen in the public docs, but I still think it is a reasonable addition for future maintainers. Trivially, you should probably add @SuppressWarnings(unchecked”) to typeCheck(Object). Done. -Chris. On 24 Jun 2014, at 01:42, Mike Duigou mike.dui...@oracle.com wrote: Hello all; This changeset corrects a reported problem with the lists returned by Collections.checkedList(). Since Java 8 the replaceAll() method on checked lists has erroneously allowed the operator providing replacements to provide illegal replacement values which are then stored, unchecked into the wrapped list. This changeset adds a check on the proposed replacement value and throws a ClassCastException if the replacement value is incompatible with the list. Additionally the javadoc is updated to inform users that a ClassCastException may result if the proposed replacement is unacceptable. Note that this changeset takes the strategy of failing when the illegal value is encountered. Replacements of earlier items in the list are retained. jbsbug: https://bugs.openjdk.java.net/browse/JDK-8047795 webrev: http://cr.openjdk.java.net/~mduigou/JDK-8047795/0/webrev/ This change will be backported to Java 8. Mike
Re: A question about Depth of container annotations
Hello, The behavior you're interested in is defined in the AnnotatedElement specification: http://docs.oracle.com/javase/8/docs/api/java/lang/reflect/AnnotatedElement.html In brief, none of the methods in AnnotatedElement look through more than one level of containment. (Likewise, when writing repeated annotation in source code, the compiler will only create one level of containers.) You may be interested in reviewing design discussions of this feature that occurred on http://mail.openjdk.java.net/mailman/listinfo/enhanced-metadata-spec-discuss Cheers, -Joe On 06/24/2014 01:21 AM, deven you wrote: Hi All, I have a question about repeated annotation depth. If this is not the proper mailing list group, please tell me where I should send it to. My question will be about the depth of container annotations. For instance, assume there are 3 annotations. - RepeatedAnn - ContainerAnn - ContainerContainerAnn So, ContainerContainerAnn can have ContainerAnn and that can also have RepeatbleAnn in it. In this case, get*AnnotationsByType(RepeatableAnn) APIs wont return repeteableAnns in depth 2. Java docs don't talk about the depth. I wonder if the get*AnnotationsByType api should return the annotations of all depth? If we have below annotations: @Retention(RetentionPolicy.RUNTIME) @Repeatable(ContainerAnn.class) @interface RepeatableAnn { String value(); } @Inherited @Retention(RetentionPolicy.RUNTIME) @Repeatable(ContainerContainerAnn.class) @interface ContainerAnn { RepeatableAnn[] value(); } @Inherited @Retention(RetentionPolicy.RUNTIME) @interface ContainerContainerAnn { ContainerAnn[] value(); } And the main class is annotated by : @ContainerAnn({@RepeatableAnn(Parent-3)}) @ContainerAnn({@RepeatableAnn(Parent-4)}) public class Test { ... when we call getAnnotationsByType(RepeatableAnn.class) on this class, we get nothing because RepeatableAnn is contained by ContainerAnn which is also contained by ContainerContainerAnn. In other words, RepeatableAnn is in depth 2 and get*AnnotationsByType APIs only searches depth 1. The test results are: /home/deven/jdk8_20/jdk1.8.0_20/bin$ java repeated.Test CLASS = repeated.Test getDeclaredAnnotationsByType(RepeatableAnn.class) getDeclaredAnnotationsByType(ContainerAnn.class) @repeated.ContainerAnn(value=[@repeated.RepeatableAnn(value=Parent-3)]) @repeated.ContainerAnn(value=[@repeated.RepeatableAnn(value=Parent-4)]) getDeclaredAnnotationsByType(ContainerContainerAnn.class) @repeated.ContainerContainerAnn(value=[@repeated.ContainerAnn(value=[@repeated.RepeatableAnn(value=Parent-3)]), @repeated.ContainerAnn(value=[@repeated.RepeatableAnn(value=Parent-4)])]) getAnnotationsByType(RepeatableAnn.class) getAnnotationsByType(ContainerAnn.class) @repeated.ContainerAnn(value=[@repeated.RepeatableAnn(value=Parent-3)]) @repeated.ContainerAnn(value=[@repeated.RepeatableAnn(value=Parent-4)]) getAnnotationsByType(ContainerContainerAnn.class) @repeated.ContainerContainerAnn(value=[@repeated.ContainerAnn(value=[@repeated.RepeatableAnn(value=Parent-3)]), @repeated.ContainerAnn(value=[@repeated.RepeatableAnn(value=Parent-4)])])
RFR: 8048021: Remove @version tag in jaxp repo
Hi, As a follow up task, please review the trivial webrev to remove @version tags used with SCM, which is likely to be meaningless to javadoc readers. In this webrev, we only remove @version with $Id or $Revision for .jave or package.html cause appearance to javadoc. We keep those in .properties files also those @version tags with specific version information. Cheers, Henry On 06/20/2014 05:24 PM, Henry Jen wrote: Looks like it's pretty easy to do a find and replace in files in jaxp folder. If this is desired, we can do it in a separate webrev to be clear on changeset history? Cheers, Henry On 06/20/2014 03:36 PM, huizhe wang wrote: Hi Henry, Is it possible to add to the @since tool/script to remove repository revision notes? They appear in the API document as if they reflect meaningful version. An example in http://cr.openjdk.java.net/~henryjen/jdk9/8047723/0/webrev/src/javax/xml/parsers/DocumentBuilderFactory.java.udiff.html * @version $Revision: 1.9 $, $Date: 2010/05/25 16:19:44 $
Re: RFR: 8048021: Remove @version tag in jaxp repo
Looks good. Thanks for the extra work! I think your webrev is here: http://cr.openjdk.java.net/~henryjen/jdk9/8048021/0/webrev/ Cheers, Joe On 6/24/2014 1:00 PM, Henry Jen wrote: Hi, As a follow up task, please review the trivial webrev to remove @version tags used with SCM, which is likely to be meaningless to javadoc readers. In this webrev, we only remove @version with $Id or $Revision for .jave or package.html cause appearance to javadoc. We keep those in .properties files also those @version tags with specific version information. Cheers, Henry On 06/20/2014 05:24 PM, Henry Jen wrote: Looks like it's pretty easy to do a find and replace in files in jaxp folder. If this is desired, we can do it in a separate webrev to be clear on changeset history? Cheers, Henry On 06/20/2014 03:36 PM, huizhe wang wrote: Hi Henry, Is it possible to add to the @since tool/script to remove repository revision notes? They appear in the API document as if they reflect meaningful version. An example in http://cr.openjdk.java.net/~henryjen/jdk9/8047723/0/webrev/src/javax/xml/parsers/DocumentBuilderFactory.java.udiff.html * @version $Revision: 1.9 $, $Date: 2010/05/25 16:19:44 $