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

[Peter Levart]

To a potential reviewer...

The change might look scary at first as it touches reflective access 
controls and by that, security of the platform, but:

- it is just (32 ins; 56 del; 62 mod) lines. The rest is a new jtreg 
test which is beneficial by itself as such test did not exist until now.
- the jtreg test is exhaustive and proves that the patch does not have 
undesired effects.

Thanks,

Peter Levart

On 10/02/2016 11:51 PM, Peter Levart wrote:
> 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
>>
>>
>