This discussion fizzled out last time it happened (2011 or something?).
So to avoid that happening again, I'm going to (hopefully clearly and
in simple unambiguous terms) explain what the problem is, why it should
be fixed, why the problem occurs, and exactly what should be done to fix
the problem.
What the problem is
-------------------
The expectation that users have when creating atomic field updaters is
that they have full access to any field that plain field accesses would
have (I believe this was reasonably well-established - or at least
implied - by all sides in the prior email thread).
The problem, as observed by users, is that when you have code which
looks like this:
public class Foo {
private volatile int foo;
private static final AtomicIntegerFieldUpdater up =
AtomicIntegerFieldUpdater.newUpdater(Foo.class, "foo");
...
}
...if you run with a security manager, and Foo.class.getClassLoader() !=
AtomicIntegerFieldUpdater.class.getClassLoader(), you get an access
control exception unless Foo.class's ProtectionDomain includes the
"accessDeclaredMembers". This is very contrary to the primary
expectation of users.
Enter the obvious workaround - first grant the requisite permission,
then do this:
public class Foo {
private volatile int foo;
private static final AtomicIntegerFieldUpdater up =
AccessController.doPrivileged(
new PrivilegedAction<AtomicIntegerFieldUpdater>() {
public AtomicIntegerFieldUpdater run() {
return
AtomicIntegerFieldUpdater.newUpdater(Foo.class, "foo");
}
}
);
...
}
Problem solved, yes? No. The newUpdater() method uses getCallerClass()
to get the class which is requesting access to the field. Since the
caller class is actually Foo$1.class or some such, the equality check in
the constructor of the implementation fails, causing an extra dynamic
check to be run on every invocation.
Here's the better, but still awful, non-obvious workaround:
public class Foo {
private volatile int foo;
private static AtomicIntegerFieldUpdater getIt() {
return AtomicIntegerFieldUpdater.newUpdater(Foo.class, "foo");
}
private static final AtomicIntegerFieldUpdater up =
AccessController.doPrivileged(
new PrivilegedAction<AtomicIntegerFieldUpdater>() {
public AtomicIntegerFieldUpdater run() {
return getIt();
}
}
);
...
}
This solves the getCallerClass() problem, however it still requires that
Foo get the dangerous accessDeclaredMembers permission.
Why it is critical to fix this issue
------------------------------------
Any code which uses Atomic*FieldUpdaters cannot run in a security
manager environment (like, say, a standard Java EE configuration)
without potentially destructive global permissions. Period. This
reason should speak for itself.
Why the problem occurs
----------------------
The root of the problem traces back to
SecurityManager.checkMemberAccess(). This method is the one remaining
method in all of SecurityManager which uses the calling class context
(stack) in order to determine the nature of the access check that is
needed. Basically it assumes the stack will look like this:
Class Method
0: SecurityManager checkMemberAccess
1: java.lang.Class checkMemberAccess
2: java.lang.Class (some reflection API method call)
3: ???? <consuming user code>
So, it looks at the class of position 3, and checks that class's class
loader against the class loader of the class to whom access is being
requested. If the class loaders are equal, the check short-circuits and
exits (permission granted). Otherwise, we fall back to the permission
check on the whole access control context.
There are two problems with this. The first problem is general - the
assumption that positions 1 and 2 are java.lang.Class calls is never
actually verified, meaning if some user code (for some reason) wanted to
use this method to check the accessDeclaredMembers permission, it would
not be able to do so (granted this is fairly unlikely).
The second problem is specific to construction of Atomic*FieldUpdaters.
In this case the call stack looks like this:
Class Method
0: SecurityManager checkMemberAccess
1: java.lang.Class checkMemberAccess
2: java.lang.Class getDeclaredField
3: j.u.c.Atomic*FieldUpdater$*Impl <init>
4: j.u.c.Atomic*FieldUpdater newUpdater
5: ???? <consuming user code>
The class loader at position 3 on the stack belongs to JDK code, and
thus will probably have a null class loader or the system class loader.
If the user code is not a part of this class loader (and it almost
never is, in the real world), then the fast check will always fail,
which causes the fallback to the global permission check.
What should - no, *must* - be done to fix the problem
-----------------------------------------------------
I understand that in the Java 9 SDK we're likely to see improved stack
introspection APIs and mechanisms, which may allow this problem to be
solved in other more elegant ways. However, I'm focusing on the
existing code because this issue still really needs to be fixed in 8 and
7, and even 6 for anyone who may still be maintaining a tree therefor.
The fix must come in two parts.
* Part 1 - checkMemberAccess assumptions should be verified
This should simply amount to adding equality comparisons for frames 1
and 2 of the introspected call stack.
* Part 2 - check for atomic field updaters
The code in checkMemberAccess should examine positions 3 and 4 in the
call stack (after an appropriate bounds check). If they correspond to
an Atomic*FieldUpdater implementation and the base class itself,
respectively, then the class in position 5 should be used to verify the
class loader rather than position 3.
While this special-case fix is not pretty, it does solve the issue and
does not measurably worsen the already somewhat fragile existing code.
It also solves a potentially serious security issue, by removing the
need to grant potentially dangerous globally effective permissions to
any code using these otherwise-innocuous atomic constructs. There is
little danger of this kind of fix creeping out to other constructs,
simply because there are no other similar constructs with the same
problem in the JDK.
Thank you for your consideration.
--
- DML
