8343981: Remove usage of security manager from Thread and related classes #22035

AlanBateman · 2024-11-12T10:41:48Z

Removes the SecurityManager usage from Thread + friends.

In Thread, the getContextClassLoader method is no longer caller-sensitive method.

JavaLangAccess.newThreadWithAcc is removed and jdk.internal.access is no longer exported to java.naming. The usage of newThreadWithAcc is removed from com.sun.jndi.ldap.VersionHelper. There will be further work on the java.naming module to remove usage of SM, the change here is specific to the usage of ewThreadWithAcc.

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-8343981: Remove usage of security manager from Thread and related classes (Enhancement - P4)

Reviewers

Roger Riggs (@RogerRiggs - Reviewer)
Yudi Zheng (@mur47x111 - Committer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 22035

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

Using diff file

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

Using Webrev

Link to Webrev Comment

Co-authored-by: Sean Mullan <[email protected]> Co-authored-by: Alan Bateman <[email protected]> Co-authored-by: Weijun Wang <[email protected]> Co-authored-by: Aleksei Efimov <[email protected]> Co-authored-by: Brian Burkhalter <[email protected]> Co-authored-by: Daniel Fuchs <[email protected]> Co-authored-by: Harshitha Onkar <[email protected]> Co-authored-by: Joe Wang <[email protected]> Co-authored-by: Jorn Vernee <[email protected]> Co-authored-by: Justin Lu <[email protected]> Co-authored-by: Kevin Walls <[email protected]>> Co-authored-by: Lance Andersen <[email protected]> Co-authored-by: Naoto Sato <[email protected]> Co-authored-by: Roger Riggs <[email protected]> Co-authored-by: Brent Christian <[email protected]>

…java, not useful without SM

…stance.

setInitialContextFactoryBuilder and setObjectFactoryBuilder methods in javax.naming.spi.NamingManager.

permission cannot be used anymore to control access.

description.

…sion checks of the Class.getNestHost and getNestMembers methods, which no longer apply.

…ested by other tests

…ava, useless without SM

ModuleDescriptor.location().

…attacker to create new instances

…Class<?> clazz, int which)

mlbridge · 2024-11-12T16:46:10Z

Webrevs

03: Full (636ebd19)
02: Full (4f320f92)
01: Full (7bffe787)
00: Full (7bffe787)

openjdk-notifier · 2024-11-12T17:20:36Z

The parent pull request that this pull request depends on has now been integrated and the target branch of this pull request has been updated. This means that changes from the dependent pull request can start to show up as belonging to this pull request, which may be confusing for reviewers. To remedy this situation, simply merge the latest changes from the new target branch into this pull request by running commands similar to these in the local repository for your personal fork:

git checkout JDK-8343981
git fetch https://git.openjdk.org/jdk.git master
git merge FETCH_HEAD
# if there are conflicts, follow the instructions given by git merge
git commit -m "Merge master"
git push

openjdk · 2024-11-12T17:21:42Z

@AlanBateman this pull request can not be integrated into master due to one or more merge conflicts. To resolve these merge conflicts and update this pull request you can run the following commands in the local repository for your personal fork:

git checkout JDK-8343981
git fetch https://git.openjdk.org/jdk.git master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push

openjdk · 2024-11-12T17:21:57Z

⚠️ @AlanBateman This pull request contains merges that bring in commits not present in the target repository. Since this is not a "merge style" pull request, these changes will be squashed when this pull request in integrated. If this is your intention, then please ignore this message. If you want to preserve the commit structure, you must change the title of this pull request to Merge <project>:<branch> where <project> is the name of another project in the OpenJDK organization (for example Merge jdk:master).

dfuch · 2024-11-12T19:06:31Z

The proposed changes look reasonable to me. Good simplification of the code. I had some uncertainty about the JNDI changes but I couldn't find any place in JNDI where a Subject would be extracted from the ACC associated with the created thread so I'd say that looks fine.

AlanBateman · 2024-11-12T19:17:54Z

