Here's why Atomic*FieldReference access checking is broken

[David M. Lloyd]

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