From 7094efb192714e37b1ab731ffe5c6c676b18aa87 Mon Sep 17 00:00:00 2001 From: Aleksa Sarai Date: Sat, 20 Jan 2024 16:58:28 +1100 Subject: [PATCH] init: use *os.File for passed file descriptors While it doesn't make much of a practical difference, it seems far more reasonable to use os.NewFile to wrap all of our passed file descriptors to make sure they're tracked by the Go runtime and that we don't double-close them. Signed-off-by: Aleksa Sarai --- libcontainer/init_linux.go | 22 ++++++++++++---------- libcontainer/mount_linux.go | 6 +++--- libcontainer/setns_init_linux.go | 7 +++---- libcontainer/standard_init_linux.go | 13 ++++++------- libcontainer/utils/utils_unix.go | 9 +++++++++ 5 files changed, 33 insertions(+), 24 deletions(-) diff --git a/libcontainer/init_linux.go b/libcontainer/init_linux.go index 0117ace5990..c46cf0ddf7a 100644 --- a/libcontainer/init_linux.go +++ b/libcontainer/init_linux.go @@ -137,24 +137,26 @@ func startInitialization() (retErr error) { logrus.SetLevel(logrus.Level(logLevel)) } - logFD, err := strconv.Atoi(os.Getenv("_LIBCONTAINER_LOGPIPE")) + logFd, err := strconv.Atoi(os.Getenv("_LIBCONTAINER_LOGPIPE")) if err != nil { return fmt.Errorf("unable to convert _LIBCONTAINER_LOGPIPE: %w", err) } + logPipe := os.NewFile(uintptr(logFd), "logpipe") - logrus.SetOutput(os.NewFile(uintptr(logFD), "logpipe")) + logrus.SetOutput(logPipe) logrus.SetFormatter(new(logrus.JSONFormatter)) logrus.Debug("child process in init()") // Only init processes have FIFOFD. - fifofd := -1 + var fifoFile *os.File envInitType := os.Getenv("_LIBCONTAINER_INITTYPE") it := initType(envInitType) if it == initStandard { - envFifoFd := os.Getenv("_LIBCONTAINER_FIFOFD") - if fifofd, err = strconv.Atoi(envFifoFd); err != nil { + fifoFd, err := strconv.Atoi(os.Getenv("_LIBCONTAINER_FIFOFD")) + if err != nil { return fmt.Errorf("unable to convert _LIBCONTAINER_FIFOFD: %w", err) } + fifoFile = os.NewFile(uintptr(fifoFd), "initfifo") } var consoleSocket *os.File @@ -208,10 +210,10 @@ func startInitialization() (retErr error) { } // If init succeeds, it will not return, hence none of the defers will be called. - return containerInit(it, &config, syncPipe, consoleSocket, pidfdSocket, fifofd, logFD, dmzExe) + return containerInit(it, &config, syncPipe, consoleSocket, pidfdSocket, fifoFile, logPipe, dmzExe) } -func containerInit(t initType, config *initConfig, pipe *syncSocket, consoleSocket, pidfdSocket *os.File, fifoFd, logFd int, dmzExe *os.File) error { +func containerInit(t initType, config *initConfig, pipe *syncSocket, consoleSocket, pidfdSocket, fifoFile, logPipe, dmzExe *os.File) error { if err := populateProcessEnvironment(config.Env); err != nil { return err } @@ -223,7 +225,7 @@ func containerInit(t initType, config *initConfig, pipe *syncSocket, consoleSock consoleSocket: consoleSocket, pidfdSocket: pidfdSocket, config: config, - logFd: logFd, + logPipe: logPipe, dmzExe: dmzExe, } return i.Init() @@ -234,8 +236,8 @@ func containerInit(t initType, config *initConfig, pipe *syncSocket, consoleSock pidfdSocket: pidfdSocket, parentPid: unix.Getppid(), config: config, - fifoFd: fifoFd, - logFd: logFd, + fifoFile: fifoFile, + logPipe: logPipe, dmzExe: dmzExe, } return i.Init() diff --git a/libcontainer/mount_linux.go b/libcontainer/mount_linux.go index 285e03fa491..f9b1adf51db 100644 --- a/libcontainer/mount_linux.go +++ b/libcontainer/mount_linux.go @@ -113,12 +113,12 @@ func mountViaFds(source string, srcFile *mountSource, target, dstFd, fstype stri // mount(2), we need to get a safe handle to /proc/thread-self. This // isn't needed for move_mount(2) because in that case the path is just // a dummy string used for error info. - fdStr := strconv.Itoa(int(srcFile.file.Fd())) + srcFileFd := srcFile.file.Fd() if isMoveMount { - src = "/proc/self/fd/" + fdStr + src = "/proc/self/fd/" + strconv.Itoa(int(srcFileFd)) } else { var closer utils.ProcThreadSelfCloser - src, closer = utils.ProcThreadSelf("fd/" + fdStr) + src, closer = utils.ProcThreadSelfFd(srcFileFd) defer closer() } } diff --git a/libcontainer/setns_init_linux.go b/libcontainer/setns_init_linux.go index 0dd72f95e7f..b88c6136e1f 100644 --- a/libcontainer/setns_init_linux.go +++ b/libcontainer/setns_init_linux.go @@ -5,7 +5,6 @@ import ( "fmt" "os" "os/exec" - "strconv" "github.com/opencontainers/selinux/go-selinux" "github.com/sirupsen/logrus" @@ -24,7 +23,7 @@ type linuxSetnsInit struct { consoleSocket *os.File pidfdSocket *os.File config *initConfig - logFd int + logPipe *os.File dmzExe *os.File } @@ -131,8 +130,8 @@ func (l *linuxSetnsInit) Init() error { // Close the log pipe fd so the parent's ForwardLogs can exit. logrus.Debugf("setns_init: about to exec") - if err := unix.Close(l.logFd); err != nil { - return &os.PathError{Op: "close log pipe", Path: "fd " + strconv.Itoa(l.logFd), Err: err} + if err := l.logPipe.Close(); err != nil { + return fmt.Errorf("close log pipe: %w", err) } if l.dmzExe != nil { diff --git a/libcontainer/standard_init_linux.go b/libcontainer/standard_init_linux.go index 3096d0d81ee..dbaf30f02ba 100644 --- a/libcontainer/standard_init_linux.go +++ b/libcontainer/standard_init_linux.go @@ -5,7 +5,6 @@ import ( "fmt" "os" "os/exec" - "strconv" "github.com/opencontainers/runtime-spec/specs-go" "github.com/opencontainers/selinux/go-selinux" @@ -25,8 +24,8 @@ type linuxStandardInit struct { consoleSocket *os.File pidfdSocket *os.File parentPid int - fifoFd int - logFd int + fifoFile *os.File + logPipe *os.File dmzExe *os.File config *initConfig } @@ -244,11 +243,11 @@ func (l *linuxStandardInit) Init() error { // Close the log pipe fd so the parent's ForwardLogs can exit. logrus.Debugf("init: about to wait on exec fifo") - if err := unix.Close(l.logFd); err != nil { - return &os.PathError{Op: "close log pipe", Path: "fd " + strconv.Itoa(l.logFd), Err: err} + if err := l.logPipe.Close(); err != nil { + return fmt.Errorf("close log pipe: %w", err) } - fifoPath, closer := utils.ProcThreadSelf("fd/" + strconv.Itoa(l.fifoFd)) + fifoPath, closer := utils.ProcThreadSelfFd(l.fifoFile.Fd()) defer closer() // Wait for the FIFO to be opened on the other side before exec-ing the @@ -269,7 +268,7 @@ func (l *linuxStandardInit) Init() error { // N.B. the core issue itself (passing dirfds to the host filesystem) has // since been resolved. // https://github.com/torvalds/linux/blob/v4.9/fs/exec.c#L1290-L1318 - _ = unix.Close(l.fifoFd) + _ = l.fifoFile.Close() s := l.config.SpecState s.Pid = unix.Getpid() diff --git a/libcontainer/utils/utils_unix.go b/libcontainer/utils/utils_unix.go index a48221b000a..3e16d1efb57 100644 --- a/libcontainer/utils/utils_unix.go +++ b/libcontainer/utils/utils_unix.go @@ -202,3 +202,12 @@ func ProcThreadSelf(subpath string) (string, ProcThreadSelfCloser) { } return threadSelf + subpath, runtime.UnlockOSThread } + +// ProcThreadSelfFd is small wrapper around ProcThreadSelf to make it easier to +// create a /proc/thread-self handle for given file descriptor. +// +// It is basically equivalent to ProcThreadSelf(fmt.Sprintf("fd/%d", fd)), but +// without using fmt.Sprintf to avoid unneeded overhead. +func ProcThreadSelfFd(fd uintptr) (string, ProcThreadSelfCloser) { + return ProcThreadSelf("fd/" + strconv.FormatUint(uint64(fd), 10)) +}