From c4dea880acc89e9128077a62c4e15488f0a0533b 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 | 21 ++++++++++++--------- libcontainer/mount_linux.go | 6 +++--- libcontainer/setns_init_linux.go | 7 +++---- libcontainer/standard_init_linux.go | 13 ++++++------- libcontainer/utils/utils_unix.go | 5 +++++ 5 files changed, 29 insertions(+), 23 deletions(-) diff --git a/libcontainer/init_linux.go b/libcontainer/init_linux.go index 0117ace5990..db1b88a507b 100644 --- a/libcontainer/init_linux.go +++ b/libcontainer/init_linux.go @@ -137,23 +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 { + if fd, err := strconv.Atoi(envFifoFd); err != nil { return fmt.Errorf("unable to convert _LIBCONTAINER_FIFOFD: %w", err) + } else { + fifoFile = os.NewFile(uintptr(fd), "initfifo") } } @@ -208,10 +211,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 +226,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 +237,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..6ef5dd0f01f 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 err } if l.dmzExe != nil { diff --git a/libcontainer/standard_init_linux.go b/libcontainer/standard_init_linux.go index 3096d0d81ee..f463c46cc9b 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 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..004d6bcd353 100644 --- a/libcontainer/utils/utils_unix.go +++ b/libcontainer/utils/utils_unix.go @@ -202,3 +202,8 @@ func ProcThreadSelf(subpath string) (string, ProcThreadSelfCloser) { } return threadSelf + subpath, runtime.UnlockOSThread } + +// ProcThreadSelfFd is shorthand for ProcThreadSelf(fmt.Sprintf("fd/%d", fd)). +func ProcThreadSelfFd(fd uintptr) (string, ProcThreadSelfCloser) { + return ProcThreadSelf("fd/" + strconv.FormatUint(uint64(fd), 10)) +}