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
