Mikael, see below. On 6/24/14, 9:48 AM, Mikael Gerdin wrote:

On Tuesday 24 June 2014 08.51.18 Coleen Phillimore wrote:

...

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/AccessibleObje ct.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. Peter has a good point actually. If the classLoader field is trustworthy we can get rid of some code in InstanceMirrorKlass::{oop_follow_contents, oop_oop_iterate} which currently picks up the class loader through the Klass* field injected into java/lang/Class. It would be a nice simplification if the ClassLoader objects were the only places where the GC would need to find the link to the ClassLoaderData metadata.

But that won't work because anonymous (as in Unsafe.defineAnonymousClass) classes don't interact with class loaders in the same way and have their own ClassLoaderData objects.

Yes, one version of this patch removed class_loader_data from InstanceKlass so we'd be space-neutral but since there isn't a 1-1 relationship to class_loader and ClassLoaderData for Unsafe anonymous classes, it doesn't work. We might think of a way around this because we want to go in this direction. Thanks, Coleen

/Mikael

...

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.chung@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.phillimore@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