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
