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

classic Classic list List threaded Threaded
17 messages Options
Reply | Threaded
Open this post in threaded view
|

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

Coleen Phillimore-3
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.

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

Commit messages:
 - 8195744: Avoid calling ClassLoader.checkPackageAccess if security manager is not installed

Changes: https://git.openjdk.java.net/jdk/pull/2410/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=2410&range=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8195744
  Stats: 144 lines in 8 files changed: 72 ins; 33 del; 39 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
Reply | Threaded
Open this post in threaded view
|

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

Mandy Chung-2
On Thu, 4 Feb 2021 19:12:32 GMT, Coleen Phillimore <[hidden email]> 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.

It's good to see this change benefitting from `-Djava.security.manager=disallow` work.  A minor comment: VM now hardcodes the value for `NEVER` (1).   It may worth adding a comment in `System::NEVER` of such dependency.

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

PR: https://git.openjdk.java.net/jdk/pull/2410
Reply | Threaded
Open this post in threaded view
|

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

Coleen Phillimore-3
On Thu, 4 Feb 2021 22:39:42 GMT, Mandy Chung <[hidden email]> 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.
>
> It's good to see this change benefitting from `-Djava.security.manager=disallow` work.  A minor comment: VM now hardcodes the value for `NEVER` (1).   It may worth adding a comment in `System::NEVER` of such dependency.

Thanks Mandy! It's nice to be able to optimize this.  How's this comment?  Is it an @implNote ??
    // @implNote The HotSpot JVM hardcodes the value of NEVER.

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

PR: https://git.openjdk.java.net/jdk/pull/2410
Reply | Threaded
Open this post in threaded view
|

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

Ioi Lam-2
On Thu, 4 Feb 2021 23:07:34 GMT, Coleen Phillimore <[hidden email]> wrote:

> Thanks Mandy! It's nice to be able to optimize this. How's this comment? Is it an @implNote ??
> // @implNote The HotSpot JVM hardcodes the value of NEVER.

I think it's better to define `const int NEVER = 1`, and add code in debug builds to compare its value with the `System::NEVER` static field.

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

PR: https://git.openjdk.java.net/jdk/pull/2410
Reply | Threaded
Open this post in threaded view
|

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

Coleen Phillimore-3
On Fri, 5 Feb 2021 00:09:51 GMT, Ioi Lam <[hidden email]> wrote:

>> Thanks Mandy! It's nice to be able to optimize this.  How's this comment?  Is it an @implNote ??
>>     // @implNote The HotSpot JVM hardcodes the value of NEVER.
>
>> Thanks Mandy! It's nice to be able to optimize this. How's this comment? Is it an @implNote ??
>> // @implNote The HotSpot JVM hardcodes the value of NEVER.
>
> I think it's better to define `const int NEVER = 1`, and add code in debug builds to compare its value with the `System::NEVER` static field.

I didn't think a static final field would have an offset?
    private static final int NEVER = 1;

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

PR: https://git.openjdk.java.net/jdk/pull/2410
Reply | Threaded
Open this post in threaded view
|

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

David Holmes-2
In reply to this post by Coleen Phillimore-3
On Thu, 4 Feb 2021 19:12:32 GMT, Coleen Phillimore <[hidden email]> 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
Reply | Threaded
Open this post in threaded view
|

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

Coleen Phillimore-3
In reply to this post by Coleen Phillimore-3
> 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:

  Add comment and read NEVER field from System

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

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/2410/files
  - new: https://git.openjdk.java.net/jdk/pull/2410/files/19ddd066..296d0adb

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2410&range=01
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2410&range=00-01

  Stats: 25 lines in 3 files changed: 17 ins; 7 del; 1 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
Reply | Threaded
Open this post in threaded view
|

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

Coleen Phillimore-3
In reply to this post by Coleen Phillimore-3
> 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 logging and comment.

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

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/2410/files
  - new: https://git.openjdk.java.net/jdk/pull/2410/files/7a2a3617..edc3770c

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=2410&range=03
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=2410&range=02-03

  Stats: 41 lines in 4 files changed: 17 ins; 17 del; 7 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
