> On 11 Jan 2017, at 14:00, Martin Buchholz <marti...@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.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.
>
