Fix runc exec vs go1.26 + older kernel

[kolyshkin]

This is an alternative to #5066, fixing runc exec vs go1.26 issue with 1.4.0.
Go 1.26.0 is to be released soon (in February).

Fixes: #5060
Closes: #5066

---

Since PR 4812, runc exec tries to use clone3 syscall with
CLONE_INTO_CGROUP, falling back to the old method if it is not
supported.

One issue with that approach is, a

Cmd cannot be reused after calling its [Cmd.Start], [Cmd.Run],
[Cmd.Output], or [Cmd.CombinedOutput] methods.

(from https://pkg.go.dev/os/exec#Cmd).

This is enforced since Go 1.26, see CL 728642, and so runc exec
actually fails in specific scenarios (go1.26 and no CLONE_INTO_CGROUP
support).

The easiest workaround is to pre-copy the p.cmd structure. From the
CL 734200 it looks like it is an acceptable way. If the upstream
will introduce cmd.Clone we'll switch to it.

As for the test case, it looks like adding Go 1.26 to the testing
matrix is sufficient to reveal the issue (some tests involving
runc exec fail with Go 1.26 before the fix).

[kolyshkin]

Note that copying of exec.Cmd is not recommended but accepted by upstream, judging by CL 734200. They may introduce cmd.Clone method later and we'll switch to it.

[kolyshkin]

@rata @lifubang @cyphar PTAL

[lifubang]

This is a great solution—overall, LGTM.

Would you consider cherry-picking the unit test from #5066, or adding a similar test case? I’m concerned about whether the second run will succeed without it, for example, maybe some fds has been closed in the first run.

[rata]

@kolyshkin so let me see if I'm following correctly. The proposal to add cmd.Clone() about cloning mentions this:

Generally this involves making a shallow copy of all exported fields: cmd = &exec.Cmd{Path: cmd.Path, Args: cmd.Args, ...}.

However, we think it is kind of accepted upstream for now because this CL https://go-review.googlesource.com/c/go/+/734200 is changing the atomic.Bool that causes go vet or something to fail, it is changed to something else that avoid the warning. Therefore, they are allowing this workaround for now.

Am I getting it right?

In that case, it seems fine, but it might break again in 1.27 (as they implied this will be removed if cmd.Clone() is merged).

[kolyshkin]

This is a great solution—overall, LGTM.

Would you consider cherry-picking the unit test from #5066, or adding a similar test case? I’m concerned about whether the second run will succeed without it, for example, maybe some fds has been closed in the first run.

@lifubang this case is already (implicitly) covered by the existing tests. For example, runc exec (cgroup v2 + init process in non-root cgroup) succeeds makes a container, moves its init to a sub-cgroup and then enables a domain controller. In this situation, we can no longer join the default (container's top-level) cgroup (IOW clone3(CLONE_INTO_CGROUP) fails) and fall back to reading the init's cgroup. When we run this test on Go 1.26 (which forces the error on the second cmd.Exec), it fails.

The failing test part is here:

runc/tests/integration/cgroups.bats

Lines 109 to 112 in 08072e9

	# an exec process can no longer join "/" after turning on a domain controller.
	# falls back to "/foo".
	runc exec test_cgroups_group cat /proc/self/cgroup
	[ "$status" -eq 0 ]

The failure itself (before the commit with the fix) is here: https://github.com/opencontainers/runc/actions/runs/21379582395/job/61543423814

This is the reason why I'm adding Go 1.26 to the CI matrix.

I am working on simplifying the test from #5066 (in its current form it's too complicated) so maybe I will add something here, but in general it's not needed.

[kolyshkin]

Am I getting it right?

In that case, it seems fine, but it might break again in 1.27 (as they implied this will be removed if cmd.Clone() is merged).

You are right, it may stop working in case they will use something in exec.Command that could/should not be copied. Let's hope they are to add the Clone method in this case, and we'll just switch to it (depending on Go version):

//go:build go1.27
func cloneCmd(cmd *exec.Cmd) *exec.Cmd {
       return cmd.Clone()
}

//go:build !go1.27
func cloneCmd(cmd *exec.Cmd) *exec.Cmd {
    cmdCopy = *cmd
    return &cmdCopy
}

and then, eventually, remove the cloneCmd wrapper and use cmd.Clone directly.

[thaJeztah]

change LGTM

are new tests needed? (I see the other PR added some tests, but I haven't looked if they're good)

[thaJeztah]

Oh, lol, missed the discussion (thanks browser refresh 😆)

[kolyshkin]

@lifubang OK I've added a (slightly simplified) test from #5066 but in fact the same code path is covered by the existing runc exec (cgroup v2 + init process in non-root cgroup) succeeds bats test (perhaps in a less hackish manner), so it's up to you whether you want to include it or not.

[lifubang]

but in fact the same code path is covered by the existing runc exec (cgroup v2 + init process in non-root cgroup) succeeds bats test

Yes, you are right.

so it's up to you whether you want to include it or not.

I think it's not necessary to include another unit test, from my perspective, you can remove the last commit.

[rata]

You are right, it may stop working in case they will use something in exec.Command that could/should not be copied. Let's hope they are to add the Clone method in this case, and we'll just switch to it (depending on Go version):

//go:build go1.27
func cloneCmd(cmd *exec.Cmd) *exec.Cmd {
       return cmd.Clone()
}

//go:build !go1.27
func cloneCmd(cmd *exec.Cmd) *exec.Cmd {
    cmdCopy = *cmd
    return &cmdCopy
}

and then, eventually, remove the cloneCmd wrapper and use cmd.Clone directly.

@kolyshkin I'd still prefer to have the pre go 1.27 cloneCmd() just copy all the public fields of the struct. It is still quite simple (https://pkg.go.dev/os/exec#Cmd there are not so many, AI can probably do it :D) and we don't have to make a release on all the branches when go 1.27 is released. We can just switch whenever we want. We can just copy all of them, that is all.

The only potential downside is if more fields are added in 1.26 and we start using them and we forget to update this copy. It seems unlikely, I don't think 1.26 will add fields. This is short lived (ideally, until 1.27 comes out), then we can remove it. So I don't think this risk (that is the only I see) is a concern.

What do you think?

[kolyshkin]

@kolyshkin I'd still prefer to have the pre go 1.27 cloneCmd() just copy all the public fields of the struct. It is still quite simple (https://pkg.go.dev/os/exec#Cmd there are not so many, AI can probably do it :D) and we don't have to make a release on all the branches when go 1.27 is released. We can just switch whenever we want. We can just copy all of them, that is all.

The only potential downside is if more fields are added in 1.26 and we start using them and we forget to update this copy. It seems unlikely, I don't think 1.26 will add fields. This is short lived (ideally, until 1.27 comes out), then we can remove it. So I don't think this risk (that is the only I see) is a concern.

What do you think?

@rata done, PTAL

[kolyshkin]

A few things to note here:

1. We can probably be fine with a post-copy (IOW copy the command only after the first Start failed), but it's more risk theoretically so I left things as is (ie pre-copy).
2. I chose not to copy Process and ProcessState fields -- the first one is populated upon the successful start, the second -- upon the process exit.
3. I chose not to copy Err -- in most cases it is populated upon the process start or exit, except that exec.Command() can also set it. Practically, exec.Command can set it on Windows or if the command path requires lookup -- neither of which is the case here. But for safely I've added the appropriate check (as a separate commit).

[kolyshkin]

The first commit should be a draft until Go 1.26 is released, but we can patch this up later. Or, I can remove the first commit for now, for a cleaner git history.