Code review request for SecurityManager changes

[Roger Riggs]

Hi Sean,

I suppose you are mostly looking for correctness comments but I had couple
of performance observations.

1) I'm not familiar with the normal patterns used by the JDK but I would 
reverse the
order of the tests for a SecurityManager and looking for '.' in the 
loadClass method.

It will run faster in the case where there is no ClassLoader since the load
and test of the static SecurityManager can be inlined or is always 
quicker than a scan of every
character in the string.


In jigsaw/Loader:

@@ -60,10 +63,18 @@
     // Primary entry point from VM
     //
     protected Class<?> loadClass(String cn, boolean resolve)
         throws ClassNotFoundException
     {
+       
+            SecurityManager sm = System.getSecurityManager();
+            if (sm != null) {
                  int i = cn.lastIndexOf('.');
+                if (i != -1) {
+                sm.checkPackageAccess(cn.substring(0, i));
+            }
+        }


2)  There are two doPriveleged calls and corresponding overhead when one 
would be sufficient.
    The method  org.openjdk.jigsaw.Loader Class<?> findClass(ModuleId 
mid, String cn)
    uses doPrivileged (line 182) to read the module info and then calls 
finishFindingClass() which also calls
    doPrivileged (line 212) to read the class contents. 

    Can the number of doPriveleged calls be reduced to 1 with a bit of 
code rearrangment?
    The security checks are not inexpensive and should be reduced if 
possible. 

Roger



Sean Mullan wrote:
> Reminder. If you have any comments, please send them by the end of 
> today. I plan to push this to the jigsaw repo on Monday since several 
> subsequent security tasks are dependent on this.
>
> More SecurityManager changes will be coming later so you will have 
> another chance to review them.
>
> Thanks,
> Sean
>
> On 5/4/10 5:32 PM, Sean Mullan wrote:
>> See
>> http://cr.openjdk.java.net/~mullan/jigsaw/webrevs/SecurityManager/webrev.00/ 
>>
>> for an initial set of changes to allow modules to be run with a
>> SecurityManager.
>>
>> ModuleClassLoader now extends SecureClassLoader.
>>
>> There is no CodeSigner support yet. That will be coming later after we
>> integrate signed modules.
>>
>> Thanks,
>> Sean
>>