8272124: Cgroup v1 initialization causes NullPointerException when cgroup path contains colon #5127

hseigel · 2021-08-16T17:25:57Z

Please review this small fix for JDK-8272124. The fix puts a limit of 3 when splitting self cgroup lines by ':' so that Cgroup paths won't get truncated if they contain embedded ':'s. For example, an entry of "11:memory:/user.sli:ce" in a /proc/self/cgroup file will now result in a Cgroup path of "/user.sli:ce" instead of "/user.sli".

The fix was tested with Mach5 tiers 1 and 2, and Mach5 tiers 3-5 on Linux x64 and Linux aarch64.

Thanks, Harold

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed

Issue

JDK-8272124: Cgroup v1 initialization causes NullPointerException when cgroup path contains colon

Reviewers

Mikhailo Seledtsov (@mseledts - Committer) ⚠️ Review applies to 918df2b
Severin Gehwolf (@jerboaa - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/5127/head:pull/5127
$ git checkout pull/5127

Update a local copy of the PR:
$ git checkout pull/5127
$ git pull https://git.openjdk.java.net/jdk pull/5127/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 5127

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/5127.diff

…th contains colon

bridgekeeper · 2021-08-16T17:26:13Z

👋 Welcome back hseigel! 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 · 2021-08-16T17:29:04Z

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

core-libs
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 · 2021-08-16T17:33:36Z

Webrevs

hseigel · 2021-08-16T17:36:53Z

/label add runtime

openjdk · 2021-08-16T17:37:36Z

@hseigel The label runtime is not a valid label. These labels are valid:

serviceability
hotspot
sound
hotspot-compiler
ide-support
kulla
i18n
shenandoah
jdk
javadoc
security
2d
swing
hotspot-runtime
jmx
nio
build
beans
core-libs
compiler
net
hotspot-gc
hotspot-jfr
awt

hseigel · 2021-08-16T17:38:55Z

/label add hotspot-runtime

openjdk · 2021-08-16T17:39:58Z

@hseigel
The hotspot-runtime label was successfully added.

mseledts

Changes look good to me.

jerboaa · 2021-08-17T08:26:30Z

test/jdk/jdk/internal/platform/cgroup/TestCgroupSubsystemFactory.java

@@ -172,7 +172,7 @@
    private String mntInfoCgroupv1MoreCpusetLine = "121 32 0:37 / /cpuset rw,relatime shared:69 - cgroup none rw,cpuset\n";
    private String mntInfoCgroupsV1DoubleCpuset = mntInfoHybrid + mntInfoCgroupv1MoreCpusetLine;
    private String mntInfoCgroupsV1DoubleCpuset2 = mntInfoCgroupv1MoreCpusetLine + mntInfoHybrid;
-    private String cgroupv1SelfCgroupContent = "11:memory:/user.slice/user-1000.slice/user@1000.service\n" +
+    private String cgroupv1SelfCgroupContent = "11:memory:/user.slice/user-1000.slice/user@1000.s:ervice\n" +


Could you please add a comment explaining that /proc/self/cgroup is supposed to contain three colon-separated fields, hierarchy-ID:controller-list:cgroup-path. Thus, this cgroup-path intentionally contains a colon so as to ensure the correct path is being extracted by the logic in CgroupSubsystemFactory.

Better yet, add a new test with mountinfo and /proc/self/cgroup contents observed in the wild and assert that memoryInfo.getCgroupPath() equals memoryInfo.getMountRoot()

jerboaa · 2021-08-17T09:07:17Z

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

@@ -196,7 +196,7 @@ static CgroupMetrics create() {
            if (isCgroupsV2) {
                action = (tokens -> setCgroupV2Path(infos, tokens));
            }
-            selfCgroupLines.map(line -> line.split(":"))
+            selfCgroupLines.map(line -> line.split(":", 3))
                     .filter(tokens -> (tokens.length >= 3))


This filter no longer makes sense as tokens.length == 3 in every case after this patch. Please remove. From the javadoc from String.split(String, limit):

If the limit is positive then the pattern will be applied at most limit-1 times, the array's length will be no greater than limit, and the array's last entry will contain all input beyond the last matched delimiter.

On second thought, we might want to keep tokens.length == 3, but I don't know if it'll ever be < 3 in reality. It certainly would no longer be > 3.

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

hseigel · 2021-08-17T17:35:58Z

Thanks Misha and Severin for looking at this change!

Please review this updated commit that tries to address Severin's comments. A new test case was added to TestCgroupSubsystemFactory.java for the multiple ':'s case and comments were added to CgroupSubsystemFactory.java.

The ".filter(tokens -> (tokens.length >= 3))" code was removed but can be restored if need be.

Thanks, Harold

jerboaa · 2021-08-17T17:46:29Z

test/jdk/jdk/internal/platform/cgroup/TestCgroupSubsystemFactory.java

+        CgroupTypeResult res = result.get();
+        CgroupInfo memoryInfo = res.getInfos().get("memory");
+        assertEquals(memoryInfo.getCgroupPath(), "/system.slice/containerd.service/kubepods-burstable-podf65e797d_d5f9_4604_9773_94f4bb9946a0.slice:cri-containerd:86ac6260f9f8a9c1276748250f330ae9c2fcefe5ae809364ad1e45f3edf7e08a");
+        assertEquals(memoryInfo.getMountRoot(), memoryInfo.getMountRoot());


What I meant here is to also change the mountinfo entry for memory and then use

assertEquals(memoryInfo.getMountRoot(), memoryInfo.getCgroupPath());

as that's really what was happening in the wild. mount root - containing colons - would correctly parse, but the cgroup path - containing colons - would not and the assertion would fail (without the change in CgroupSubsystemFactory). Thanks!

@hseigel it still has assertEquals(memoryInfo.getMountRoot(), memoryInfo.getMountRoot()); - tautology - should be: assertEquals(memoryInfo.getMountRoot(), memoryInfo.getCgroupPath());

hseigel · 2021-08-18T12:22:07Z

Please review this update that modifies the new test case to have a mountinfo entry that contains multiple ":" characters.
Thanks, Harold

jerboaa · 2021-08-18T12:40:08Z

test/jdk/jdk/internal/platform/cgroup/TestCgroupSubsystemFactory.java

+    // `/proc/self/cgroup` should contain **three** colon-separated fields,
+    // `hierarchy-ID:controller-list:cgroup-path`. This cgroup-path intentionally
+    // contains a colon to ensure that the correct path is being extracted by the
+    // login in CgroupSubsystemFactory.


s/login/logic

jerboaa · 2021-08-18T12:44:40Z

test/jdk/jdk/internal/platform/cgroup/TestCgroupSubsystemFactory.java

+        CgroupTypeResult res = result.get();
+        CgroupInfo memoryInfo = res.getInfos().get("memory");
+        assertEquals(memoryInfo.getCgroupPath(), "/system.slice/containerd.service/kubepods-burstable-podf65e797d_d5f9_4604_9773_94f4bb9946a0.slice:cri-containerd:86ac6260f9f8a9c1276748250f330ae9c2fcefe5ae809364ad1e45f3edf7e08a");
+        assertEquals(memoryInfo.getMountRoot(), memoryInfo.getMountRoot());


@hseigel it still has assertEquals(memoryInfo.getMountRoot(), memoryInfo.getMountRoot()); - tautology - should be: assertEquals(memoryInfo.getMountRoot(), memoryInfo.getCgroupPath());

hseigel · 2021-08-18T13:01:18Z

Thanks for finding those issues. Please review the latest iteration of this fix.
Thanks, Harold

jerboaa

Looks good. Thank you!

openjdk · 2021-08-18T14:11:23Z

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

8272124: Cgroup v1 initialization causes NullPointerException when cgroup path contains colon

Reviewed-by: mseledtsov, 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 24 new commits pushed to the master branch:

30b0f82: 8272626: Avoid C-style array declarations in java.*
e8f1219: 8271276: C2: Wrong JVM state used for receiver null check
79a06df: 8272567: [IR Framework] Make AbstractInfo.getRandom() static
481c1f0: 8269951: [macos] Focus not painted in JButton when setBorderPainted(false) is invoked
5189047: 8272558: IR Test Framework README misses some flags
ec63957: 8272398: Update DockerTestUtils.buildJdkDockerImage()
14623cd: 8270835: regression after JDK-8261006
fe72197: 8272551: mark hotspot runtime/modules tests which ignore external VM flags
05d64da: 8272291: mark hotspot runtime/logging tests which ignore external VM flags
a68b5b9: 8272369: java/io/File/GetXSpace.java failed with "RuntimeException: java.nio.file.NoSuchFileException: /run/user/0"
... and 14 more: https://git.openjdk.java.net/jdk/compare/b2c272d4e21eb4aeb6c8e1c0be0d08943dc36db8...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.

hseigel · 2021-08-18T14:23:27Z

Thanks Severin and Misha for your reviews and improvements for this fix!

/integrate

openjdk · 2021-08-18T14:25:09Z

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

30b0f82: 8272626: Avoid C-style array declarations in java.*
e8f1219: 8271276: C2: Wrong JVM state used for receiver null check
79a06df: 8272567: [IR Framework] Make AbstractInfo.getRandom() static
481c1f0: 8269951: [macos] Focus not painted in JButton when setBorderPainted(false) is invoked
5189047: 8272558: IR Test Framework README misses some flags
ec63957: 8272398: Update DockerTestUtils.buildJdkDockerImage()
14623cd: 8270835: regression after JDK-8261006
fe72197: 8272551: mark hotspot runtime/modules tests which ignore external VM flags
05d64da: 8272291: mark hotspot runtime/logging tests which ignore external VM flags
a68b5b9: 8272369: java/io/File/GetXSpace.java failed with "RuntimeException: java.nio.file.NoSuchFileException: /run/user/0"
... and 14 more: https://git.openjdk.java.net/jdk/compare/b2c272d4e21eb4aeb6c8e1c0be0d08943dc36db8...master

Your commit was automatically rebased without conflicts.

openjdk · 2021-08-18T14:25:23Z

@hseigel Pushed as commit 4d6593c.

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

8272124: Cgroup v1 initialization causes NullPointerException when pa…

918df2b

…th contains colon

openjdk bot added the rfr Pull request is ready for review label Aug 16, 2021

openjdk bot added serviceability serviceability-dev@openjdk.org core-libs core-libs-dev@openjdk.org labels Aug 16, 2021

openjdk bot added the hotspot-runtime hotspot-runtime-dev@openjdk.org label Aug 16, 2021

mseledts approved these changes Aug 17, 2021

View reviewed changes

jerboaa reviewed Aug 17, 2021

View reviewed changes

add test case, comments, and other small changes

5fa37e9

jerboaa reviewed Aug 17, 2021

View reviewed changes

add mountinfo containing colongs to test

fcc8c90

hseigel changed the title ~~8272124: Cgroup v1 initialization causes NullPointerException when path contains colon~~ 8272124: Cgroup v1 initialization causes NullPointerException when cgroup path contains colon Aug 18, 2021

jerboaa suggested changes Aug 18, 2021

View reviewed changes

Fix test typo and error

f339fe1

jerboaa approved these changes Aug 18, 2021

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Aug 18, 2021

openjdk bot closed this Aug 18, 2021

openjdk bot added the integrated Pull request has been integrated label Aug 18, 2021

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8272124: Cgroup v1 initialization causes NullPointerException when cgroup path contains colon #5127

8272124: Cgroup v1 initialization causes NullPointerException when cgroup path contains colon #5127

hseigel commented Aug 16, 2021 •

edited by openjdk bot

bridgekeeper bot commented Aug 16, 2021

openjdk bot commented Aug 16, 2021

mlbridge bot commented Aug 16, 2021 •

edited

hseigel commented Aug 16, 2021

openjdk bot commented Aug 16, 2021

hseigel commented Aug 16, 2021

openjdk bot commented Aug 16, 2021

mseledts left a comment

jerboaa Aug 17, 2021

jerboaa Aug 17, 2021

jerboaa Aug 17, 2021

hseigel commented Aug 17, 2021

jerboaa Aug 17, 2021

jerboaa Aug 18, 2021

hseigel commented Aug 18, 2021

jerboaa Aug 18, 2021

jerboaa Aug 18, 2021

hseigel commented Aug 18, 2021

jerboaa left a comment

openjdk bot commented Aug 18, 2021 •

edited

hseigel commented Aug 18, 2021

openjdk bot commented Aug 18, 2021

openjdk bot commented Aug 18, 2021

8272124: Cgroup v1 initialization causes NullPointerException when cgroup path contains colon #5127

8272124: Cgroup v1 initialization causes NullPointerException when cgroup path contains colon #5127

Conversation

hseigel commented Aug 16, 2021 • edited by openjdk bot

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Aug 16, 2021

openjdk bot commented Aug 16, 2021

mlbridge bot commented Aug 16, 2021 • edited

Webrevs

hseigel commented Aug 16, 2021

openjdk bot commented Aug 16, 2021

hseigel commented Aug 16, 2021

openjdk bot commented Aug 16, 2021

mseledts left a comment

Choose a reason for hiding this comment

jerboaa Aug 17, 2021

Choose a reason for hiding this comment

jerboaa Aug 17, 2021

Choose a reason for hiding this comment

jerboaa Aug 17, 2021

Choose a reason for hiding this comment

hseigel commented Aug 17, 2021

jerboaa Aug 17, 2021

Choose a reason for hiding this comment

jerboaa Aug 18, 2021

Choose a reason for hiding this comment

hseigel commented Aug 18, 2021

jerboaa Aug 18, 2021

Choose a reason for hiding this comment

jerboaa Aug 18, 2021

Choose a reason for hiding this comment

hseigel commented Aug 18, 2021

jerboaa left a comment

Choose a reason for hiding this comment

openjdk bot commented Aug 18, 2021 • edited

hseigel commented Aug 18, 2021

openjdk bot commented Aug 18, 2021

openjdk bot commented Aug 18, 2021

hseigel commented Aug 16, 2021 •

edited by openjdk bot

mlbridge bot commented Aug 16, 2021 •

edited

openjdk bot commented Aug 18, 2021 •

edited