RFR 8160710: Enable Thread to grant VarHandle field access to ThreadLocalRandom/Striped64

[Martin Buchholz]

Doug, please try to remember the details of the thread group code.


On Wed, Jan 11, 2017 at 3:13 PM, Paul Sandoz <paul.sandoz at oracle.com> wrote:

>
> > On 11 Jan 2017, at 14:00, Martin Buchholz <martinrb at google.com> wrote:
>
> > Our inline assignment style is non-standard, but we like it and it saves
> a byte of bytecode occasionally, making things a tiny bit easier for the
> JIT.
> >
>
> Are you referring to cases like:
>
>  964         int r = (int) SECONDARY.get(t);
>  965         if (r != 0) {
>
> ?
>
> I moved it out to make it clearer on the cast, but i can inline it as you
> prefer.
>
>
Yes. jsr166 style is to inline, even though we don't recommend it for
regular java code.


> ---
> >
> > The threadgroup creation code that uses cheating has always seemed fishy
> to me.  The straightforward
> >
> >     /**
> >      * Returns a new group with the system ThreadGroup (the
> >      * topmost, parent-less group) as parent.
> >      */
> >     static final ThreadGroup createThreadGroup(String name) {
> >         if (name == null)
> >             throw new NullPointerException();
> >         ThreadGroup group = Thread.currentThread().getThreadGroup();
> >         for (ThreadGroup p; (p = group.getParent()) != null; )
> >             group = p;
> >         return new ThreadGroup(group, name);
> >     }
> >
> > passes all tests.  That could be wrapped in a doPrivileged, or we could
> add something more principled to ThreadGroup itself.  I don't know what the
> motivations are for the threadgroup code.
> >
>
> Good point, me neither, nor the association with TLR (perhaps because
> there are over methods used by InnocuousForkJoinWorkerThread, see below).
> The Unsafe usage rolled in with:
>
>   https://bugs.openjdk.java.net/browse/JDK-8157523
>   http://hg.openjdk.java.net/jdk9/dev/jdk/rev/fd4819ec5afd
>
>
> In ForkJoinWorkerThread:
>
> static final class InnocuousForkJoinWorkerThread extends
> ForkJoinWorkerThread {
>     /** The ThreadGroup for all InnocuousForkJoinWorkerThreads */
>     private static final ThreadGroup innocuousThreadGroup =
>         ThreadLocalRandom.createThreadGroup("
> InnocuousForkJoinWorkerThreadGroup”);
>
>
> In ForkJoinPool:
>
> private ForkJoinPool(byte forCommonPoolOnly) {
> ...
>     ForkJoinWorkerThreadFactory fac = null;
> ...
>     if (fac == null) {
>         if (System.getSecurityManager() == null)
>             fac = defaultForkJoinWorkerThreadFactory;
>         else // use security-managed default
>             fac = new InnocuousForkJoinWorkerThreadFactory();
>     }
> …
>
>
> private static final class InnocuousForkJoinWorkerThreadFactory
>     implements ForkJoinWorkerThreadFactory {
> ...
>     public final ForkJoinWorkerThread newThread(ForkJoinPool pool) {
>         return AccessController.doPrivileged(new PrivilegedAction<>() {
>             public ForkJoinWorkerThread run() {
>                 return new ForkJoinWorkerThread.
>                     InnocuousForkJoinWorkerThread(pool);
>             }}, INNOCUOUS_ACC);
>     }
> }
>
>
> So everything is wrapped in a doPrivileged block.
>
> My guess as at some point in development it was important to efficiently
> bypass the security check of ThreadGroup.getParent, and Thread.getGroup
> came along for the ride.
>
> AFAICT I don’t think we need it, and it would be nice to remove the
> VarHandle code and move createThreadGroup to be a static method of
> InnocuousForkJoinWorkerThread. How about:
>
>
> static final class InnocuousForkJoinWorkerThread extends
> ForkJoinWorkerThread {
>     /** The ThreadGroup for all InnocuousForkJoinWorkerThreads */
>     private static final ThreadGroup innocuousThreadGroup =
>             createThreadGroup();
> …
>     private static ThreadGroup createThreadGroup() {
>         ThreadGroup currentGroup = Thread.currentThread().
> getThreadGroup();
>         ThreadGroup topGroup = java.security.
> AccessController.doPrivileged(
>                 new java.security.PrivilegedAction<ThreadGroup>() {
>                     public ThreadGroup run() {
>                         ThreadGroup group = currentGroup;
>                         for (ThreadGroup p; (p = group.getParent()) !=
> null; )
>                             group = p;
>                         return group;
>                     }});
>         return new ThreadGroup(topGroup, "InnocuousForkJoinWorkerThreadG
> roup");
>     }
> }
>
> That also passes the tests we have for the Innocuous case.
>
>
That seems good to me.


>
> > ---
> >
> >  431             // Teleport the lookup to access fields in Thread
> >
> > Teleport?!  Maybe "Obtain a private lookup object" but that should be
> obvious from the name “privateLookupIn”
> >
>
> Yes, i removed the comments.
>
> Thanks,
> Paul.
>
>
> >
> > On Wed, Jan 11, 2017 at 12:21 PM, Paul Sandoz <paul.sandoz at oracle.com>
> wrote:
> > Hi,
> >
> > Please review:
> >
> >   http://cr.openjdk.java.net/~psandoz/jdk9/8160710-thread-to-tlr/webrev/
> >
> > This patch updates ThreadLocalRandom, Striped64 and LockSupport to use
> VarHandles instead of Unsafe to access fields in Thread, via the
> MethodsHandles.privateLookupIn method.
> >
> > Since there are three usages of MethodsHandles.privateLookupIn in
> ThreadLocalRandom i consolidated the doPrivileged block in a private static
> helper method, being paranoid i placed some checks to avoid this method
> being potentially abused to circumvent the security check, perhaps that is
> overkill?
> >
> > I ran this patch though our test infra a few times and have not observed
> any transient failures.
> >
> > Thanks,
> > Paul.
> >
>
>