[concurrency-interest] Here's why Atomic*FieldReference access checking is broken

[Peter Levart]

On 10/04/2014 05:15 AM, Alan Bateman wrote:
> On 03/10/2014 08:13, David M. Lloyd wrote:
>> :
>>
>> 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.
> Are you sure you see this in JDK 8 too? I ask because I remember David 
> Holmes changed the Atomic*Updater methods to call getDeclaredField in 
> a privileged block (JDK-7103570). Also there work in JDK 8 on caller 
> sensitive methods (JEP 176). As part of this then SM.checkMemberAccess 
> was deprecated and usages in the JDK dropped (Class.getDeclaredField 
> and the others no longer use it).
>
> -Alan.
>

Hi,

It seems that construction-time access checks are fine in JDK9 and 8u20. 
But I found a corner case where invocation-time access check is overly 
restrictive. Take for example the following code:

package test;

import java.util.concurrent.atomic.AtomicIntegerFieldUpdater;

public class Test {

     static class A {
         protected volatile int x;
     }

     static class B extends A {
         static void test() {
             A a = new A();
             B b = new B();

             b.x = 10; // OK

             a.x = 10; // OK

             AtomicIntegerFieldUpdater<A> xUpdater =
                 AtomicIntegerFieldUpdater.newUpdater(A.class, "x"); // OK

             xUpdater.set(b, 10); // OK

             xUpdater.set(a, 10); // IllegalAccessException:
             // Class test.Test$B can not access a protected member of class
             // test.Test$A using an instance of test.Test$A
         }
     }

     public static void main(String[] args) {
         B.test();
     }
}


Here we see that Java allows accessing protected field A.x from class B 
using either instance of A or instance of B. That's because B is in the 
same package as A .

Atomic*FieldUpdater check is therefore overly restrictive. If the 
'protected' modifier is removed from A.x field, the test passes.

So here's a preview of how this could be fixed:

http://cr.openjdk.java.net/~plevart/jdk9-dev/AtomicFieldUpdater.AccessChecks/webrev.01/

I have just made a fix for AtomicIntegerFieldUpdater. Other 
Atomic*FieldUpdaters have similar code. The fix also includes a 
simplification of invocation-time access check. In original code, 
'cclass' is non-null and equal to 'caller' only in case when the field 
is protected and 'caller' class is different from 'tclass', meaning that 
fullCheck() is always called in such cases. fullCheck() in such cases 
checks that 'obj' is instanceof 'tclass' and 'caller' at the same time. 
But in such cases, 'caller' is also a subclass of 'tclass' or else the 
construction-time checks would fail.

So I think that we can get away with only one instanceof check. In 
patched code the class to check against is equal to 'tclass' if we are 
not performing a protected field access check or 'caller' if protected 
access is checked. So if we change the meaning of 'cclass' from:

             this.cclass = (Modifier.isProtected(modifiers) &&
                            caller != tclass) ? caller : null;

to:

             this.cclass = (Modifier.isProtected(modifiers)) ? caller : 
tclass;

we can change the invocation-time check from:

             if (obj == null || obj.getClass() != tclass || cclass != 
null) fullCheck(obj);

to:

             if (!cclass.isInstance(obj)) failCheck(obj);

That's the whole check, so fullCheck() becomes failCheck() which always 
throws exception (the type of which is determined from the runtime class 
of 'obj' instance).

Now is this check fast enough? It seems so. The Class.isInstance() is 
intrinsified. I checked with the following benchmark (the results on my 
i7 Linux PC are attached as comments):

http://cr.openjdk.java.net/~plevart/jdk9-dev/AtomicFieldUpdater.AccessChecks/AtomicUpdaterBench.java

The 1st and 2nd report should be compared. It seems that get() is even 
faster with simplified check while other methods are the same.

The 3rd report shows a result of experimental AtomicIntegerFieldUpdater 
implementation which loads new VM-anonymous class for each new instance 
which allows VM compiler to specialize code for a particular field. Such 
implementation is nearly as fast as Java field access. This is just a 
proof of concept. A little hack-ish, doesn't include the fix for the 
overly restrictive protected access yet, but here it is if anyone is 
interested:

  http://cr.openjdk.java.net/~plevart/jdk9-dev/AtomicFieldUpdater.AccessChecks/AnonClassPerInstance/AtomicIntegerFieldUpdater.java


Regards, Peter

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/security-dev/attachments/20141005/4f564c9d/attachment.htm>