On Thu, Apr 15, 2010 at 17:49, David Holmes <david.hol...@oracle.com> wrote: > Hi Martin, > > If this proceeds I think you need to include AtomicReferenceFieldUpdater in > this as well.
Agreed. I may have created some confusion because the test in my webrev did not actually demonstrate the problem. I have since fixed that. http://cr.openjdk.java.net/~martin/webrevs/openjdk7/Atomic-security/test/java/util/concurrent/atomic/AccessControl.java.html The problem happens even for well-behaved classes that only use atomic updaters to update their own fields. Now, they could work around the problem by creating their atomic updater inside a doPrivileged, but probably we intend for them not to have to do that. Martin > But this is not a clear cut issue (security never is!). > > If I understand the test program correctly the problem arises when the > target object's class was loaded by a different class-loader to the class > doing the AtomicXXXFieldUpdater creation. This seems reasonable as > getDeclaredField states: > > SecurityException - If a security manager, s, is present and any of the > following conditions is met: > * invocation of s.checkMemberAccess(this, Member.DECLARED) denies > access to the declared field > * the caller's class loader is not the same as or an ancestor of the > class loader for the current class and invocation of s.checkPackageAccess() > denies access to the package of this class > > So here we would not have package access. What puzzles me is then why our > following explicit checks pass: > > sun.reflect.misc.ReflectUtil.ensureMemberAccess( > caller, tclass, null, modifiers); > sun.reflect.misc.ReflectUtil.checkPackageAccess(tclass); > > It seems inconsistent that getDeclaredField's internal checks say you don't > have permission to access this field, while what should amount to the same > checks via ReflectUtil say you do. The question is: which check is wrong? > > David > ----- > > > Martin Buchholz said the following on 04/16/10 08:34: >> >> Hi java.util.concurrent security team, >> >> People are using Atomic field updaters to update fields in classes in >> other classloaders. >> >> Toby writes: >> >> We received a bug report for App Engine that AtomicLongFieldUpdater >> (and its sibling) were failing with RuntimePermission >> accessDeclaredMembers. Looking at the code, it appears that it is a >> bug for that permission to be required. AtomicLongFieldUpdater (and >> sibling) are already performing subsequent access checks that ensure >> that the caller isn't accessing a field it shouldn't be. I've attached >> a patch which moves the field lookups into doPrivileged blocks. >> >> ... >> >> There was a bug was filed by ehcache folks. I could follow up with >> them to see if I could get some pointers to actual usage, if you're >> looking for motivational reasoning. >> >> --- >> >> http://code.google.com/p/googleappengine/issues/detail?id=2769 >> >> GAE Bug Submitter writes: >> >> AtomicLongFieldUpdater is broken in the production sandbox. >> >> The simple test application that I wrote (atomically increments and prints >> a volatile long on each get request) works perfectly in the development >> environment. In production creating the field updater fails with an >> AccessControlException when the internal CASUpdater tries to do reflection >> (on a user class) in order to get a reference to the volatile field. >> >> I've attached the test app which works perfectly in development but fails >> in production. >> >> This issue is linked to an associated Ehcache issue: >> >> https://jira.terracotta.org/jira/browse/EHC-617 >> >> I have a modified experimental version of Toby's patch against openjdk7 >> here: >> >> http://cr.openjdk.java.net/~martin/webrevs/openjdk7/Atomic-security/ >> >> I'm afraid to touch this security stuff myself, >> especially where we might be loosening permissions, >> but in principle I agree with Toby. >> (Even though atomic field updaters were designed to be >> used within a module (whatever that is)) >> >> The doPrivileged should not be too costly, as it's >> only performed at updater creation time. >> >> Martin >
