Doug, please try to remember the details of the thread group code.
On Wed, Jan 11, 2017 at 3:13 PM, Paul Sandoz <paul.san...@oracle.com> wrote:
>
> > On 11 Jan 2017, at 14:00, Martin Buchholz <marti...@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.san...@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.
> >
>
>
