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

[Peter Levart]

Hi,

I added an exhaustive jtreg test that covers all possible situations.

 From the set of the following classes:

package a;
public class PublicSuper {...}

package a;
class Package {...}

package b;
public class PublicSub extends a.PublicSuper {...}

package b;
class Package {...}

it creates a set of all possible triplets:

(currentClass, memberClass, targetClass)

where:

currentClass - the class making the reflective access
memberClass - the member's declaring class
targetClass - the target object's class (for accessing instance fields 
and methods - must be equal to or subclass of memberClass)

For each such triplet it checks the reflective access to each of the 
following members:

{private, package, protected, public} x {instance, static} x {field, method}

and:

  {private, package, protected, public} x {constructor}

When running on unpatched build 9-ea+137, the result is:

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

When running on patched build of 9, the result is:

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


The difference is exactly in the following two cases which fail for 
unpatched and are fixed by the patch:

b.PublicSub accessing a.PublicSuper's PROTECTED_STATIC_FIELD - expected 
allowed, actual denied : FAILURE
b.PublicSub accessing a.PublicSuper's PROTECTED_STATIC_METHOD - expected 
allowed, actual denied : FAILURE

QED.


This is new webrev:

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

I think the test proves the effect of the patch is as intended, 
therefore it should not be a problem to review it.


Regards, Peter


On 10/01/2016 12:20 AM, Peter Levart wrote:
> 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
>
>