8344289: SM cleanup in jdk.internal.util #22141

eirbjo · 2024-11-15T09:35:17Z

Please review this PR to clean up RandomSupport, ClassFileDumper and StaticProperty in the jdk.internal.util namespace:

RandomSupport is updated to replace an AccessController::doPrivileged call with Boolean::getBoolean. (Existing code uses String::equalsIgnoreCase, equivalent to Boolean::getBoolean)
ClassFileDumper constructor is updated to remove a comment referencing GetPropertyAction. (I left the VM::getSavedProperty call as-is, please advise if this should be replaced with `System::getProperty)
ClassFileDumper::write is updated to unwrap a AccessController::doPrivileged call
StaticProperty is updated to remove SecurityManager::checkPropertyAccess references in the documentation

Verification: GHA results pending.

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-8344289: SM cleanup in jdk.internal.util (Enhancement - P4)

Reviewers

Chen Liang (@liach - Reviewer)
Roger Riggs (@RogerRiggs - Reviewer)
Brian Burkhalter (@bplb - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 22141

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

Using diff file

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

Using Webrev

Link to Webrev Comment

…JEP486 integration

bridgekeeper · 2024-11-15T09:36:24Z

👋 Welcome back eirbjo! 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-15T09:37:05Z

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

8344289: SM cleanup in jdk.internal.util

Reviewed-by: liach, rriggs, bpb

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

ff12ff5: 8340453: C2: Improve encoding of LoadNKlass for compact headers
1866c0c: 8344259: Annotate Float16 with jdk.internal.ValueBased
0b9b82a: 8343039: Remove jdk.internal.misc.InternalLock and usages from java.io
3c38ed4: 8344314: Revert removal of jdk.internal.java.PreviewFeature.CLASSFILE_API
40a055e: 8344228: Revisit SecurityManager usage in java.net.http after JEP 486 integration
84ffb64: 8334714: Implement JEP 484: Class-File API
6cdebf0: 8343599: Kmem limit and max values swapped when printing container information
a672138: 8344161: Argument type mismatch for jfr_type_id
3245f56: 8344164: [s390x] ProblemList hotspot/jtreg/runtime/NMT/VirtualAllocCommitMerge.java
5e27608: 8344188: Cleanup sun.net.www.protocol.jar.JarFileFactory after JEP 486 integration
... and 35 more: https://git.openjdk.org/jdk/compare/c3776db498193d45088ed19e2a1d2697281fd590...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-15T09:37:44Z

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

mlbridge · 2024-11-15T09:46:59Z

Webrevs

01: Full - Incremental (64a9ec15)
00: Full (09c7c2e3)

liach

A few remarks; look fine otherwise. Pinging @rgiulietti to review the random generator changes.

liach · 2024-11-15T15:01:13Z

src/java.base/share/classes/jdk/internal/util/ClassFileDumper.java

@@ -134,48 +128,39 @@ public void dumpFailedClass(String name, byte[] bytes) {

    @SuppressWarnings("removal")


Please remove this redundant suppression.

Thanks, both fixed. IntelliJ IDEA keeps hiding these annotations, making them hard to spot.

liach · 2024-11-15T15:01:46Z

src/java.base/share/classes/jdk/internal/util/ClassFileDumper.java

+                                path + " " + ex.getMessage());
+            }
+            // simply don't care if this operation failed
+        }
    }

    /*
     * Validate if the given dir is a writeable directory if exists.
     */
    @SuppressWarnings("removal")


Same redundant suppression.

eirbjo · 2024-11-15T15:42:51Z

/reviewers 2

openjdk · 2024-11-15T15:43:47Z

@eirbjo
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).

RogerRiggs

LGTM

RogerRiggs · 2024-11-15T15:58:36Z

src/java.base/share/classes/jdk/internal/util/ClassFileDumper.java

@@ -79,10 +77,6 @@ public static ClassFileDumper getInstance(String key, String path) {
    private final AtomicInteger counter = new AtomicInteger();

    private ClassFileDumper(String key, String path) {
-        /*


The comment might still be relevant if changed to System.getProperty.

It's fine now: previously user defined security manager was effectively loading user code that uses java.lang.invoke, now users cannot specify security managers, so this problem does not exist any more.

In that case, the code should be calling System.getProperty and not the backup VM saved props.

@liach Does this mean we can/should use System.getProperty here?

VM saved props is still better imo as user code cannot tamper it with System.setProperty.

The use of the properties in ClassFileDumper is all in support of debug capabilities and is not sensitive.

Either way is fine. I think we probably need to review the majority of uses of VM.savedPropoerty as they mostly relate to SM boot circularity. So we will change this one either now or later.

I would prefer if we could deal with getSavedProperty as a wider area follow up, since that would creep out of scope for this particular PR.

Ok with you, @liach ?

Perhaps @seanjmullan has input on how to deal with VM.getSavedProperty across the code base?

I prefer to minimize code changes for these cleanup issues. We should file a separate issue, "Examine usages of VM.getSavedProperty after JEP 486" or something like that.

Thanks Sean, I filed https://bugs.openjdk.org/browse/JDK-8344335 to track this.

rgiulietti · 2024-11-15T18:12:31Z

src/java.base/share/classes/jdk/internal/util/random/RandomSupport.java

-                new sun.security.action.GetPropertyAction(
-                        "java.util.secureRandomSeed"));
-        return (pp != null && pp.equalsIgnoreCase("true"));
+        return Boolean.getBoolean("java.util.secureRandomSeed");


Not sure why access to this system property required privileges.

It is documented in SplittableRandom, so I guess it's OK to have unprivileged access.

Previously, all properties were enabled only if allowed by the security policy.
Any property could be denied. Though it could/would cause havoc if used indiscriminately.

bplb

Looks fine.

eirbjo · 2024-11-15T21:07:21Z

Thanks for the reviews and helpful discussions. I'll go ahead and integrate this pending a tier2 run.

eirbjo · 2024-11-16T13:30:31Z

tier2 run completed with only VerifyJimage.java failing. (This fails for some reason on all tier2 runs in my local MacOS build environment).

/integrate

openjdk · 2024-11-16T13:31:10Z

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

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
2c509a1: 8344326: Move jpackage tests from "jdk.jpackage.tests" package to the default package
41a2d49: 8331497: Implement JEP 483: Ahead-of-Time Class Loading & Linking
276251c: 8342826: Improve performance of oopDesc::klass() after JDK-8305895
c388455: 8344197: SM cleanup in java.util.concurrent
c5b6ed8: 8344252: SM cleanup in java.util classes
59ffac8: 8342811: java/net/httpclient/PlainProxyConnectionTest.java failed: Unexpected connection count: 5
f316808: 8344189: Cleanup code in sun.net.www.protocol.mailto.MailToURLConnection and sun.net.smtp after JEP 486 integration
... and 49 more: https://git.openjdk.org/jdk/compare/c3776db498193d45088ed19e2a1d2697281fd590...master

Your commit was automatically rebased without conflicts.

openjdk · 2024-11-16T13:31:20Z

@eirbjo Pushed as commit d0b770c.

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

Clean up ClassFileDumper, RandomSupport and StaticProperty following …

09c7c2e

…JEP486 integration

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

eirbjo marked this pull request as ready for review November 15, 2024 09:43

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

liach suggested changes Nov 15, 2024

View reviewed changes

Remove redundant suppression

64a9ec1

RogerRiggs approved these changes Nov 15, 2024

View reviewed changes

liach 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

rgiulietti reviewed Nov 15, 2024

View reviewed changes

bplb approved these changes Nov 15, 2024

View reviewed changes

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

openjdk bot closed this Nov 16, 2024

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8344289: SM cleanup in jdk.internal.util #22141

8344289: SM cleanup in jdk.internal.util #22141

eirbjo commented Nov 15, 2024 •

edited by openjdk bot

Loading

bridgekeeper bot commented Nov 15, 2024

openjdk bot commented Nov 15, 2024 •

edited

Loading

openjdk bot commented Nov 15, 2024

mlbridge bot commented Nov 15, 2024 •

edited

Loading

liach left a comment

liach Nov 15, 2024

eirbjo Nov 15, 2024

liach Nov 15, 2024

eirbjo commented Nov 15, 2024

openjdk bot commented Nov 15, 2024

RogerRiggs left a comment

RogerRiggs Nov 15, 2024

liach Nov 15, 2024

RogerRiggs Nov 15, 2024

eirbjo Nov 15, 2024

liach Nov 15, 2024

RogerRiggs Nov 15, 2024

liach Nov 15, 2024

eirbjo Nov 15, 2024

seanjmullan Nov 15, 2024

eirbjo Nov 15, 2024

rgiulietti Nov 15, 2024

RogerRiggs Nov 15, 2024

bplb left a comment

eirbjo commented Nov 15, 2024

eirbjo commented Nov 16, 2024

openjdk bot commented Nov 16, 2024

openjdk bot commented Nov 16, 2024

		@@ -134,48 +128,39 @@ public void dumpFailedClass(String name, byte[] bytes) {

		@SuppressWarnings("removal")

8344289: SM cleanup in jdk.internal.util #22141

8344289: SM cleanup in jdk.internal.util #22141

Conversation

eirbjo commented Nov 15, 2024 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Nov 15, 2024

openjdk bot commented Nov 15, 2024 • edited Loading

openjdk bot commented Nov 15, 2024

mlbridge bot commented Nov 15, 2024 • edited Loading

Webrevs

liach left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eirbjo commented Nov 15, 2024

openjdk bot commented Nov 15, 2024

RogerRiggs left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bplb left a comment

Choose a reason for hiding this comment

eirbjo commented Nov 15, 2024

eirbjo commented Nov 16, 2024

openjdk bot commented Nov 16, 2024

openjdk bot commented Nov 16, 2024

eirbjo commented Nov 15, 2024 •

edited by openjdk bot

Loading

openjdk bot commented Nov 15, 2024 •

edited

Loading

mlbridge bot commented Nov 15, 2024 •

edited

Loading