jdk.internal.reflect.ReflectionFactory and SecurityManager

[Paul Sandoz]

Hi Peter,

You beat me to it!

My plan was to define a private method, accepting the lookup and teleporting class, and encapsulating the doPrivileged block. Then i could call that method for all usages in TLR. It does mean three doPrivileged executions, but that is probably ok.

Thanks,
Paul.

> On 11 Jan 2017, at 03:14, Peter Levart <peter.levart at gmail.com> wrote:
> 
> Hi Paul,
> 
> On 01/11/2017 02:55 AM, Paul Sandoz wrote:
>> I am concerned it’s getting too late to twiddle with the security setting so i took the plunge and wrapped the privateLookupIn calls in doPrivileged blocks. It’s ugly but avoids the circularity (JPRT core/hotspot testing has not failed).
>> 
>> http://cr.openjdk.java.net/~psandoz/jdk9/8160710-thread-to-tlr/webrev/ <http://cr.openjdk.java.net/~psandoz/jdk9/8160710-thread-to-tlr/webrev/>
>> 
>> I’’ll resend this out for closer review by Martin and Doug.
> 
> You could simplify the static initializer in TLR.ThreadGroupCreator a bit:
> 
>     static {
>         try {
>             // Teleport the lookup to access fields in Thread and ThreadGroup
>             MethodHandles.Lookup[] l = java.security.AccessController.doPrivileged(
>                 new java.security.PrivilegedExceptionAction<MethodHandles.Lookup[]>() {
>                     public MethodHandles.Lookup[] run() {
>                         MethodHandles.Lookup lookup = MethodHandles.lookup();
>                         return new MethodHandles.Lookup[] {
>                             MethodHandles.privateLookupIn(Thread.class, lookup),
>                             MethodHandles.privateLookupIn(ThreadGroup.class, lookup)
>                         };
>                     }
>                 });
>             THREAD_GROUP =
>                 l[0].findVarHandle(Thread.class, "group", ThreadGroup.class);
>             THREAD_GROUP_PARENT =
>                 l[1].findVarHandle(ThreadGroup.class, "parent", ThreadGroup.class);
>         } catch (Exception e) {
>             throw new Error(e);
>         }
>     }
> 
> 
> Regards, Peter
> 
>> Paul.
>> 
>> 
>>> On 10 Jan 2017, at 05:00, Alan Bateman <Alan.Bateman at oracle.com> <mailto:Alan.Bateman at oracle.com> wrote:
>>> 
>>> 
>>> 
>>> On 09/01/2017 19:09, Paul Sandoz wrote:
>>>>> On 9 Jan 2017, at 05:36, Alan Bateman <Alan.Bateman at oracle.com> <mailto:Alan.Bateman at oracle.com> wrote:
>>>>> 
>>>>> On 05/01/2017 19:01, Paul Sandoz wrote:
>>>>> 
>>>>>> Hi,
>>>>>> 
>>>>>> I encountered some circularity issues with security manager and VarHandles, specifically when attempting to use the new MethodHandles.privateLookupIn, so say ThreadLocalRandom has access to fields in Thread [1].
>>>>>> 
>>>>>> When running with a security manager ThreadLocalRandom clinit depends on various security stuff that eventually depends on ConcurrentSkipListMap which depends on ThreadLocalRandom, whose static state has not been fully initialzed.
>>>>>> 
>>>>>> The current security permission check in MethodHandles.privateLookupIn may be a too blunt and possibly should be refined along similar lines to Lookup.checkSecurityManager e.g. no check should be needed for classes within the same module (since this is a refined/controlled/principled form of setAccessible, plus no pounding on final fields). That would solve the problem in this case. But, the general point remains, and i have not thought too hard if there are other ways to solve this.
>>>>>> 
>>>>> This would mean inconsistency with setAccessible where you need this blunt permission when suppressing access. Also I think Lookup.checkSecurityManager will do different checks when you don't have a full power lookup so it would mean adjusting the privateLookupIn.
>>>> Some adjustment would definitely be required, the perhaps the simplest solution is not to perform a security check if within the same module?
>>>> 
>>> It might be okay when the lookup is a full-power lookup to class in java.base and it's called to get a full-power look to some other class in java.base. However, generalizing this would probably need security eyes. Consider two libraries on the class path (same unnamed module) for example. Also the inconsistency with setAccessible where it always checks the permission when running with a security manager.
>>> 
>>> -Alan
>