Code review request for SecurityManager changes

[Roger Riggs]

Corrected...

Roger Riggs wrote:
> 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 SecurityManager 
> *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
>>>
>