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.

Sign up for GitHub

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

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

Merged
merged 1 commit into from
Oct 21, 2024
Merged

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

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions libcontainer/container_linux.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -618,6 +618,8 @@ func (c *Container) newParentProcess(p *Process) (parentProcess, error) {
)
}

// TODO: After https://go-review.googlesource.com/c/go/+/515799 included
// in go versions supported by us, we can remove this logic.
if safeExe != nil {
// Due to a Go stdlib bug, we need to add safeExe to the set of
// ExtraFiles otherwise it is possible for the stdlib to clobber the fd
Expand All @@ -628,6 +630,18 @@ func (c *Container) newParentProcess(p *Process) (parentProcess, error) {
//
// See <https://github.com/golang/go/issues/61751>.
cmd.ExtraFiles = append(cmd.ExtraFiles, safeExe)

// 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.
// 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.
cmd.Path = "/proc/self/fd/" + strconv.Itoa(stdioFdCount+len(cmd.ExtraFiles)-1)
Copy link
Member

Choose a reason for hiding this comment

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

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.

}

// NOTE: when running a container with no PID namespace and the parent
Expand Down
Loading