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

[David Holmes]

On Thu, 4 Feb 2021 19:12:32 GMT, Coleen Phillimore <coleenp at openjdk.org> wrote:

> This change does not call up to Java for checkPackageAccess if the security manager is NULL, but still saves the protection domain in the pd_set for that dictionary entry.  If the option -Djava.security.manager=disallow is set, that means that there will never be a security manager and the JVM code can avoid saving the protection domains completely. 
> See the two functions java_lang_System::has_security_manager() and java_lang_System::allow_security_manager() for details.
> Also deleted ProtectionDomainVerification because there's no use for this option.
> 
> Tested with tier1 hotspot, jdk and langtools.
> and tier2-6.

Hi Coleen,

Basic optimization looks good. I have a concern over the locking change. Some minor comments below.

Thanks,
David

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

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

src/hotspot/share/classfile/systemDictionary.cpp line 503:

> 501:     } else {
> 502:      log_debug(protectiondomain)("granted");
> 503:     }

Did you intend leaving this in?

test/hotspot/jtreg/runtime/logging/ProtectionDomainVerificationTest.java line 2:

> 1: /*
> 2:  * Copyright (c) 2016, 2020, Oracle and/or its affiliates. All rights reserved.

2021 :)

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.

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

Changes requested by dholmes (Reviewer).

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