Hi Martin,
If this proceeds I think you need to include AtomicReferenceFieldUpdater
in this as well.
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
