8341916: Remove ProtectionDomain related hotspot code and tests #22064

coleenp · 2024-11-13T11:42:11Z

Remove Hotspot code that passes protection_domain around class loading so that checkPackageAccess can be called and the result stored. With the removal of the Security Manager in JEP 486, this code no longer does anything.

Tested with tier1-4.

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8341916: Remove ProtectionDomain related hotspot code and tests (Enhancement - P3)

Reviewers

Ioi Lam (@iklam - Reviewer) Review applies to 14e11e59
David Holmes (@dholmes-ora - Reviewer)
John R Rose (@rose00 - Reviewer) Review applies to 14e11e59

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/22064/head:pull/22064
$ git checkout pull/22064

Update a local copy of the PR:
$ git checkout pull/22064
$ git pull https://git.openjdk.org/jdk.git pull/22064/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 22064

View PR using the GUI difftool:
$ git pr show -t 22064

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/22064.diff

Using Webrev

Link to Webrev Comment

bridgekeeper · 2024-11-13T11:43:10Z

👋 Welcome back coleenp! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2024-11-13T11:43:26Z

@coleenp This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8341916: Remove ProtectionDomain related hotspot code and tests

Reviewed-by: dholmes, iklam, jrose

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 11 new commits pushed to the master branch:

a47d9ba: 8344349: Problemlist jdk/jfr/jvm/TestVirtualThreadExclusion.java before JDK-8344199 resolved
80e37a9: 8344265: RISC-V: Remove unused function get_previous_sp_entry
e1c4b49: 8343237: Improve the copying of the available set of Currencies
41a627b: 8343876: Enhancements to jpackage test lib
aa10ec7: 8343123: Nimbus: javax/swing/JInternalFrame/bug6726866.java does not have green undecorated window
fec0d1c: 8343777: Add since checker tests to Internationalisation modules
d0b770c: 8344289: SM cleanup in jdk.internal.util
a91d4c0: 8344233: Remove calls to SecurityManager and doPrivileged in java.net.ProxySelector and sun.net.spi.DefaultProxySelector after JEP 486 integration
d2e4b51: 8344186: Cleanup sun.net.www.MimeTable after JEP 486 integration
da40388: 8344315: Clean up sun.net.www.protocol.jrt.JavaRuntimeURLConnection after JEP 486 integration
... and 1 more: https://git.openjdk.org/jdk/compare/41a2d49f0a1ed298b8ab023ce634335464454fe7...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

openjdk · 2024-11-13T11:43:59Z

@coleenp The following labels will be automatically applied to this pull request:

graal
hotspot
serviceability

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

mlbridge · 2024-11-13T11:47:53Z

Webrevs

seanjmullan · 2024-11-13T12:34:57Z

/label security

openjdk · 2024-11-13T12:35:23Z

@seanjmullan
The security label was successfully added.

iklam

LGTM. Good to see all this code deleted.

dholmes-ora · 2024-11-14T06:16:56Z

@coleenp it is great to see all this code go but I'm unclear about the uses of "protection domain" that have been removed, compared to those that still remain in the hotspot code in particular how CDS still uses it. To be fair I'm unclear what role PD still plays on the JDK side and would not be surprised if it is destined for removal at some point. How do we recognise that the remaining uses of and reference to the PD are still needed and not something we could now delete?

AlanBateman · 2024-11-14T07:01:54Z

To be fair I'm unclear what role PD still plays on the JDK side and would not be surprised if it is destined for removal at some point.

PD is not deprecated as PD::getCodeSource is widely used. It may be that an alternative means is introduced in the future to expose the code location but nothing specific at this time.

dholmes-ora

This is a great cleanup!

I may have missed something, but it seems to me that java_security_AccessControlContext is all dead code now too.

Thanks

dholmes-ora · 2024-11-14T05:42:51Z

src/hotspot/share/ci/ciEnv.cpp

  // The very first entry is the InstanceKlass of the root method of the current compilation in order to get the right
-  // protection domain to load subsequent classes during replay compilation.
+  // (class loader???) protection domain to load subsequent classes during replay compilation.


Suggestion: simply have:

// The very first entry is the InstanceKlass of the root method of the current compilation .

The rest of the comment doesn't really make sense even before your change as this method basically just prints the class name

Thanks for noticing this. Updated comment that didn't make sense to me either.

