fix an error caused by fd reuse race when starting runc init

[lifubang]

Fixes: #4294

There is a race situation when we are opening a file, if there is a
small fd was closed at that time, maybe it will be reused by safeExe.
Because of Go stdlib fds shuffling bug, if the fd of safeExe is too
small, go stdlib will dup3 it to another fd, or dup3 a other fd to this
fd, then it will cause the fd type cmd.Path refers to a random path,
and it can lead to an error "permission denied" when starting the process.

Please see #4294 and golang/go#61751.

So we should not use the original fd of safeExe, but use the fd after
shuffled by Go stdlib. Because Go stdlib will guarantee this fd refers to
the correct file.

Signed-off-by: lfbzhm [email protected]

[kolyshkin]

Related: it would be nice to finish https://go-review.googlesource.com/c/go/+/515799

[lifubang]

Related: it would be nice to finish https://go-review.googlesource.com/c/go/+/515799

Yes, but this logic is very complex, maybe need more time to test.

Another thing I want to know, the closed fd 6/7 is opened and closed by whom? I printed the first 6 fds:

fd 0 -> /dev/null
fd 1 -> /dev/pts/3
fd 2 -> /dev/pts/3
fd 3 -> /sys/fs/cgroup
fd 4 -> anon_inode:[eventpoll]
fd 5 -> anon_inode:[eventfd]

Maybe by go runtime?

[cyphar]

This solution is simpler than the previous one. It's still a bit ugly but at least we are using ExtraFiles "properly" now.

I'll see if I have some time to rework the Go stdlib patch.

[lifubang]

It's still a bit ugly

Yes, another ugly place:

runc/libcontainer/container_linux.go

Line 563 in e669926

exePath = "/proc/self/fd/" + strconv.Itoa(int(safeExe.Fd()))

We should set a correct valut to exePath at this line, but it's hard to move the patch code to this place. Because append some files to p.ExtraFiles will cause more problems.
It indeed seems that it's the simplest way to fix the bug at this time.

[cyphar]

Yeah, it would be nice if Go supported fork+fexecve natively, but I don't think there's even a nice backward-compatible way of doing that.

[rata]

LGTM, thanks for fixing this nasty bug! I guess the debugging wasn't easy :D

[rata]

Super nit: I think it would be slightly more clear if we add a comment here: https://github.com/opencontainers/runc/blob/main/libcontainer/container_linux.go#L563 saying we will override it later.

Also, IMHO having this in a function and just calling it from https://github.com/opencontainers/runc/blob/main/libcontainer/container_linux.go#L563 seems slightly better. But I don't know if it feels safer, due to this nasty bug, to do it way later when we won't have a small fd number (to be extra cautious).

In any case, this is very subjective and if the code as is feels better, I'm completely fine. And even if we want to add it, this can be a follow-up PR.