From ff6fe1324663538167eca8b3d3eec61e1bd4fa51 Mon Sep 17 00:00:00 2001 From: Aleksa Sarai Date: Fri, 20 Jun 2025 15:43:49 +1000 Subject: [PATCH] utils: use safe procfs for /proc/self/fd loop code From a safety perspective this might not be strictly required, but it paves the way for us to remove utils.ProcThreadSelf. Signed-off-by: Aleksa Sarai --- internal/linux/linux.go | 19 +++++++++++++++++++ libcontainer/integration/exec_test.go | 15 ++++++++------- libcontainer/utils/utils_unix.go | 11 ++++++----- utils_linux.go | 9 +++++++-- 4 files changed, 40 insertions(+), 14 deletions(-) diff --git a/internal/linux/linux.go b/internal/linux/linux.go index ef230b133..88fe5e0df 100644 --- a/internal/linux/linux.go +++ b/internal/linux/linux.go @@ -86,6 +86,25 @@ func SetMempolicy(mode uint, mask *unix.CPUSet) error { return os.NewSyscallError("set_mempolicy", err) } +// Readlinkat wraps [unix.Readlinkat]. +func Readlinkat(dir *os.File, path string) (string, error) { + size := 4096 + for { + linkBuf := make([]byte, size) + n, err := retryOnEINTR2(func() (int, error) { + return unix.Readlinkat(int(dir.Fd()), path, linkBuf) + }) + if err != nil { + return "", &os.PathError{Op: "readlinkat", Path: dir.Name() + "/" + path, Err: err} + } + if n != size { + return string(linkBuf[:n]), nil + } + // Possible truncation, resize the buffer. + size *= 2 + } +} + // GetPtyPeer is a wrapper for ioctl(TIOCGPTPEER). func GetPtyPeer(ptyFd uintptr, unsafePeerPath string, flags int) (*os.File, error) { // Make sure O_NOCTTY is always set -- otherwise runc might accidentally diff --git a/libcontainer/integration/exec_test.go b/libcontainer/integration/exec_test.go index f5a24777e..c46a576ea 100644 --- a/libcontainer/integration/exec_test.go +++ b/libcontainer/integration/exec_test.go @@ -15,10 +15,11 @@ import ( "github.com/opencontainers/cgroups" "github.com/opencontainers/cgroups/systemd" + "github.com/opencontainers/runc/internal/linux" + "github.com/opencontainers/runc/internal/pathrs" "github.com/opencontainers/runc/libcontainer" "github.com/opencontainers/runc/libcontainer/configs" "github.com/opencontainers/runc/libcontainer/internal/userns" - "github.com/opencontainers/runc/libcontainer/utils" "github.com/opencontainers/runtime-spec/specs-go" "golang.org/x/sys/unix" @@ -1683,11 +1684,9 @@ func TestFdLeaksSystemd(t *testing.T) { } func fdList(t *testing.T) []string { - procSelfFd, closer := utils.ProcThreadSelf("fd") - defer closer() - - fdDir, err := os.Open(procSelfFd) + fdDir, closer, err := pathrs.ProcThreadSelfOpen("fd/", unix.O_DIRECTORY|unix.O_CLOEXEC) ok(t, err) + defer closer() defer fdDir.Close() fds, err := fdDir.Readdirnames(-1) @@ -1726,8 +1725,10 @@ func testFdLeaks(t *testing.T, systemd bool) { count := 0 - procSelfFd, closer := utils.ProcThreadSelf("fd/") + procSelfFd, closer, err := pathrs.ProcThreadSelfOpen("fd/", unix.O_DIRECTORY|unix.O_CLOEXEC) + ok(t, err) defer closer() + defer procSelfFd.Close() next_fd: for _, fd1 := range fds1 { @@ -1736,7 +1737,7 @@ next_fd: continue next_fd } } - dst, _ := os.Readlink(filepath.Join(procSelfFd, fd1)) + dst, _ := linux.Readlinkat(procSelfFd, fd1) for _, ex := range excludedPaths { if ex == dst { continue next_fd diff --git a/libcontainer/utils/utils_unix.go b/libcontainer/utils/utils_unix.go index f2c1e1f2b..c07d2e7d9 100644 --- a/libcontainer/utils/utils_unix.go +++ b/libcontainer/utils/utils_unix.go @@ -14,6 +14,7 @@ import ( securejoin "github.com/cyphar/filepath-securejoin" "github.com/opencontainers/runc/internal/linux" + "github.com/opencontainers/runc/internal/pathrs" "github.com/sirupsen/logrus" "golang.org/x/sys/unix" ) @@ -59,15 +60,15 @@ type fdFunc func(fd int) // fdRangeFrom calls the passed fdFunc for each file descriptor that is open in // the current process. func fdRangeFrom(minFd int, fn fdFunc) error { - procSelfFd, closer := ProcThreadSelf("fd") - defer closer() - - fdDir, err := os.Open(procSelfFd) + fdDir, closer, err := pathrs.ProcThreadSelfOpen("fd/", unix.O_DIRECTORY|unix.O_CLOEXEC) if err != nil { - return err + return fmt.Errorf("get handle to /proc/thread-self/fd: %w", err) } + defer closer() defer fdDir.Close() + // NOTE: This is not really necessary since securejoin.ProcThreadSelf + // verifies this in a far stricter sense than EnsureProcHandle. if err := EnsureProcHandle(fdDir); err != nil { return err } diff --git a/utils_linux.go b/utils_linux.go index bd45bf059..6195a5898 100644 --- a/utils_linux.go +++ b/utils_linux.go @@ -16,6 +16,7 @@ import ( "github.com/urfave/cli" "golang.org/x/sys/unix" + "github.com/opencontainers/runc/internal/pathrs" "github.com/opencontainers/runc/libcontainer" "github.com/opencontainers/runc/libcontainer/configs" "github.com/opencontainers/runc/libcontainer/specconv" @@ -241,10 +242,14 @@ func (r *runner) run(config *specs.Process) (int, error) { process.ExtraFiles = append(process.ExtraFiles, r.listenFDs...) } baseFd := 3 + len(process.ExtraFiles) - procSelfFd, closer := utils.ProcThreadSelf("fd/") + 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 = os.Stat(filepath.Join(procSelfFd, strconv.Itoa(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) }