src/hotspot/share/classfile/dictionary.cpp

src/hotspot/share/classfile/javaClasses.hpp

src/hotspot/share/classfile/systemDictionary.hpp

src/hotspot/share/logging/logTag.hpp

dholmes-ora · 2024-11-14T07:53:32Z

To be fair I'm unclear what role PD still plays on the JDK side and would not be surprised if it is destined for removal at some point.

PD is not deprecated as PD::getCodeSource is widely used. It may be that an alternative means is introduced in the future to expose the code location but nothing specific at this time.

Okay but I still remain unclear about the role of PD in the VM, in particular how CDS is using it.

AlanBateman · 2024-11-14T08:28:14Z

@coleenp Do you plan a follow-up to purge the remaining refs to AccessController and AccessControlContext?

coleenp

Thanks for looking through these changes. Thanks @AlanBateman for answering the question about remaining uses of protection domain. When we create an instance of java.lang.Class, the VM stores the protection domain given in resolve_from_stream. I may have already said this somewhere. So we need to pass it through that path.

coleenp · 2024-11-14T12:17:11Z

src/hotspot/share/ci/ciEnv.cpp

  // The very first entry is the InstanceKlass of the root method of the current compilation in order to get the right
-  // protection domain to load subsequent classes during replay compilation.
+  // (class loader???) protection domain to load subsequent classes during replay compilation.


Thanks for noticing this. Updated comment that didn't make sense to me either.

src/hotspot/share/classfile/javaClasses.hpp

src/hotspot/share/classfile/systemDictionary.hpp

src/hotspot/share/logging/logTag.hpp

coleenp · 2024-11-14T12:24:23Z

Do you plan a follow-up to purge the remaining refs to AccessController and AccessControlContext?

I was unclear if they were still needed in the places they appear. Maybe I should do a follow-up.

Edit: I'll clean them up with this change.

…walking questions.

coleenp · 2024-11-14T13:06:25Z

@AlanBateman there was that AccessControlContext in the stack that I asked about in the main review that I can't find the answer to. Is it obsolete now? See the change for where I asked this question. Thank you!

coleenp · 2024-11-14T13:07:55Z

hotspot/share/include/jvm.h:JVM_GetClassContext(JNIEnv *env);

I think this is obsolete too.

AlanBateman · 2024-11-14T13:20:23Z

hotspot/share/include/jvm.h:JVM_GetClassContext(JNIEnv *env);

I think this is obsolete too.

As part of the JEP 486 work, I changed SecurityManager::getClassContext to use StackWalker, the native method that called into JVM_GetClassContext is removed. So more cleanup here.

AlanBateman · 2024-11-14T13:24:14Z

@AlanBateman there was that AccessControlContext in the stack that I asked about in the main review that I can't find the answer to. Is it obsolete now? See the change for where I asked this question. Thank you!

The stack walk that stopped when it found a privileged frame is removed. I can't think of any scenario now where the VM will be interested in the AccessControlContext. Also AccessController is re-implemented to just invoke the actions so there should be no reason for the VM to know about AccessController either.

Note that we need to keep JVM_EnsureMaterializedForStackWalk as that is needed for ScopedValue when recovering from SOE.

…on - shouldn't take TRAPS).

coleenp · 2024-11-14T14:00:14Z

The stack walk ignoring AccessControlContext was in some logging code, so now removed. Also, I saw that getClassContext was rewritten, so removed that bit too.

AlanBateman · 2024-11-14T14:42:30Z

I see a few left over refs to SecurityManager in vmSymbols.hpp, vmClassMacros.hpp, and a comment in logDiagnosticCommand.hpp.

coleenp · 2024-11-14T16:57:01Z

Thanks @AlanBateman There's a DCmd permissions() function that talks about DiagnosticCommandMBean security. I don't know what that is so I'm leaving it. Edit: appears unrelated.

iklam · 2024-11-14T18:03:36Z

To be fair I'm unclear what role PD still plays on the JDK side and would not be surprised if it is destined for removal at some point.

PD is not deprecated as PD::getCodeSource is widely used. It may be that an alternative means is introduced in the future to expose the code location but nothing specific at this time.

Okay but I still remain unclear about the role of PD in the VM, in particular how CDS is using it.

