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 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 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 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: RFR 6642881: Improve performance of Class.getClassLoader()
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 6642881: Improve performance of Class.getClassLoader()
Coleen, On 6/23/2014 4:45 PM, 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. This change looks reasonable.As a side note: Joel mentions about the mechanism to hide class members from reflection. I discussed with Joel offline before he went on parental leave and suggest that we should revisit the two mechanisms that both effectively disallow access to private members in the future. Mandy 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 6642881: Improve performance of Class.getClassLoader()
On 6/23/14, 9:36 PM, Mandy Chung wrote: Coleen, On 6/23/2014 4:45 PM, 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. This change looks reasonable.As a side note: Joel mentions about the mechanism to hide class members from reflection. I discussed with Joel offline before he went on parental leave and suggest that we should revisit the two mechanisms that both effectively disallow access to private members in the future. Thanks, Mandy. Yes, let me know what you come up with and we can improve this. Thank you for the help fixing this bug. Coleen Mandy 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 6642881: Improve performance of Class.getClassLoader()
On 24/06/2014 11:44 AM, Coleen Phillimore wrote: On 6/23/14, 9:36 PM, Mandy Chung wrote: Coleen, On 6/23/2014 4:45 PM, 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. This change looks reasonable.As a side note: Joel mentions about the mechanism to hide class members from reflection. I discussed with Joel offline before he went on parental leave and suggest that we should revisit the two mechanisms that both effectively disallow access to private members in the future. Thanks, Mandy. Yes, let me know what you come up with and we can improve this. Thank you for the help fixing this bug. Note that completely hiding something from reflection is different to controlling its accessability via reflection. I think what is being done here is the right way to go. The changes look okay to me. Thanks, David Coleen Mandy 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
