8345286: Remove use of SecurityManager API from misc areas #22478

jaikiran · 2024-12-02T12:13:57Z

Can I please get a review of this change which removes usages of SecurityManager related APIs and some leftover related to SecurityManager changes?

This addresses https://bugs.openjdk.org/browse/JDK-8345286. Most of these changes are trivial. The src/java.base/linux/classes/jdk/internal/platform/CgroupUtil.java used to expose utility methods for dealing with SecurityManager permissions and it was called from a few places. That class is no longer needed with the clean up done in this PR.

No new tests have been introduced and tier testing is currently in progress.

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-8345286: Remove use of SecurityManager API from misc areas (Enhancement - P4)

Reviewers

Alan Bateman (@AlanBateman - Reviewer)
Severin Gehwolf (@jerboaa - Reviewer) Review applies to 9baa279b
Kevin Walls (@kevinjwalls - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 22478

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

Using diff file

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

Using Webrev

Link to Webrev Comment

bridgekeeper · 2024-12-02T12:14:31Z

👋 Welcome back jpai! 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-12-02T12:15:16Z

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

8345286: Remove use of SecurityManager API from misc areas

Reviewed-by: alanb, kevinw, sgehwolf

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

38927fc: 8343213: TEST_BUG: [Graal] java/lang/ref/Basic.java fails
cf1eb58: 8344935: [ubsan]: javaThread.hpp:1241:52: runtime error: load of value 9831830, which is not a valid value for type 'freeze_result'
943aa03: 8345404: [ppc64le] ProblemList TestAllocOutOfMemory.java#large
9e2b66f: 8345178: RISC-V: Add gtests for narrow cmpxchg
4c33caa: 8344609: Check ResourceMark nesting when allocating a GrowableArray on an alternative ResourceArea
4b92816: 8345375: Improve debuggability of test/jdk/java/net/Socket/CloseAvailable.java
e15912b: 8345248: Module name 'transitive' not accepted for requires transitive
521ed72: 8345357: test/jdk/javax/swing/JRadioButton/8033699/bug8033699.java fails in ubuntu22.04
447f8d4: 8345353: Test for JDK-8344800 W3C DTDs and XSDs in the built-in Catalog
43b337e: 8344304: [s390x] ubsan: negation of -2147483648 cannot be represented in type 'int'
... and 37 more: https://git.openjdk.org/jdk/compare/a3b58ee5cd1ec0ea78649d4128d272458b05eb13...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-12-02T12:15:54Z

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

core-libs
net
security
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.

jaikiran · 2024-12-02T12:16:44Z

Hello Severin @jerboaa, given your work in the cgroups area of the JDK, could you help review the updates in this PR for those classes?

mlbridge · 2024-12-02T12:20:08Z

Webrevs

AlanBateman

Good cleanup, a few small nits spotted along the way.

AlanBateman · 2024-12-02T12:39:30Z

src/java.base/share/classes/sun/reflect/misc/MethodUtil.java

 import java.security.AllPermission;
 import java.security.CodeSource;
 import java.security.PermissionCollection;
-import java.security.PrivilegedExceptionAction;


I'm half tempted to suggest leaving MethodUtil out of this change. There is further work required here and leaving the SM usage is a reminder of that.

Understood. I've now updated this PR to revert back the changes in this file.

AlanBateman · 2024-12-02T12:42:47Z

src/java.base/linux/classes/jdk/internal/platform/CgroupSubsystemController.java

-        }
-        catch (IOException e) {
+        try (BufferedReader bufferedReader =
+                         Files.newBufferedReader(Paths.get(controller.path(), param))) {


The formatting has got messed up here. If you create Path path = Path.of(controller.path(), param) then the try line would fit on one line and would fix the formatting issue. Maybe some future cleanup will replace this with Files.lines as this just needs to return the first line.

Fixed. I moved the Path.of() outside of the try-with-resources and the line length is now more reasonable.

AlanBateman · 2024-12-02T12:43:20Z

src/java.base/linux/classes/jdk/internal/platform/CgroupSubsystemController.java

        if (controller == null) return defaultRetval;

-        try (Stream<String> lines = CgroupUtil.readFilePrivileged(Paths.get(controller.path(), param))) {
+        try (Stream<String> lines = Files.lines(Paths.get(controller.path(), param))) {


Using Path.of might be clearer here.

What Alan said.

Done, here and one other place in this file.

AlanBateman · 2024-12-02T12:45:00Z

src/java.management/share/classes/sun/management/VMManagementImpl.java

        // construct PerfInstrumentation object
-        @SuppressWarnings("removal")
-        Perf perf =  AccessController.doPrivileged(new Perf.GetPerfAction());
+        Perf perf =  Perf.getPerf();


An extra space crept in that at some point

Thanks for spotting that, I hadn't noticed it.

jerboaa

I've reviewed only the cgroups parts.

jerboaa · 2024-12-02T13:06:05Z

src/java.base/linux/classes/jdk/internal/platform/cgroupv2/CgroupV2Subsystem.java

-            return CgroupUtil.readFilePrivileged(Paths.get(unified.path(), "io.stat"))
-                                .map(mapFunc)
-                                .collect(Collectors.summingLong(e -> e));
+        try (Stream<String> lines = Files.lines(Paths.get(unified.path(), "io.stat"))) {


Suggested change

try (Stream<String> lines = Files.lines(Paths.get(unified.path(), "io.stat"))) {

try (Stream<String> lines = Files.lines(Paths.of(unified.path(), "io.stat"))) {

jerboaa · 2024-12-02T13:33:17Z

src/java.base/linux/classes/jdk/internal/platform/CgroupSubsystemController.java

+        try (BufferedReader bufferedReader =
+                         Files.newBufferedReader(Paths.get(controller.path(), param))) {
+            String line = bufferedReader.readLine();
+            return line;
+        } catch (IOException e) {


I think we can do this refactoring now:

Suggested change

try (BufferedReader bufferedReader =

Files.newBufferedReader(Paths.get(controller.path(), param))) {

String line = bufferedReader.readLine();

return line;

} catch (IOException e) {

try (Stream<String> lines =

Files.lines(Paths.of(controller.path(), param))) {

Optional<String> firstLine = lines.findFirst();

return firstLine.orElse(null);

} catch (IOException e) {

jerboaa · 2024-12-02T13:33:56Z

src/java.base/linux/classes/jdk/internal/platform/CgroupSubsystemController.java

        if (controller == null) return defaultRetval;

-        try (Stream<String> lines = CgroupUtil.readFilePrivileged(Paths.get(controller.path(), param))) {
+        try (Stream<String> lines = Files.lines(Paths.get(controller.path(), param))) {


What Alan said.

src/java.base/linux/classes/jdk/internal/platform/cgroupv2/CgroupV2Subsystem.java

src/java.base/linux/classes/jdk/internal/platform/CgroupSubsystemController.java

…oupV2Subsystem.java Co-authored-by: Severin Gehwolf <jerboaa@gmail.com>

…temController.java Co-authored-by: Severin Gehwolf <jerboaa@gmail.com>

jerboaa

cgroups changes look good. Haven't looked at the other bits.

jerboaa · 2024-12-02T14:16:30Z

src/java.base/linux/classes/jdk/internal/platform/CgroupSubsystemController.java


 package jdk.internal.platform;

+import java.io.BufferedReader;


Unused now?

Yes, it's now unused. I've removed it in the latest update to the PR and also replaced a few more Paths.get() with Path.of() in the current set of files that have been updated in this PR.

jaikiran · 2024-12-02T14:21:34Z

cgroups changes look good. Haven't looked at the other bits.

Thank you Severin.

AlanBateman · 2024-12-02T14:25:12Z

src/java.base/linux/classes/jdk/internal/platform/CgroupSubsystemController.java

@@ -1,5 +1,6 @@
 /*
 * Copyright (c) 2020, 2022, Red Hat Inc.
+ * Copyright (c) 2024, Oracle and/or its affiliates. All rights reserved.


Not sure about this as it's not a significant change to the file.

Do you mean for just this file or the rest of the files in this PR as well? I am open to removing the copyright additions.

AlanBateman · 2024-12-02T15:42:37Z

src/java.base/unix/classes/sun/security/provider/NativePRNG.java

    /**
     * Create a RandomIO object for all I/O of this Variant type.
     */
-    @SuppressWarnings("removal")


Sean has included in this one in the changes for sun.security (pull/22418) so I think you can drop it from this PR.

Done. Removed it from this PR.

…tivePRNG.java

openjdk · 2024-12-02T15:56:09Z

@jaikiran 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 8345286
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

jerboaa

cgroup changes still look good.

dfuch

Good cleanup! Changes to logger + net look fine.

dfuch · 2024-12-02T17:47:57Z

src/java.base/linux/classes/jdk/internal/platform/CgroupSubsystemController.java

+            Optional<String> firstLine = lines.findFirst();
+            return firstLine.orElse(null);


you could probably just:

return lines.findFirst().orElse(null);

unless the local variable is needed or type inference?

Hello Daniel, no syntactical reason behind this. Severin proposed this style in his review and I noticed that this area of the code has been using this style of local variable assigment and immediate return in one or two other places, so I decided to stick with it. I don't have a strong preference but am willing to update it if you or others think we should.

In that case you may leave it as is - it's fine with me.

seanjmullan · 2024-12-02T18:48:27Z

src/java.base/share/classes/jdk/internal/misc/InnocuousThread.java

 import java.util.concurrent.atomic.AtomicInteger;

 /**
 * A thread that has no permissions, is not a member of any user-defined


I think you can also remove the words "has no permissions" as it no longer applies. @AlanBateman ?

Yes, that can be removed.

jaikiran · 2024-12-03T09:03:20Z

Although trivial, there are some changes to files from the serviceability area. So it would be good if someone from that area could review this too.

kevinjwalls · 2024-12-04T09:20:57Z

Although trivial, there are some changes to files from the serviceability area. So it would be good if someone from that area could review this too.

Yes, looks good. I will update #22478 to avoid the clash!

jaikiran · 2024-12-04T09:23:14Z

Thank you everyone for the help in reviewing this.

jaikiran · 2024-12-04T09:23:19Z

/integrate

openjdk · 2024-12-04T09:23:57Z

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

38927fc: 8343213: TEST_BUG: [Graal] java/lang/ref/Basic.java fails
cf1eb58: 8344935: [ubsan]: javaThread.hpp:1241:52: runtime error: load of value 9831830, which is not a valid value for type 'freeze_result'
943aa03: 8345404: [ppc64le] ProblemList TestAllocOutOfMemory.java#large
9e2b66f: 8345178: RISC-V: Add gtests for narrow cmpxchg
4c33caa: 8344609: Check ResourceMark nesting when allocating a GrowableArray on an alternative ResourceArea
4b92816: 8345375: Improve debuggability of test/jdk/java/net/Socket/CloseAvailable.java
e15912b: 8345248: Module name 'transitive' not accepted for requires transitive
521ed72: 8345357: test/jdk/javax/swing/JRadioButton/8033699/bug8033699.java fails in ubuntu22.04
447f8d4: 8345353: Test for JDK-8344800 W3C DTDs and XSDs in the built-in Catalog
43b337e: 8344304: [s390x] ubsan: negation of -2147483648 cannot be represented in type 'int'
... and 37 more: https://git.openjdk.org/jdk/compare/a3b58ee5cd1ec0ea78649d4128d272458b05eb13...master

Your commit was automatically rebased without conflicts.

openjdk · 2024-12-04T09:24:06Z

@jaikiran Pushed as commit 3d49665.

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

jaikiran added 2 commits December 2, 2024 15:33

8345286: Remove use of SecurityManager API from misc areas

1902fad

update VectorIntrinsics too

e36c520

openjdk bot added the rfr Pull request is ready for review label Dec 2, 2024

openjdk bot added serviceability serviceability-dev@openjdk.org security security-dev@openjdk.org core-libs core-libs-dev@openjdk.org net net-dev@openjdk.org labels Dec 2, 2024

AlanBateman approved these changes Dec 2, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Dec 2, 2024

jaikiran added 4 commits December 2, 2024 18:58

leave out MethodUtil from the clean up

8248cd6

fix formatting of try-with-resources in CgroupSubsystemController.java

3512a42

Path.of() instead of Paths.get()

30d729d

remove unnecessary space

75511e3

openjdk bot removed the ready Pull request is ready to be integrated label Dec 2, 2024

jerboaa reviewed Dec 2, 2024

View reviewed changes

jaikiran and others added 3 commits December 2, 2024 19:29

Path.of() instead of Paths.get() in CgroupV2Subsystem.java

ad62bd4

Update src/java.base/linux/classes/jdk/internal/platform/cgroupv2/Cgr…

52a47f7

…oupV2Subsystem.java Co-authored-by: Severin Gehwolf <jerboaa@gmail.com>

Update src/java.base/linux/classes/jdk/internal/platform/CgroupSubsys…

26df25b

…temController.java Co-authored-by: Severin Gehwolf <jerboaa@gmail.com>

jerboaa approved these changes Dec 2, 2024

View reviewed changes

replace remaining Paths.get() with Path.of() in the updated files

b5f2ed9

openjdk bot added the ready Pull request is ready to be integrated label Dec 2, 2024

remove unused import

b018ef0

openjdk bot removed the ready Pull request is ready to be integrated label Dec 2, 2024

AlanBateman reviewed Dec 2, 2024

View reviewed changes

AlanBateman approved these changes Dec 2, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Dec 2, 2024

AlanBateman reviewed Dec 2, 2024

View reviewed changes

remove changes to src/java.base/unix/classes/sun/security/provider/Na…

5a96fa5

…tivePRNG.java

openjdk bot added merge-conflict Pull request has merge conflict with target branch and removed ready Pull request is ready to be integrated labels Dec 2, 2024

merge latest from master branch

9baa279

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

jerboaa approved these changes Dec 2, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Dec 2, 2024

dfuch reviewed Dec 2, 2024

View reviewed changes

seanjmullan reviewed Dec 2, 2024

View reviewed changes

jaikiran added 2 commits December 3, 2024 11:31

remove permission text from InnocuousThread

02d2a81

merge latest from master branch

d53fe8a

openjdk bot removed the ready Pull request is ready to be integrated label Dec 3, 2024

AlanBateman approved these changes Dec 3, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Dec 3, 2024

kevinjwalls approved these changes Dec 4, 2024

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Dec 4, 2024

openjdk bot closed this Dec 4, 2024

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

jaikiran deleted the 8345286 branch December 4, 2024 09:24

	try (Stream<String> lines = Files.lines(Paths.get(unified.path(), "io.stat"))) {
	try (Stream<String> lines = Files.lines(Paths.of(unified.path(), "io.stat"))) {


		package jdk.internal.platform;

		import java.io.BufferedReader;

		Optional<String> firstLine = lines.findFirst();
		return firstLine.orElse(null);

8345286: Remove use of SecurityManager API from misc areas #22478

8345286: Remove use of SecurityManager API from misc areas #22478

Uh oh!

Conversation

jaikiran commented Dec 2, 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 Dec 2, 2024

Uh oh!

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

Uh oh!

Uh oh!

openjdk bot commented Dec 2, 2024

Uh oh!

jaikiran commented Dec 2, 2024

Uh oh!

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

Uh oh!

Webrevs

Uh oh!

AlanBateman left a comment

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

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!

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!

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!

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

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

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

jaikiran commented Dec 2, 2024

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

openjdk bot commented Dec 2, 2024

Uh oh!

jerboaa left a comment

jaikiran commented Dec 2, 2024 •

edited by openjdk bot

Loading

openjdk bot commented Dec 2, 2024 •

edited

Loading

mlbridge bot commented Dec 2, 2024 •

edited

Loading

AlanBateman Dec 2, 2024 •

edited

Loading