On Fri, 5 Feb 2021 01:45:58 GMT, David Holmes <dhol...@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
