Re: RFR: 8195744: Avoid calling ClassLoader.checkPackageAccess if security manager is not installed [v3]
On Fri, 5 Feb 2021 03:00:05 GMT, David Holmes wrote: >> Coleen Phillimore has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Fix comments and copyright. > > src/hotspot/share/classfile/javaClasses.cpp line 4415: > >> 4413: >> 4414: // This field means that a security manager can be installed so we >> still have to >> 4415: // populate the ProtectionDomainCacheTable. > > No this field returns the installed SM if any. It doesn't tell you anything > about whether you can install a SM or not (though obviously if non-NULL then > you could). // This field tells us that a security manager is installed. How about I just put this. I had a bug earlier that this explained to me but the allow_security_manager() also explains it. > src/java.base/share/classes/java/lang/System.java line 163: > >> 161: >> 162: // indicates if a security manager is possible >> 163: // @implNote The HotSpot JVM hardcodes the value of NEVER. > > You don't need this if the VM reads the value of NEVER. ok. - PR: https://git.openjdk.java.net/jdk/pull/2410
Re: RFR: 8195744: Avoid calling ClassLoader.checkPackageAccess if security manager is not installed [v3]
On Fri, 5 Feb 2021 02:28:31 GMT, Coleen Phillimore wrote:
>> 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.
There's no context for this final bit out of output. It could appear many lines
after the earlier logging, and unless there is some locking around all this,
could be interleaved with different PD checks. It seems to me that all the
logging could occur after the Java call and include the success/failure. It
could also include whether the check trivially succeeded due to there being no
SM.
-
PR: https://git.openjdk.java.net/jdk/pull/2410
Re: RFR: 8195744: Avoid calling ClassLoader.checkPackageAccess if security manager is not installed [v3]
On Fri, 5 Feb 2021 02:25:15 GMT, Coleen Phillimore wrote:
>> 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.
I missed the re-check at line 163 means we are going to return immediately
after we execute the debug code.
-
PR: https://git.openjdk.java.net/jdk/pull/2410
Re: RFR: 8195744: Avoid calling ClassLoader.checkPackageAccess if security manager is not installed [v3]
On Fri, 5 Feb 2021 02:37:54 GMT, Coleen Phillimore 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. > > Coleen Phillimore has updated the pull request incrementally with one > additional commit since the last revision: > > Fix comments and copyright. Some comments remain confusing. Thanks, David src/hotspot/share/classfile/javaClasses.cpp line 4415: > 4413: > 4414: // This field means that a security manager can be installed so we > still have to > 4415: // populate the ProtectionDomainCacheTable. No this field returns the installed SM if any. It doesn't tell you anything about whether you can install a SM or not (though obviously if non-NULL then you could). src/java.base/share/classes/java/lang/System.java line 163: > 161: > 162: // indicates if a security manager is possible > 163: // @implNote The HotSpot JVM hardcodes the value of NEVER. You don't need this if the VM reads the value of NEVER. - PR: https://git.openjdk.java.net/jdk/pull/2410
Re: RFR: 8195744: Avoid calling ClassLoader.checkPackageAccess if security manager is not installed [v3]
> 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. Coleen Phillimore has updated the pull request incrementally with one additional commit since the last revision: Fix comments and copyright. - Changes: - all: https://git.openjdk.java.net/jdk/pull/2410/files - new: https://git.openjdk.java.net/jdk/pull/2410/files/296d0adb..7a2a3617 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2410&range=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2410&range=01-02 Stats: 14 lines in 2 files changed: 3 ins; 3 del; 8 mod Patch: https://git.openjdk.java.net/jdk/pull/2410.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2410/head:pull/2410 PR: https://git.openjdk.java.net/jdk/pull/2410