Reply | Threaded
Open this post in threaded view
|

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

Coleen Phillimore-3
In reply to this post by David Holmes-2
On Fri, 5 Feb 2021 03:14:52 GMT, David Holmes <[hidden email]> wrote:

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

Ok, I see what you're suggesting.  Yes, I'll move all the logging after the check. I don't see any reason to have it if we're not going to make the upcall.  Also fixed the test.

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

PR: https://git.openjdk.java.net/jdk/pull/2410
Reply | Threaded
Open this post in threaded view
|

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

Mandy Chung-2
In reply to this post by Coleen Phillimore-3
On Fri, 5 Feb 2021 16:03:45 GMT, Coleen Phillimore <[hidden email]> wrote:

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

This is also good to me.

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

PR: https://git.openjdk.java.net/jdk/pull/2410
Reply | Threaded
Open this post in threaded view
|

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

David Holmes-2
In reply to this post by Coleen Phillimore-3
On Fri, 5 Feb 2021 16:28:01 GMT, Coleen Phillimore <[hidden email]> 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 logging and comment.

Updates seem fine to me.

Thanks,
David

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

Marked as reviewed by dholmes (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/2410
Reply | Threaded
Open this post in threaded view
|

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

Coleen Phillimore-3
On Sun, 7 Feb 2021 12:07:47 GMT, David Holmes <[hidden email]> wrote:

>> Coleen Phillimore has updated the pull request incrementally with one additional commit since the last revision:
>>
>>   Fix logging and comment.
>
> Updates seem fine to me.
>
> Thanks,
> David

Thanks for the code review, David.

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

PR: https://git.openjdk.java.net/jdk/pull/2410
Reply | Threaded
Open this post in threaded view
|

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

Ioi Lam-2
In reply to this post by Coleen Phillimore-3
On Fri, 5 Feb 2021 16:28:01 GMT, Coleen Phillimore <[hidden email]> 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 logging and comment.

Marked as reviewed by iklam (Reviewer).

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

PR: https://git.openjdk.java.net/jdk/pull/2410
Reply | Threaded
Open this post in threaded view
|

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

Coleen Phillimore-3
On Mon, 8 Feb 2021 20:51:31 GMT, Ioi Lam <[hidden email]> wrote:

>> Coleen Phillimore has updated the pull request incrementally with one additional commit since the last revision:
>>
>>   Fix logging and comment.
>
> Marked as reviewed by iklam (Reviewer).

Thanks for the code review Ioi, and thank you Mandy for looking at it.

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

PR: https://git.openjdk.java.net/jdk/pull/2410
Reply | Threaded
Open this post in threaded view
|

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

Coleen Phillimore-3
In reply to this post by Coleen Phillimore-3
On Thu, 4 Feb 2021 19:12:32 GMT, Coleen Phillimore <[hidden email]> 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.

This pull request has now been integrated.

Changeset: ace8f946
Author:    Coleen Phillimore <[hidden email]>
URL:       https://git.openjdk.java.net/jdk/commit/ace8f946
Stats:     175 lines in 8 files changed: 94 ins; 45 del; 36 mod

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

Reviewed-by: dholmes, iklam

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

PR: https://git.openjdk.java.net/jdk/pull/2410
Reply | Threaded
Open this post in threaded view
|

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

Mandy Chung-2
In reply to this post by Coleen Phillimore-3
On Fri, 5 Feb 2021 16:28:01 GMT, Coleen Phillimore <[hidden email]> 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 logging and comment.

Looks good.

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

PR: https://git.openjdk.java.net/jdk/pull/2410
Reply | Threaded
Open this post in threaded view
|

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

Coleen Phillimore-3
On Mon, 8 Feb 2021 22:05:16 GMT, Mandy Chung <[hidden email]> wrote:

>> Coleen Phillimore has updated the pull request incrementally with one additional commit since the last revision:
>>
>>   Fix logging and comment.
>
> Looks good.

Thanks Mandy!

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

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