Cgroup paths containing ":" are not supported

[elezar]

Considering

procfs/proc_cgroup.go

Line 52 in bc932d6

fields := strings.Split(cgroupStr, ":")

it is clear that if the path contains additional ":" characters, the path will be truncated.

Colons may be inserted, for example when running kubernetes with containerd using a systemd cgroup-driver.

See:

• https://github.com/containerd/containerd/blob/c35b4cfed520bf4827737f294f9292ffa73bdf8b/pkg/cri/server/helpers_linux.go#L67

And how containerd parses the file: https://github.com/containerd/cgroups/blob/8a68de567b68b30602948417a7a8e99ccf00ee9b/utils.go#L305

Changing:

	fields := strings.Split(cgroupStr, ":")

to

	fields := strings.SplitN(cgroupStr, ":", 3)

should address this.

[discordianfish]

Good catch, yeah this seems to be a bug. Can you submit a PR to change the lines as suggested?

[elezar]

I see that #260 added an explicit check and a test that would have to be modified.

The name on the test is:

"extra fields (such as those added by later kernel versions)"

Before I go and change this -- which I'm happy to do -- it may be good to get @hamiltont (who authored the PR) and @pgier who reviewed it to weigh in here.

[hamiltont]

Nice bug report. Here is a screengrab of the relevant bit of conversation from the linked PR. It was buried deep it the thread as an outdated discussion, but luckily Github preserved it for us :) Seems to me you could continue addressing this concern when using SplitN (I'm no expert at golang, so this may have just been my lack of knowledge that caused me to use Split). If you see a way to improve the docs on that one test case I would encourage that as well. Because it's testing for a pretty rare event, it may be hard for someone reading the test case to understand what exactly we are looking for (e.g. it's a rock you don't often look under). IMO this means it's a good test case, because it helps avoid a subtle "gotcha" that only comes from experience, but it also means that one test case is deserving of some clear documentation

[elezar]

Yes, I saw the discussion you posted, but it does not really answer my question. This functionality seems to cover hypothetical future situations, but currently breaks in at least one real-world use case (containerd under kubernetes uses : to separate path components when systemd is used as a cgroup driver).

My proposal would be to only handle the first two : characters and assume that everything following the second is the cgroup path. Note that the spec for the file does not state that the path cannot contain colons.

[discordianfish]

Yeah, I was under the impression that : is a reserved separator. Looks like that's not the case, so the kernel can't introduce new fields and there is no need to worry about it.
So agreed that we should change:

fields := strings.Split(cgroupStr, ":")

to

fields := strings.SplitN(cgroupStr, ":", 3)

as @elezar suggested

[elezar]

Thanks @discordianfish

With regards to:

Yeah, I was under the impression that : is a reserved separator. Looks like that's not the case, so the kernel can't introduce new fields and there is no need to worry about it.

Just for my information / education: where would one check / confirm this?

[discordianfish]

Just for my information / education: where would one check / confirm this?

If you find out, let me know :D I think the kernel source is the only way to verify. There doesn't seem to be much standards around these kind of things.

[elezar]

I will do some digging when I get a chance, but for the time being I have created #354