jdk.internal.reflect.ReflectionFactory and SecurityManager
David Holmes
david.holmes at oracle.com
Wed Jan 4 07:27:39 UTC 2017
Hi Peter,
> ...so the only way for this to not cause the same problem is for
> Class.reflectionFactory field to be initialized before SecurityManager
> is set.
>
> Was this pure luck and we were just waiting for a situation where this
> was not the case?
I don't know. If you run with -Xlog:class+init=trace you will see the
normal initialization order, and can then compare with and without a
security manager; with and without assertions enabled etc etc.
I'm getting more and more paranoid about initialization order
dependencies and the overall fragility of it all (eg that no class
initialized before java.lang.Class can use assertions).
And its even worse when you consider that JVM TI can totally change
initialization order in unexpected ways that can lead to total breakage.
Cheers,
David
On 4/01/2017 5:16 PM, Peter Levart wrote:
> Hi David,
>
> On 12/26/2016 10:16 PM, David Holmes wrote:
>> Hi Peter,
>>
>> I know this is response to the problems with your other recent change,
>> but this is an enhancement in my opinion, not a bug fix, and the time
>> for enhancements is passed - though there is still a process to
>> request them. I do not like to see last minutes changes like this
>> where not enough due diligence may be done to identify all the
>> ramifications.
>
> Since the initialization cycle has now been broken by fix/simplification
> (JDK-8172048), there is no imminent need to pursue this one at this
> time, but...
>
>>
>> The Class.getMethod() fixes seem to have overstepped the line in that
>> regard as well, in my opinion. Anything that potentially changes
>> initialization order is fragile and risky and requires additional
>> testing.
>
> ...I was surprised that this actually happened at first. The
> initialization cycle was apparently introduced by my introduction of new
> class (java.lang.PublicMethods$Key) which in its <clinit> requests
> ReflectionFactory which does a permission check and all that is
> triggered from assertion in java.lang.invoke.VarHandle$AccessMode
> constructor which uses Class::getMethod to verify the assertion
> condition...
>
> Here's the stack trace, repeated here for clarity:
>
> Error occurred during initialization of VM
> java.lang.ExceptionInInitializerError
> at
> java.lang.PublicMethods$MethodList.filter(java.base at 9-internal/PublicMethods.java:151)
>
> at
> java.lang.Class.getMethodsRecursive(java.base at 9-internal/Class.java:3191)
> at java.lang.Class.getMethod0(java.base at 9-internal/Class.java:3175)
> at java.lang.Class.getMethod(java.base at 9-internal/Class.java:2036)
> at
> java.lang.invoke.VarHandle$AccessMode.getReturnType(java.base at 9-internal/VarHandle.java:1826)
>
> at
> java.lang.invoke.VarHandle$AccessMode.<init>(java.base at 9-internal/VarHandle.java:1792)
>
> at
> java.lang.invoke.VarHandle$AccessMode.<clinit>(java.base at 9-internal/VarHandle.java:1590)
>
> at
> java.lang.invoke.VarForm.linkFromStatic(java.base at 9-internal/VarForm.java:127)
>
> at java.lang.invoke.VarForm.<init>(java.base at 9-internal/VarForm.java:50)
> at
> java.lang.invoke.VarHandleObjects$FieldInstanceReadOnly.<clinit>(java.base at 9-internal/VarHandleObjects.java:84)
>
> at
> java.lang.invoke.VarHandles.makeFieldHandle(java.base at 9-internal/VarHandles.java:38)
>
> at
> java.lang.invoke.MethodHandles$Lookup.getFieldVarHandleCommon(java.base at 9-internal/MethodHandles.java:2241)
>
> at
> java.lang.invoke.MethodHandles$Lookup.getFieldVarHandle(java.base at 9-internal/MethodHandles.java:2201)
>
> at
> java.lang.invoke.MethodHandles$Lookup.findVarHandle(java.base at 9-internal/MethodHandles.java:1361)
>
> at
> java.util.concurrent.atomic.AtomicReference.<clinit>(java.base at 9-internal/AtomicReference.java:57)
>
> at java.security.Policy.<clinit>(java.base at 9-internal/Policy.java:111)
> at
> java.security.AccessControlContext.getDebug(java.base at 9-internal/AccessControlContext.java:110)
>
> at
> java.security.AccessControlContext.checkPermission(java.base at 9-internal/AccessControlContext.java:398)
>
> at
> java.security.AccessController.checkPermission(java.base at 9-internal/AccessController.java:894)
>
> at
> java.lang.SecurityManager.checkPermission(java.base at 9-internal/SecurityManager.java:548)
>
> at
> java.lang.SecurityManager.checkPropertyAccess(java.base at 9-internal/SecurityManager.java:1292)
>
> at java.lang.System.getProperty(java.base at 9-internal/System.java:761)
> at
> java.lang.ClassLoader.initSystemClassLoader(java.base at 9-internal/ClassLoader.java:1902)
>
> at java.lang.System.initPhase3(java.base at 9-internal/System.java:1979)
> Caused by: java.lang.NullPointerException
> at java.security.Policy.isSet(java.base at 9-internal/Policy.java:126)
> at
> java.security.AccessControlContext.getDebug(java.base at 9-internal/AccessControlContext.java:110)
>
> at
> java.security.AccessController.checkPermission(java.base at 9-internal/AccessController.java:871)
>
> at
> java.lang.SecurityManager.checkPermission(java.base at 9-internal/SecurityManager.java:548)
>
> at
> jdk.internal.reflect.ReflectionFactory.getReflectionFactory(java.base at 9-internal/ReflectionFactory.java:132)
>
> at
> jdk.internal.reflect.ReflectionFactory$GetReflectionFactoryAction.run(java.base at 9-internal/ReflectionFactory.java:106)
>
> at
> jdk.internal.reflect.ReflectionFactory$GetReflectionFactoryAction.run(java.base at 9-internal/ReflectionFactory.java:103)
>
> at
> java.security.AccessController.doPrivileged(java.base at 9-internal/Native
> Method)
> at
> java.lang.PublicMethods$Key.<clinit>(java.base at 9-internal/PublicMethods.java:90)
>
> at
> java.lang.PublicMethods$MethodList.filter(java.base at 9-internal/PublicMethods.java:151)
>
> at
> java.lang.Class.getMethodsRecursive(java.base at 9-internal/Class.java:3191)
> at java.lang.Class.getMethod0(java.base at 9-internal/Class.java:3175)
> at java.lang.Class.getMethod(java.base at 9-internal/Class.java:2036)
> at
> java.lang.invoke.VarHandle$AccessMode.getReturnType(java.base at 9-internal/VarHandle.java:1826)
>
> at
> java.lang.invoke.VarHandle$AccessMode.<init>(java.base at 9-internal/VarHandle.java:1792)
>
> at
> java.lang.invoke.VarHandle$AccessMode.<clinit>(java.base at 9-internal/VarHandle.java:1590)
>
> at
> java.lang.invoke.VarForm.linkFromStatic(java.base at 9-internal/VarForm.java:127)
>
> at java.lang.invoke.VarForm.<init>(java.base at 9-internal/VarForm.java:50)
> at
> java.lang.invoke.VarHandleObjects$FieldInstanceReadOnly.<clinit>(java.base at 9-internal/VarHandleObjects.java:84)
>
> at
> java.lang.invoke.VarHandles.makeFieldHandle(java.base at 9-internal/VarHandles.java:38)
>
> at
> java.lang.invoke.MethodHandles$Lookup.getFieldVarHandleCommon(java.base at 9-internal/MethodHandles.java:2241)
>
> at
> java.lang.invoke.MethodHandles$Lookup.getFieldVarHandle(java.base at 9-internal/MethodHandles.java:2201)
>
> at
> java.lang.invoke.MethodHandles$Lookup.findVarHandle(java.base at 9-internal/MethodHandles.java:1361)
>
> at
> java.util.concurrent.atomic.AtomicReference.<clinit>(java.base at 9-internal/AtomicReference.java:57)
>
> at java.security.Policy.<clinit>(java.base at 9-internal/Policy.java:111)
> at
> java.security.AccessControlContext.getDebug(java.base at 9-internal/AccessControlContext.java:110)
>
> at
> java.security.AccessControlContext.checkPermission(java.base at 9-internal/AccessControlContext.java:398)
>
> at
> java.security.AccessController.checkPermission(java.base at 9-internal/AccessController.java:894)
>
> at
> java.lang.SecurityManager.checkPermission(java.base at 9-internal/SecurityManager.java:548)
>
> at
> java.lang.SecurityManager.checkPropertyAccess(java.base at 9-internal/SecurityManager.java:1292)
>
> at java.lang.System.getProperty(java.base at 9-internal/System.java:761)
> at
> java.lang.ClassLoader.initSystemClassLoader(java.base at 9-internal/ClassLoader.java:1902)
>
> at java.lang.System.initPhase3(java.base at 9-internal/System.java:1979)
>
>
> But even before my change, Class::getMethod needed access to
> ReflectionFactory instance. To remind: each reflective object (Method,
> Field, Constructor) is copied before handed out of a public method such
> as Class::getMethod. Copying is performed by ReflectionFactory.
> ReflectionFactory in java.lang.Class is obtained lazily:
>
> // Fetches the factory for reflective objects
> private static ReflectionFactory getReflectionFactory() {
> if (reflectionFactory == null) {
> reflectionFactory =
> java.security.AccessController.doPrivileged
> (new ReflectionFactory.GetReflectionFactoryAction());
> }
> return reflectionFactory;
> }
> private static ReflectionFactory reflectionFactory;
>
> ...so the only way for this to not cause the same problem is for
> Class.reflectionFactory field to be initialized before SecurityManager
> is set.
>
> Was this pure luck and we were just waiting for a situation where this
> was not the case?
>
> It is good that this danger is now past as java.security.Policy does not
> need AtomicReference any more.
>
> Regards, Peter
>
>>
>> That said, I am an admirer of your work on OpenJDK! :)
>>
>> Cheers,
>> David
>>
>>
>> On 27/12/2016 5:29 AM, Peter Levart wrote:
>>> Hi,
>>>
>>> There are 2 ReflectionFactory classes in JDK 9. The old one is
>>> sun.reflect.ReflectionFactory which ended in jdk.unsupported module and
>>> to which access is restricted with SecurityManager. There is also new
>>> jdk.internal.reflect.ReflectionFactory which is used internally by JDK,
>>> is exported to internal modules only but still uses SecurityManager to
>>> restrict access to itself. I checked all usages and they all use
>>> AccessControler.doPrivileged() for obtaining the instance of
>>> jdk.internal.reflect.ReflectionFactory, which somehow defeats the
>>> purpose of SecurityManager access checks in this API.
>>>
>>> I think this could be simplified by removing the SecurityManager check
>>> from jdk.internal.reflect.ReflectionFactory#getReflectionFactory static
>>> method and change all usages to invoke this method directly without
>>> doPrivileged(). There are already two sensitive internal APIs exposed
>>> without SecurityManager checks: jdk.internal.misc.Unsafe#getUnsafe and
>>> various jdk.internal.misc.SharedSecrets#getXxxAccess methods. So why
>>> wouldn't internal ReflectionFactory be exposed the same way?
>>>
>>> This would make obtaining the ReflectionFactory more robust and not
>>> sensitive to bootstrap issues that surfaced recently after my push of a
>>> fix for issues 8062389, 8029459, 8061950.
>>>
>>> So, what do you think? Is this a worthwhile cleanup and simplification?
>>>
>>> Regards, Peter
>>>
>
More information about the core-libs-dev
mailing list