8344011: Remove usage of security manager from Class and reflective APIs #22063

AlanBateman · 2024-11-13T10:32:34Z

Remove code required for the now defunct SecurityManager execution mode from java.lang.Class, friends, and reflection APIs. Careful review is required so I've set Reviewer to 2. I've tried to keep the changes as easy to review as possible and not go over board with cleanup.

sun.reflect.misc.ReflectUtil are been hollowed out. A future pass will remove empty methods and qualified exports once the changes in "far away" code and modules is done.

In Lookup's class description, the removal of the sentence "avoid package access checks for classes accessible to the lookup class" and the link to the removed "Security manager interactions" section is in discussion/non-normative text, just missed in the JEP 486 update that remove the linked section.

runtime/cds/appcds/StaticArchiveWithLambda.java is updated as creating the archive no longer skips a generated class.

Testing: tier1-5

Progress

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

Issue

JDK-8344011: Remove usage of security manager from Class and reflective APIs (Enhancement - P4)

Reviewers

Roger Riggs (@RogerRiggs - Reviewer) Review applies to b46c4fea
Chen Liang (@liach - Reviewer)
Yudi Zheng (@mur47x111 - Committer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 22063

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

Using diff file

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

Using Webrev

Link to Webrev Comment

bridgekeeper · 2024-11-13T10:33:14Z

👋 Welcome back alanb! 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-13T10:33:27Z

@AlanBateman 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:

8344011: Remove usage of security manager from Class and reflective APIs

Reviewed-by: liach, yzheng, rriggs

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 14 new commits pushed to the master branch:

c977ef7: 8342047: Create Template Assertion Predicates with Halt nodes only instead of uncommon traps
23a8c71: 8341790: Fix ExceptionOccurred in java.desktop
1e97c1c: 8335989: Implement JEP 494: Module Import Declarations (Second Preview)
e7d90b9: 8343460: ZGC: Crash in ZRemembered::scan_page_and_clear_remset
95a00f8: 8343875: Minor improvements of jpackage test library
90e9234: 8344074: RISC-V: C1: More accurate _exception_handler_size and _deopt_handler_size
3b28354: 8339288: Improve diagnostic logging runtime/cds/DeterministicDump.java
0dab920: 8343984: Fix Unsafe address overflow
168b18e: 8343958: Remove security manager impl in java.lang.Process and java.lang.Runtime.exec
5ac330b: 8344039: Remove security manager dependency in java.time
... and 4 more: https://git.openjdk.org/jdk/compare/dbf23466aff902836838f06bcbbf3c9e7c5e9c6a...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-13T10:34:05Z

@AlanBateman The following label will be automatically applied to this pull request:

core-libs

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

AlanBateman · 2024-11-13T12:05:10Z

/reviewers 2

openjdk · 2024-11-13T12:06:12Z

@AlanBateman
The total number of required reviews for this PR (including the jcheck configuration and the last /reviewers command) is now set to 2 (with at least 1 Reviewer, 1 Author).

mlbridge · 2024-11-13T12:19:58Z

Webrevs

01: Full - Incremental (1687ad54)
00: Full (b46c4fea)

RogerRiggs

lgtm

liach

Reflection and invoke changes look good. Not sure about ServiceLoader. Module/Package changes look innocuous but not a professional in those areas either.

…hen creating archive

AlanBateman · 2024-11-13T19:01:18Z

runtime/cds/appcds/StaticArchiveWithLambda.java is updated to not assume that the archive create skips a generated class (confirmed with Ioi).

liach

The update to StaticArchiveWithLambda test makes sense.

seanjmullan

Reviewed most of it, so far only couple of minor comments, can try to finish up tomorrow.

seanjmullan · 2024-11-13T22:25:17Z

src/java.base/share/classes/jdk/internal/reflect/ReflectionFactory.java

Lines 55-57, can the "access to the constructor" part be removed from the comment?

src/java.base/share/classes/jdk/internal/reflect/ReflectionFactory.java

mur47x111

LGTM. Graal changes are ready

AlanBateman · 2024-11-14T07:38:40Z

/integrate

openjdk · 2024-11-14T07:39:29Z

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

c977ef7: 8342047: Create Template Assertion Predicates with Halt nodes only instead of uncommon traps
23a8c71: 8341790: Fix ExceptionOccurred in java.desktop
1e97c1c: 8335989: Implement JEP 494: Module Import Declarations (Second Preview)
e7d90b9: 8343460: ZGC: Crash in ZRemembered::scan_page_and_clear_remset
95a00f8: 8343875: Minor improvements of jpackage test library
90e9234: 8344074: RISC-V: C1: More accurate _exception_handler_size and _deopt_handler_size
3b28354: 8339288: Improve diagnostic logging runtime/cds/DeterministicDump.java
0dab920: 8343984: Fix Unsafe address overflow
168b18e: 8343958: Remove security manager impl in java.lang.Process and java.lang.Runtime.exec
5ac330b: 8344039: Remove security manager dependency in java.time
... and 4 more: https://git.openjdk.org/jdk/compare/dbf23466aff902836838f06bcbbf3c9e7c5e9c6a...master

Your commit was automatically rebased without conflicts.

openjdk · 2024-11-14T07:39:39Z

@AlanBateman Pushed as commit abacece.

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

mlbridge · 2025-02-21T01:10:46Z

Mailing list message from James Perkins on core-libs-dev:

Please forgive me if this is the wrong medium to report this.

I've found an issue with the change in the ServiceLoader. Specifically, the
getConstructor()[1] method. Previously, the method caught Throwable and
then fail which would throw a java.util.ServiceConfigurationError
exception. With the changes, only a catches a NoSuchMethodException and
throws a ServiceConfigurationError then. Is this change in behavior
expected?

For some background on how I found this. I had a legitimate classpath issue
missing a CDI dependency. However, the constructor was throwing a
NoClassDefFoundException because of the missing required class. In versions
less than Java 24 this was okay because Throwable was caught. In Java 24
and 25 early access, this is an issue because the NoClassDefFoundException
is thrown instead of it being wrapped in a ServiceConfigurationError.

[1]:
https://github.com/openjdk/jdk/commit/abacece8265996aaec888c8f109f2e476ec7a8e3#diff-32ee5f2b2ad157956f95f404ef7001d8a1ca597ff07f4eb88181309a606af199L647-L668

James R. Perkins

Principal Software Engineer

Red Hat <https://www.redhat.com/>
<https://www.redhat.com/>

On Wed, Nov 13, 2024 at 11:44?PM Alan Bateman <alanb at openjdk.org> wrote:

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/core-libs-dev/attachments/20250220/951e6c6e/attachment-0001.htm>

mlbridge · 2025-02-21T06:45:46Z

Mailing list message from Alan Bateman on core-libs-dev:

On 21/02/2025 01:08, James Perkins wrote:

Please forgive me if this is the wrong medium to report this.

I've found an issue with the change in the ServiceLoader.
Specifically, the getConstructor()[1] method. Previously, the method
caught Throwable and then fail which would throw a
java.util.ServiceConfigurationError exception. With the changes, only
a catches a?NoSuchMethodException and throws a
ServiceConfigurationError then. Is this change in behavior expected?

For some background on how I found this. I had a legitimate?classpath
issue missing a CDI dependency. However, the constructor was throwing
a NoClassDefFoundException because of the missing required class. In
versions less than Java 24 this was okay because Throwable was caught.
In Java 24 and 25 early access, this is an issue because the
NoClassDefFoundException is thrown instead of it being wrapped in a
ServiceConfigurationError.

Thanks for the bug report. Yes, this is a change in behavior that was
not intended, and a reminder that there aren't enough tests for NCDFE
and other linkage errors. Note that there are other cases, that date
back to JDK 6, where linkage errors aren't wrapped so is other work to
do in this area. I've created JDK-8350481 to track the behavior change.

-Alan

[1] https://bugs.openjdk.org/browse/JDK-8350481

mlbridge · 2025-02-21T22:58:46Z

Mailing list message from James Perkins on core-libs-dev:

Perfect. Thank you so much Alan.

James R. Perkins

Principal Software Engineer

Red Hat <https://www.redhat.com/>
<https://www.redhat.com/>

On Thu, Feb 20, 2025 at 10:44?PM Alan Bateman <alan.bateman at oracle.com>
wrote:

On 21/02/2025 01:08, James Perkins wrote:

Please forgive me if this is the wrong medium to report this.

I've found an issue with the change in the ServiceLoader.
Specifically, the getConstructor()[1] method. Previously, the method
caught Throwable and then fail which would throw a
java.util.ServiceConfigurationError exception. With the changes, only
a catches a NoSuchMethodException and throws a
ServiceConfigurationError then. Is this change in behavior expected?

For some background on how I found this. I had a legitimate classpath
issue missing a CDI dependency. However, the constructor was throwing
a NoClassDefFoundException because of the missing required class. In
versions less than Java 24 this was okay because Throwable was caught.
In Java 24 and 25 early access, this is an issue because the
NoClassDefFoundException is thrown instead of it being wrapped in a
ServiceConfigurationError.

Thanks for the bug report. Yes, this is a change in behavior that was
not intended, and a reminder that there aren't enough tests for NCDFE
and other linkage errors. Note that there are other cases, that date
back to JDK 6, where linkage errors aren't wrapped so is other work to
do in this area. I've created JDK-8350481 to track the behavior change.

-Alan

[1] https://bugs.openjdk.org/browse/JDK-8350481

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/core-libs-dev/attachments/20250221/12e4fa77/attachment.htm>

Initial commit

b46c4fe

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

AlanBateman marked this pull request as ready for review November 13, 2024 12:15

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

graalvmbot mentioned this pull request Nov 13, 2024

[JDK-8344132] Adapt JDK-8344011: Remove usage of security manager from Class and reflective APIs oracle/graal#10086

Merged

RogerRiggs approved these changes Nov 13, 2024

View reviewed changes

liach 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

AlanBateman added 2 commits November 13, 2024 18:48

Merge branch 'master' into JDK-8344011

9e2ba1d

StaticArchiveWithLambda no longer skips dynamically generated class w…

1687ad5

…hen creating archive

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

liach approved these changes Nov 13, 2024

View reviewed changes

seanjmullan reviewed Nov 13, 2024

View reviewed changes

mur47x111 approved these changes Nov 14, 2024

View reviewed changes

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

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

openjdk bot closed this Nov 14, 2024

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

graalvmbot mentioned this pull request Nov 27, 2024

[GR-59976] Update labsjdk to 24+25-jvmci-b01 oracle/graal#10171

Merged

8344011: Remove usage of security manager from Class and reflective APIs #22063

8344011: Remove usage of security manager from Class and reflective APIs #22063

Uh oh!

Conversation

AlanBateman 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!

AlanBateman commented Nov 13, 2024

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!

RogerRiggs left a comment

Choose a reason for hiding this comment

Uh oh!

liach left a comment

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

liach left a comment

Choose a reason for hiding this comment

Uh oh!

seanjmullan left a comment

Choose a reason for hiding this comment

Uh oh!

seanjmullan Nov 13, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mur47x111 left a comment

Choose a reason for hiding this comment

Uh oh!

AlanBateman commented Nov 14, 2024

Uh oh!

openjdk bot commented Nov 14, 2024

Uh oh!

openjdk bot commented Nov 14, 2024

Uh oh!

mlbridge bot commented Feb 21, 2025

Uh oh!

mlbridge bot commented Feb 21, 2025

Uh oh!

mlbridge bot commented Feb 21, 2025

Uh oh!

Uh oh!

AlanBateman 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

AlanBateman commented Nov 13, 2024 •

edited

Loading