The proposed changes look reasonable to me. Good simplification of the code. I had some uncertainty about the JNDI changes but I couldn't find any place in JNDI where a Subject would be extracted from the ACC associated with the created thread so I'd say that looks fine.

ACC is dead and Subject is re-implemented on ScopedValue now so I think we are okay. There will be many other places where we will also drop the capture and use of the ACC.

RogerRiggs · 2024-11-12T22:08:42Z

src/java.naming/share/classes/com/sun/jndi/ldap/VersionHelper.java

-        PrivilegedAction<Thread> act =
-                () -> SharedSecrets.getJavaLangAccess().newThreadWithAcc(r, acc);
-        return AccessController.doPrivileged(act);
+        return new Thread(r);


Can the refactoring go back to the callers of createThread() to just create threads themselves.
And remove this trivial method.

Yes, this will happen in the PR that does the cleanup of java.naming module.

RogerRiggs

LGTM

mur47x111 · 2024-11-13T16:25:18Z

~~Please hold until I have the Graal changes ready~~
Good to go

AlanBateman · 2024-11-13T18:48:44Z

/integrate

openjdk · 2024-11-13T18:49:34Z

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

dbf2346: 8341260: Add Float16 to jdk.incubator.vector
a5f11b5: 8343483: Remove unnecessary @SuppressWarnings annotations (serviceability)
7be7772: 8344112: Remove code to support security manager execution mode from DatagramChannel implementation
bd3fec3: 8344086: Remove security manager dependency in FFM
916694f: 8343317: Extend test generation tool to handle APX NDD/NF flavor of instructions
eb240a7: 8344051: Problemlist jdk/jfr/event/runtime/TestNativeMemoryUsageEvents.java with ZGC until JDK-8343893 is fixed
c00e20c: 8343285: java.lang.Process is unresponsive and CPU usage spikes to 100%
cc2acd1: 8343286: Missing unchecked cast warning in polymorphic method call
b80ca49: 8344124: JDK-8341411 Broke the build
a08d67c: 8344080: Return type mismatch for jfr_unregister_stack_filter
... and 12 more: https://git.openjdk.org/jdk/compare/63eb4853f6782f350f67b6bcf25d83bc4480be71...master

Your commit was automatically rebased without conflicts.

openjdk · 2024-11-13T18:49:47Z

@AlanBateman Pushed as commit 5e01c40.

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

AlanBateman and others added 30 commits September 30, 2024 18:02

ServerSocket bind/accept no longer throw SecurityException

591d3d6

Merge

781eecd

hack out Security Manager from LoaderHandler

4035376

Update spec for RMIClassLoader

6d2006d

Fix whitespace.

6c325c2

Restore URL parsing to LoaderHandler::getClassLoader.

463d46b

Merge

439ed26

Remove vmTestbase/nsk/jvmti/AttachOnDemand/attach004/TestDescription.…

53e06d0

…java, not useful without SM

Remove leftover SecurityException text from RMIConnection.getObjectIn…

9e1cec8

…stance.

Remove leftover "@see java.lang.SecurityManager#checkSetFactory" from

7cf5824

setInitialContextFactoryBuilder and setObjectFactoryBuilder methods in javax.naming.spi.NamingManager.

Remove unnecessary comma.

77b58bf

Merge

b5f2ca3

Fix typo.

938737a

Remove text about granting permission and add api note indicating

2612400

permission cannot be used anymore to control access.

Remove text about implementers using Security Manager checks from class

4688f78

description.

Remove Security Manager text from EXIT_ON_CLOSE.

34da8c7

Removed TestSecurityManagerChecks.java - this test was testing permis…

c62c4ad

…sion checks of the Class.getNestHost and getNestMembers methods, which no longer apply.

Drop reference to permissions from Font.getFont

d8e15dd

Merge

29c0bb2

Update record tests to not require SM

23e0f09

Remove tests for repeating annotations with SM set, no SM case well t…

bd27286

…ested by other tests

Change reflect/Nestmates/TestReflectionAPI to not test with a SM set

7f451ea

Remove test/langtools/tools/javac/api/ToolProvider/ToolProviderTest.j…

