Review request for the incorrect check for "getClassLoader" permission

[Paul Sandoz]

On Jun 25, 2012, at 7:00 PM, Mandy Chung wrote:

> 
> 
> On 6/23/2012 1:05 PM, Alan Bateman wrote:
>> On 21/06/2012 20:02, Mandy Chung wrote:
>>> David, Paul,
>>> 
>>> I have a fix for the incorrect check w.r.t. "getClassLoader" permission [1] and also update j.u.c.atomic for module mode.
>>> 
>>> Webrev at:
>>> http://cr.openjdk.java.net/~mchung/jigsaw/webrevs/getclassloader-permission-fix/
>> I agree with David that"doClassLoaderPermissionCheck" is a bit odd given that it returns a boolean.
>> 
> 
> I rename it to "needsClassLoaderPermissionCheck" as David suggests.
> 
>> In Class.checkMemberAccess L2187 it might be better to swap the expressions so that it reads:
>> 
>>  if (!Platform.isPlatformLoader(ccl) && (ccl != cl) &&
>> 
>> It doesn't change anything of course but some of these checks are subtle so this gives you the minimum change against jdk8.
>> 
>> Same comment on Atomic* classes where it would be very easy to break something in a merge.
> 
> I see your concern of the potential mistake in merging with jdk8.   As we plan to push some jdk changes in jigsaw to jdk8 regularly to keep jigsaw repo with minimum change against jdk8, to prepare for it - what do you think if I add a new method sun.misc.VM.isSystemDomain(ClassLoader) something like that?  The jdk8 version of this method will just check cl == null.
> 
>> Just on thought on the Atomic* changes: given that this is maintained in Doug's CVS and then pulled into OpenJDK periodically then maybe we should just create a package private class in j,u.c with one static method like Platform.isPlatformLoader. This could make life easier for a few people.
>> 
> 
> Would the new sun.misc.VM.isSystemDomain method help?  That would avoid having j.u.c. to reference org.openjdk.jigsaw code.
> 

Should we encapsulate the following into a method?
+                if (ccl != cl && !Platform.isPlatformLoader(ccl) &&
+                        (Platform.isPlatformLoader(cl) || !isAncestor(cl, ccl)))
  if (Platform.needsPackageAccessCheck(ccl, cl))

Because it is rather subtle i think it useful to stuff the implementation (and document) in one place, plus we could decide to change the impl e.g. if both CLs are module CLs then it always returns false.

Paul.