Code review request for SecurityManager changes

[Sean Mullan]

On 5/7/10 2:03 PM, Roger Riggs wrote:
> 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.

Seems reasonable to me. That code was copied directly from AppClassLoader.loadClass.

>> 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.

I might be able to remove the doPriv from finishFindingClass and carefully widen 
the scope of the doPriv in findClass. I'll take a closer look.

Thanks,
Sean