c9ab253

…ava, useless without SM

Remove text about granting permissions to module from

36522ed

ModuleDescriptor.location().

Remove text about Proxy objects being granted AllPermission.

c769f2d

Remove text about needing ReflectPermission to access AccessibleObject.

79b4b21

JDK-8003881: Prevent lambda implementing inner classes from allowing …

e46d99c

…attacker to create new instances

JDK-8007035: deprecate public void SecurityManager.checkMemberAccess(…

422c489

…Class<?> clazz, int which)

Update copyright header

1f4be6f

openjdk bot added security [email protected] core-libs [email protected] labels Nov 12, 2024

AlanBateman marked this pull request as ready for review November 12, 2024 16:42

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

openjdk-notifier bot changed the base branch from pr/21498 to master November 12, 2024 17:20

openjdk bot added the merge-conflict Pull request has merge conflict with target branch label Nov 12, 2024

Merge commit 'db85090553ab14a84c3ed0a2604dd56c5b6e6982' into JDK-8343981

4f320f9

openjdk bot removed the merge-conflict Pull request has merge conflict with target branch label Nov 12, 2024

Merge branch 'master' into JDK-8343981

636ebd1

RogerRiggs reviewed Nov 12, 2024

View reviewed changes

RogerRiggs 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

graalvmbot mentioned this pull request Nov 13, 2024

[JDK-8344133] Adapt JDK-8343981: Remove usage of security manager from Thread and related classes oracle/graal#10087

Merged

mur47x111 approved these changes Nov 13, 2024

View reviewed changes

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

openjdk bot closed this Nov 13, 2024

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

eirbjo mentioned this pull request Nov 19, 2024

8344534: Remove leftover import of java.security.AccessControlContext in JavaLangAccess #22234

Closed

3 tasks

graalvmbot mentioned this pull request Nov 19, 2024

[GR-59836] Update labsjdk to 24+24-jvmci-b01 oracle/graal#10108

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8343981: Remove usage of security manager from Thread and related classes #22035

8343981: Remove usage of security manager from Thread and related classes #22035

AlanBateman commented Nov 12, 2024 •

edited by openjdk bot

Loading

mlbridge bot commented Nov 12, 2024 •

edited

Loading

openjdk-notifier bot commented Nov 12, 2024

openjdk bot commented Nov 12, 2024

openjdk bot commented Nov 12, 2024

dfuch commented Nov 12, 2024

AlanBateman commented Nov 12, 2024

RogerRiggs Nov 12, 2024

AlanBateman Nov 13, 2024 •

edited

Loading

RogerRiggs left a comment

mur47x111 commented Nov 13, 2024 •

edited

Loading

AlanBateman commented Nov 13, 2024

openjdk bot commented Nov 13, 2024

openjdk bot commented Nov 13, 2024

8343981: Remove usage of security manager from Thread and related classes #22035

8343981: Remove usage of security manager from Thread and related classes #22035

Conversation

AlanBateman commented Nov 12, 2024 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

mlbridge bot commented Nov 12, 2024 • edited Loading

Webrevs

openjdk-notifier bot commented Nov 12, 2024

openjdk bot commented Nov 12, 2024

openjdk bot commented Nov 12, 2024

dfuch commented Nov 12, 2024

AlanBateman commented Nov 12, 2024

RogerRiggs Nov 12, 2024

Choose a reason for hiding this comment

AlanBateman Nov 13, 2024 • edited Loading

Choose a reason for hiding this comment

RogerRiggs left a comment

Choose a reason for hiding this comment

mur47x111 commented Nov 13, 2024 • edited Loading

AlanBateman commented Nov 13, 2024

openjdk bot commented Nov 13, 2024

openjdk bot commented Nov 13, 2024

AlanBateman commented Nov 12, 2024 •

edited by openjdk bot

Loading

mlbridge bot commented Nov 12, 2024 •

edited

Loading

AlanBateman Nov 13, 2024 •

edited

Loading

mur47x111 commented Nov 13, 2024 •

edited

Loading