Skip to content

Fix --preserve-fds validation logic. #5475

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

Open
mgelde wants to merge 1 commit into
base: main
Choose a base branch
from
+32 −9
Open

Fix --preserve-fds validation logic. #5475

Show file tree
Hide file tree
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 tests/integration/exec.bats
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -180,6 +180,20 @@ function teardown() {
[ "${output}" = "hello" ]
}

@test "runc exec --preserve-fds with no inherited fd" {
runc run -d --console-socket "$CONSOLE_SOCKET" test_busybox
[ "$status" -eq 0 ]

setup_runc_cmdline
log=preserve-fds.log
# Use a separate shell so closing fd 3 does not interfere with bats itself.
# The check must happen before runc opens its log file in fd 3 and mistakes
# that descriptor for one inherited from runc's caller.
run bash -c 'exec 3>&-; exec "$@"' bash "${RUNC_CMDLINE[@]}" --log "$log" exec --preserve-fds=1 test_busybox true
[ "$status" -ne 0 ]
[[ "$output" == *"preserved-fd 0"* ]]
}

function check_exec_debug() {
[[ "$*" == *"nsexec container setup"* ]]
[[ "$*" == *"child process in init()"* ]]
Expand Down
7 changes: 7 additions & 0 deletions tests/integration/run.bats
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,13 @@ function teardown() {
[ "$status" -ne 0 ]
}

@test "runc run --preserve-fds with no inherited fd" {
setup_runc_cmdline
run bash -c 'exec 3>&-; exec "$@"' bash "${RUNC_CMDLINE[@]}" run --preserve-fds=1 test_missing_fd
[ "$status" -ne 0 ]
[[ "$output" == *"preserved-fd 0"* ]]
}

@test "runc run --keep" {
runc run --keep test_run_keep
[ "$status" -eq 0 ]
Expand Down
20 changes: 11 additions & 9 deletions utils_linux.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@ import (
"github.com/urfave/cli/v3"
"golang.org/x/sys/unix"

"github.com/opencontainers/runc/internal/pathrs"
"github.com/opencontainers/runc/internal/third_party/systemd/activation"
"github.com/opencontainers/runc/libcontainer"
"github.com/opencontainers/runc/libcontainer/configs"
Expand Down Expand Up @@ -245,16 +244,19 @@ func (r *runner) run(config *specs.Process) (_ int, retErr error) {
process.ExtraFiles = append(process.ExtraFiles, r.listenFDs...)
}
baseFd := 3 + len(process.ExtraFiles)
procSelfFd, closer, err := pathrs.ProcThreadSelfOpen("fd/", unix.O_DIRECTORY|unix.O_CLOEXEC)
if err != nil {
return -1, err
}
defer closer()
defer procSelfFd.Close()
for i := baseFd; i < baseFd+r.preserveFDs; i++ {
err := unix.Faccessat(int(procSelfFd.Fd()), strconv.Itoa(i), unix.F_OK, 0)

@kolyshkin kolyshkin Sep 15, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Since Go runtime and runc itself opens all FDs with CLOEXEC, we can just replace unix.Faccessat to unix.Fctnl(F_GETFD) and check there's no CLOEXEC flag, and error out otherwise (basically the check you added to checkPreserveFDs can be done right here).

This will simplify the logic a lot (no cli parsing, no NumFiles etc) without any downsides.

@kolyshkin kolyshkin Sep 15, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I mean, the fix itself (modulo tests) can be as simple as this:

--- a/utils_linux.go
+++ b/utils_linux.go
@@ -15,7 +15,6 @@ import (
      "github.com/urfave/cli/v3"
      "golang.org/x/sys/unix"

-     "github.com/opencontainers/runc/internal/pathrs"
      "github.com/opencontainers/runc/internal/third_party/systemd/activation"
      "github.com/opencontainers/runc/libcontainer"
      "github.com/opencontainers/runc/libcontainer/configs"
@@ -245,16 +244,15 @@ func (r *runner) run(config *specs.Process) (_ int, retErr error) {
              process.ExtraFiles = append(process.ExtraFiles, r.listenFDs...)
      }
      baseFd := 3 + len(process.ExtraFiles)
-     procSelfFd, closer, err := pathrs.ProcThreadSelfOpen("fd/", unix.O_DIRECTORY|unix.O_CLOEXEC)
-     if err != nil {
-             return -1, err
-     }
-     defer closer()
-     defer procSelfFd.Close()
      for i := baseFd; i < baseFd+r.preserveFDs; i++ {
-             err := unix.Faccessat(int(procSelfFd.Fd()), strconv.Itoa(i), unix.F_OK, 0)
-             if err != nil {
-                     return -1, fmt.Errorf("unable to stat preserved-fd %d (of %d): %w", i-baseFd, r.preserveFDs, err)
+             // Check that the fd was really inherited from runc's caller. Merely
+             // checking that the fd is open is not sufficient, as the fd number
+             // could have been reused by runc itself (or the Go runtime). Any such
+             // fd has the close-on-exec flag set, while an inherited one can not
+             // have it, as it would have been closed by execve.
+             flags, err := unix.FcntlInt(uintptr(i), unix.F_GETFD, 0)
+             if err != nil || flags&unix.FD_CLOEXEC != 0 {
+                     return -1, fmt.Errorf("preserved-fd %d (of %d) was not passed to runc", i-baseFd, r.preserveFDs)
              }
              process.ExtraFiles = append(process.ExtraFiles, os.NewFile(uintptr(i), "PreserveFD:"+strconv.Itoa(i)))
      }

(note I simplified the error message as well)

@mgelde mgelde Sep 16, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I see no way that should fail, as long as all those FDs really always set CLOEXEC; now and forever AND this check continues to happen both before execve AND before joining the container namespaces e.g. during runc run --preserve-fds=X (otherwise we'd have a race). At present that appears to be the case.

So if that is how it's done (i.e. CLOEXEC is always set), I agree the extra safety of checking early does not outweigh the added complexity.

I'll push an updated version.


// Check that the fd was really inherited from runc's caller. Merely
// checking that the fd is open is not sufficient, as the fd number
// could have been reused by runc itself (or the Go runtime). Any such
// fd has the close-on-exec flag set, while an inherited one can not
// have it, as it would have been closed by execve.
flags, err := unix.FcntlInt(uintptr(i), unix.F_GETFD, 0)
if err != nil {
return -1, fmt.Errorf("unable to stat preserved-fd %d (of %d): %w", i-baseFd, r.preserveFDs, err)
return -1, fmt.Errorf("fcntl on preserved-fd %d (of %d) failed: %w", i-baseFd, r.preserveFDs, err)
}
if flags&unix.FD_CLOEXEC != 0 {
return -1, fmt.Errorf("preserved-fd %d (of %d) has the close-on-exec flag set", i-baseFd, r.preserveFDs)
}
process.ExtraFiles = append(process.ExtraFiles, os.NewFile(uintptr(i), "PreserveFD:"+strconv.Itoa(i)))
}
Expand Down