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

[Paul Sandoz]

> On 11 Jan 2017, at 14:00, Martin Buchholz <martinrb at google.com> wrote:
> 
> Thanks, Paul.
> 
> Looks good.  Nits:
> 
> ---
> 
> 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.


> ---
> 
> 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, "InnocuousForkJoinWorkerThreadGroup");
    }
}

That also passes the tests we have for the Innocuous case.


> ---
> 
>  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.
>