Jeff, I totally agree with your analysis.
I'd like to see better javadoc for java.util.concurrent.atomic in general. I'm not sure I'm the best person to come up with the kind of security-related javadoc we want to see here. The key idea is that, like an instance of Unsafe, an updater is a token that gives its holder special powers to update the field, inherited from the immediate creator of the updater. Martin On Wed, Apr 21, 2010 at 17:30, Jeff Nisewanger <jeffrey.nisewan...@oracle.com> wrote: > On 4/16/2010 5:50 AM, David Holmes wrote: >> >> Hi Doug, >> >> <aside: FYI copies of my replies to security-dev are being held for >> approval as I'm not a subscriber.> >> >> Doug Lea said the following on 04/16/10 21:43: >>> >>> On 04/15/10 18:34, Martin Buchholz wrote: >>> >>>> People are using Atomic field updaters to update fields in classes in >>>> other classloaders. >>>> >>> >>> I think the policy on this awaits interpretation by Jeff >>> or other members of security team. FWIW, my take is that >>> if users know that they may cross class loaders, then they >>> should wrap these in doPrivileged anyway. As in ... >> >> I'm coming around to agreeing with the proposed fix. My take is that the >> real security check should take place at the time the field is set: >> >> field_x_updater.set(obj, val); >> >> At this point the calling code must have the necessary permissions to set >> field x of the given obj of type T. And I believe we do indeed check this. >> >> When the AtomicXXXXFieldUpdater constructor binds itself to the Field >> object for T.x that's an optimization. There's no reason we couldn't do this >> on each call to set() - other than it would perform terribly. So in that >> sense the security checks that take place at construction are incidental** >> and so we should be as permissive as we can make them _provided_ that the >> actual set() call will make the necessary permission checks. >> >> ** This particular check is also incidental because we happen to use a >> public reflection method to get the Field object. We could just as easily >> have used a magic VM hook. >> > > This is describing the security checking philosophy of the > java.lang.reflect apis > which mimic the security semantics of static bytecode at the point at which > they > are dynamically invoked. They perform the full security > check on every get() or set() method. This has a substantial performance > penalty > for various reasons but it allows java.lang.reflect.Field instances to be > freely passed > around internally within an application or library's implementation classes > since the > actual security check is against the caller of the get/set() method. Static > bytecode > doesn't have these performance issues since the check is performed once at > constant pool resolution time and the calling point is inherently bound to > that class. > > On the other hand, the java.util.concurrent.atomic APIs were designed to > allow > highly efficient atomic access where performing a full security check on the > set() > method would be a substantial performance burden. Therefore, all of the > access-oriented > security checks are performed at construction time and the set() method (for > example) > only performs type checks to ensure the integrity of the field offset within > the > enclosing object. > > I vaguely recall discussions from years past about the need to improve > the security-relevant > aspects of the javadoc so this distinction would be clear to developers > using the API. > However, I'm not seeing any of this in the jdk 7 docs. This needs to be > fixed. (!) > > The current webrev looks reasonable to me aside from the need to improve > the javadoc. > > > Jeff >
