RFR: 6378384 (reflect) subclass can’t access superclass’s protected fields and methods by reflection

[Peter Levart]

Hi,

I have a fix for a 10 year old bug (JDK-6378384 
<https://bugs.openjdk.java.net/browse/JDK-6378384>). It was marked as a 
duplicate of a 19 year old bug (JDK-4032740 
<https://bugs.openjdk.java.net/browse/JDK-4032740>) which is marked as a 
duplicate of a 17 year old bug (JDK-4283544 
<https://bugs.openjdk.java.net/browse/JDK-4283544>) which is still open. 
But this bug is not a strict duplicate. This bug only concerns 
reflective access to protected members.

Here's a proposed fix:

http://cr.openjdk.java.net/~plevart/jdk9-dev/6378384_Reflection.ensureAccess/webrev.01/


The bug manifests itself as not being able to access protected static 
methods or fields from a subclass located in a different package. 
Instance protected methods and fields can be accessed, and using an 
undocumented trick, also static methods and fields, but the trick is 
very subtle. The specification for Field.get/set and Method.invoke says, 
respectively:

      * <p>If the underlying field is a static field, the {@code obj} 
argument
      * is ignored; it may be null.

and:

      * <p>If the underlying method is static, then the specified {@code 
obj}
      * argument is ignored. It may be null.

Well, it is not exactly so! The obj argument is used as a 'target' even 
for protected static members and it is ensured that its class is equal 
or a subclass of the class that accesses the member. So if you pass an 
instance of a subclass of the protected method's declaring class into 
the get/set/invoke, you can access the static protected member. If you 
pass 'null', you get IllegalAccessException.

The problem is in the design of 
jdk.internal.reflect.Reflection#ensureMemberAccess method which is used 
to check reflective access. It takes an Object 'target' argument, which 
is supposed to be null when accessing static methods/fields and it is 
null also when accessing constructors. Because of constructors and the 
method's API, it has to be overly restrictive as it must only allow 
calling protected constructors from within the constructor's declaring 
class itself or same package, while protected static methods could be 
called from any subclass.

By redesigning the API of this method, replacing Object 'target' 
parameter with Class<?> 'targetClass' parameter and by passing the 
constructor's declaring class into this method instead of null, 
reflective checks suddenly start to look more like JLS dictates (but 
still a long way from it, unfortunately).

As a bonus, sun.reflect.misc.ReflectUtil#ensureMemberAccess method (used 
from AtomicXXXFieldUpdater classes) does not need the following comment 
any more:

      * Reflection.ensureMemberAccess is overly-restrictive
      * due to a bug. We awkwardly work around it for now.

...as it can now delegate straight to Reflection.ensureMemberAccess 
without invoking it twice with different modified member access 
modifiers and performing part of the check itself.

java.lang.reflect.AccessibleObject#checkAccess delegates to 
Reflection.ensureMemberAccess and caches the result, so it had to be 
modified too.

Constructor now passes it's declaring class to the 'targetClass' 
parameter and Filed/Method obey the spec and REALLY IGNORE 'obj' 
parameter in get/set/invoke on a static member.

All java/lang/reflect and java/util/concurrent/atomic tests pass with 
this patch applied except the following:

java/lang/reflect/AccessibleObject/ModuleSetAccessibleTest.java: Test 
java.lang.reflect.AccessibleObject with modules
java/lang/reflect/Generics/TestBadSignatures.java: Test bad signatures 
get a GenericSignatureFormatError thrown.
java/lang/reflect/Method/invoke/TestPrivateInterfaceMethodReflect.java: 
Reflection support for private methods in interfaces
java/lang/reflect/Module/AddExportsTest.java: Test Module isExported 
methods with exports changed by -AddExportsTest
java/lang/reflect/Proxy/ProxyModuleMapping.java: Basic test of proxy 
module mapping and the access to Proxy class
java/lang/reflect/WeakPairMap/Driver.java: Functional test for WeakPairMap

...which all fail because of:

     javac: invalid flag: -XaddExports:java.base/jdk.internal....



Regards, Peter