CDS just emulates what the Java code does -- to ensure that Class.getProtectionDomain() would get the same answer as if the class was loaded from bytecodes.

dholmes-ora · 2024-11-15T02:37:56Z

src/hotspot/share/classfile/javaClasses.cpp

  macro(_holder_offset,        k, "holder", thread_fieldholder_signature, false); \
  macro(_name_offset,          k, vmSymbols::name_name(), string_signature, false); \
-  macro(_contextClassLoader_offset, k, vmSymbols::contextClassLoader_name(), classloader_signature, false); \
+  macro(_contextClassLoader_offset, k, "contextClassLoader", classloader_signature, false); \


I didn't think the context class loader was related to SM in any way. ??

It isn't. This symbol was near the ones I deleted, and I deleted it by mistake, so I moved it here.

dholmes-ora · 2024-11-15T02:46:37Z

src/hotspot/share/logging/logDiagnosticCommand.hpp


-  // Used by SecurityManager. This DCMD requires ManagementPermission = control.
  static const JavaPermission permission() {


Is any of this permission stuff still relevant? I couldn't figure out what ultimately looks at them. ??

I don't know that. It is passed by the MBean code. It might be another (different) opportunity for a cleanup if the MBean code doesn't use it anymore.

dholmes-ora · 2024-11-15T02:48:01Z

src/hotspot/share/prims/jvm.cpp

 */

-static void trace_class_resolution_impl(Klass* to_class, TRAPS) {
+extern void trace_class_resolution(Klass* to_class) {


why extern ?

jni.cpp functions call this.

I don't see any difference in the callers in relation to this PR and the function is not presently declared extern. ??

There was an extern trace_class_resolution() function that called this _impl function that I removed, so renamed this impl function to trace_class_resolution().
It's declared extern in jvm.hp file, and this 'extern' qualifier is added so it's easy to see that this is used externally.

Sorry but not seeing that. It is declared in jvm_misc.hpp but not as extern. The original version was not extern.

You're right, it has extern linkage but not declared with extern.

AlanBateman · 2024-11-15T13:51:19Z

Thanks @AlanBateman There's a DCmd permissions() function that talks about DiagnosticCommandMBean security. I don't know what that is so I'm leaving it. Edit: appears unrelated.

Right, no need to change anything there. MBeanServer's spec was changed by JEP 486 to still allow a security exception when access is not authorized. DiagnosticCommandMBean still supports permissions. Kevin Walls is doing a clean-up pass over the java.management and jdk.management to remove vestiges of the security manager but I don't know if he plans to check the VM code.

iklam

LGTM

src/hotspot/share/classfile/systemDictionary.hpp

src/hotspot/share/prims/jvm.cpp

rose00

Except for a couple of suggested tweaks to comments, it all looks correct.

Thanks!

coleenp · 2024-11-18T12:47:52Z

Thanks for the reviews, Ioi, John and David.
/integrate

coleenp · 2024-11-18T12:48:22Z

Thanks also for the comments and more code deletion suggestions, Alan.

openjdk · 2024-11-18T12:48:40Z

Going to push as commit dfddbca.
Since your change was applied there have been 23 commits pushed to the master branch:

5eb0733: 8344383: Include ZipArchive and JarArchive directly
b8b70c8: 8344379: [s390x] build failure due to missing change from JDK-8339466
5fc4322: 8288298: Resolve multiline message parsing ambiguities in UL
ea8f289: 8344271: Comparison build fails due to difference in doc summary
b9c6ce9: 8344122: IGV: Extend c2 IdealGraphPrinter to send subgraphs to IGV
00ff6a3: 8344105: Remove SecurityManager and related calls from jdk.attach and jdk.hotspot.agent
475feb0: 8344056: Use markdown format for man pages
6c2ae44: 8344204: IGV: Button to enable/disable cutting of long edges
4a7ce1d: 8344205: [PPC]: failing assertion: sharedRuntime_ppc.cpp:1652: cookie not found
b6c2122: 8316151: [macos14] ActionListenerCalledTwiceTest.java fails on macOS 14
... and 13 more: https://git.openjdk.org/jdk/compare/41a2d49f0a1ed298b8ab023ce634335464454fe7...master

Your commit was automatically rebased without conflicts.

openjdk · 2024-11-18T12:48:48Z

@coleenp Pushed as commit dfddbca.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

coleenp added 2 commits November 12, 2024 18:34

8341916: Remove ProtectionDomain related hotspot code and tests

e42f69d

Remove some more includes.

c7b5fd1

openjdk bot added the rfr Pull request is ready for review label Nov 13, 2024

openjdk bot added graal graal-dev@openjdk.org serviceability serviceability-dev@openjdk.org hotspot hotspot-dev@openjdk.org labels Nov 13, 2024

openjdk bot added the security security-dev@openjdk.org label Nov 13, 2024

iklam approved these changes Nov 13, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Nov 13, 2024

dholmes-ora reviewed Nov 14, 2024

View reviewed changes

coleenp commented Nov 14, 2024

View reviewed changes

coleenp added 2 commits November 14, 2024 12:26

David comments.

ebf5ed7

More purging of AccessController, AccessControlContext and some stack…

79831e0

…walking questions.

openjdk bot removed the ready Pull request is ready to be integrated label Nov 14, 2024

Found more obsolete security manager code.

11337a0

More obsolete code. Fix trace_class_resolution (doesn't throw excepti…

ca34fc5

…on - shouldn't take TRAPS).

Purge last references to SecurityManager.

aee8efd

dholmes-ora reviewed Nov 15, 2024

View reviewed changes

coleenp added 2 commits November 15, 2024 23:04

Merge branch 'master' into protection-domain

05d0251

Handle merge conflicts in new resolve_instance_class calls.

14e11e5

iklam approved these changes Nov 15, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Nov 15, 2024

rose00 suggested changes Nov 16, 2024

View reviewed changes

src/hotspot/share/classfile/systemDictionary.hpp Outdated Show resolved Hide resolved

src/hotspot/share/classfile/systemDictionary.hpp Outdated Show resolved Hide resolved

src/hotspot/share/prims/jvm.cpp Show resolved Hide resolved

rose00 approved these changes Nov 16, 2024

View reviewed changes

Fix comments.

dd1766e

openjdk bot removed the ready Pull request is ready to be integrated label Nov 16, 2024

dholmes-ora approved these changes Nov 18, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Nov 18, 2024

openjdk bot added the integrated Pull request has been integrated label Nov 18, 2024

openjdk bot closed this Nov 18, 2024

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Nov 18, 2024

coleenp deleted the protection-domain branch November 18, 2024 12:49


		// Used by SecurityManager. This DCMD requires ManagementPermission = control.
		static const JavaPermission permission() {

8341916: Remove ProtectionDomain related hotspot code and tests #22064

8341916: Remove ProtectionDomain related hotspot code and tests #22064

Uh oh!

Conversation

coleenp commented Nov 13, 2024 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Uh oh!

bridgekeeper bot commented Nov 13, 2024

Uh oh!

openjdk bot commented Nov 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openjdk bot commented Nov 13, 2024

Uh oh!

mlbridge bot commented Nov 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

seanjmullan commented Nov 13, 2024

Uh oh!

openjdk bot commented Nov 13, 2024

Uh oh!

iklam left a comment

Choose a reason for hiding this comment

Uh oh!

dholmes-ora commented Nov 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AlanBateman commented Nov 14, 2024

Uh oh!

dholmes-ora left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dholmes-ora commented Nov 14, 2024

Uh oh!

AlanBateman commented Nov 14, 2024

Uh oh!

coleenp left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

coleenp commented Nov 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coleenp commented Nov 14, 2024

Uh oh!

coleenp commented Nov 14, 2024

Uh oh!

AlanBateman commented Nov 14, 2024

Uh oh!

AlanBateman commented Nov 14, 2024

Uh oh!

coleenp commented Nov 14, 2024

Uh oh!

AlanBateman commented Nov 14, 2024

Uh oh!

coleenp commented Nov 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

iklam commented Nov 14, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

coleenp commented Nov 13, 2024 •

edited by openjdk bot

Loading

openjdk bot commented Nov 13, 2024 •

edited

Loading

mlbridge bot commented Nov 13, 2024 •

edited

Loading

dholmes-ora commented Nov 14, 2024 •

edited

Loading

coleenp commented Nov 14, 2024 •

edited

Loading

coleenp commented Nov 14, 2024 •

edited

Loading

coleenp Nov 18, 2024 •

edited

Loading

dholmes-ora Nov 19, 2024 •

edited

Loading