Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 4 commits into from
Closed
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
@@ -196,7 +196,7 @@ public static Optional<CgroupTypeResult> determineType(String mountInfo,
if (isCgroupsV2) {
action = (tokens -> setCgroupV2Path(infos, tokens));
}
selfCgroupLines.map(line -> line.split(":"))
selfCgroupLines.map(line -> line.split(":", 3))
hseigel marked this conversation as resolved.
Show resolved Hide resolved
.filter(tokens -> (tokens.length >= 3))
Copy link
Contributor

@jerboaa jerboaa Aug 17, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@jerboaa jerboaa Aug 17, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

.forEach(action);
}
@@ -172,7 +172,7 @@ public class TestCgroupSubsystemFactory {
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" +
Copy link
Contributor

@jerboaa jerboaa Aug 17, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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()

"10:hugetlb:/\n" +
"9:cpuset:/\n" +
"8:pids:/user.slice/user-1000.slice/user@1000.service\n" +
@@ -335,7 +335,7 @@ public void testHybridCgroupsV1() throws IOException {
CgroupTypeResult res = result.get();
assertFalse("hybrid hierarchy expected as cgroups v1", res.isCgroupV2());
CgroupInfo memoryInfo = res.getInfos().get("memory");
assertEquals("/user.slice/user-1000.slice/user@1000.service", memoryInfo.getCgroupPath());
assertEquals("/user.slice/user-1000.slice/user@1000.s:ervice", memoryInfo.getCgroupPath());
assertEquals("/", memoryInfo.getMountRoot());
assertEquals("/sys/fs/cgroup/memory", memoryInfo.getMountPoint());
}