RFR: 8195744: Avoid calling ClassLoader.checkPackageAccess if security manager is not installed [v2]

[Coleen Phillimore]

On Fri, 5 Feb 2021 01:45:58 GMT, David Holmes <dholmes at openjdk.org> wrote:

>> Coleen Phillimore has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Add comment and read NEVER field from System
>
> src/hotspot/share/classfile/dictionary.cpp line 145:
> 
>> 143: #ifdef ASSERT
>> 144:   if (protection_domain == instance_klass()->protection_domain()) {
>> 145:     MutexLocker ml(ProtectionDomainSet_lock, Mutex::_no_safepoint_check_flag);
> 
> By splitting the lock scope into two blocks you have lost the atomicity of the entire action in a debug build, and now you check a potentially different pd_set().

If the dictionary entry's class matches the protection domain, then it should never be added to the pd_set list.  So it doesn't need to be locked to check that.  It doesn't need atomicity.

> src/hotspot/share/classfile/javaClasses.cpp line 4406:
> 
>> 4404: }
>> 4405: 
>> 4406: // This field means that a security manager is never installed so we can completely
> 
> This field tells you whether a SM can be installed, and if not then you can completely ...

updated with "we"

> src/hotspot/share/classfile/systemDictionary.cpp line 503:
> 
>> 501:     } else {
>> 502:      log_debug(protectiondomain)("granted");
>> 503:     }
> 
> Did you intend leaving this in?

Yes, why not? It's very useful in logging.

> test/hotspot/jtreg/runtime/logging/ProtectionDomainVerificationTest.java line 2:
> 
>> 1: /*
>> 2:  * Copyright (c) 2016, 2020, Oracle and/or its affiliates. All rights reserved.
> 
> 2021 :)

I had a bug in my fix-copyrights script.

> test/hotspot/jtreg/runtime/logging/ProtectionDomainVerificationTest.java line 47:
> 
>> 45:         .shouldContain("[protectiondomain] Checking package access")
>> 46:         .shouldContain("[protectiondomain] pd set count = #")
>> 47:         .shouldHaveExitValue(0);
> 
> Minor nit: checking the exit value first can be more informative in case of a crash.

The patterns I see have that last. (?)

-------------

PR: https://git.openjdk.java.net/jdk/pull/2410