From 65cdbad8c1a0057d07c78474cdb0ea60278c0984 Mon Sep 17 00:00:00 2001 From: Cory Snider Date: Wed, 28 Sep 2022 13:33:53 -0400 Subject: [PATCH] Lock OS threads when exec'ing with Pdeathsig On Linux, when (os/exec.Cmd).SysProcAttr.Pdeathsig is set, the signal will be sent to the process when the OS thread on which cmd.Start() was executed dies. The runtime terminates an OS thread when a goroutine exits after being wired to the thread with runtime.LockOSThread(). If other goroutines are allowed to be scheduled onto a thread which called cmd.Start(), an unrelated goroutine could cause the thread to be terminated and prematurely signal the command. See https://github.com/golang/go/issues/27505 for more information. Prevent started subprocesses with Pdeathsig from getting signaled prematurely by wiring the starting goroutine to the OS thread until the subprocess has exited. No other goroutines can be scheduled onto a locked thread so it will remain alive until unlocked or the daemon process exits. Signed-off-by: Cory Snider --- daemon/graphdriver/overlay2/mount.go | 7 +++++ libcontainerd/supervisor/remote_daemon.go | 25 ++++++++++++++--- libnetwork/portmapper/proxy_linux.go | 33 ++++++++++++++++++++--- pkg/chrootarchive/archive_unix.go | 12 +++++++++ pkg/chrootarchive/diff_unix.go | 6 +++++ 5 files changed, 75 insertions(+), 8 deletions(-) diff --git a/daemon/graphdriver/overlay2/mount.go b/daemon/graphdriver/overlay2/mount.go index dcd7c0149009d..eff0f781bfab7 100644 --- a/daemon/graphdriver/overlay2/mount.go +++ b/daemon/graphdriver/overlay2/mount.go @@ -50,6 +50,13 @@ func mountFrom(dir, device, target, mType string, flags uintptr, label string) e output := bytes.NewBuffer(nil) cmd.Stdout = output cmd.Stderr = output + + // reexec.Command() sets cmd.SysProcAttr.Pdeathsig on Linux, which + // causes the started process to be signaled when the creating OS thread + // dies. Ensure that the reexec is not prematurely signaled. See + // https://go.dev/issue/27505 for more information. + runtime.LockOSThread() + defer runtime.UnlockOSThread() if err := cmd.Start(); err != nil { w.Close() return fmt.Errorf("mountfrom error on re-exec cmd: %v", err) diff --git a/libcontainerd/supervisor/remote_daemon.go b/libcontainerd/supervisor/remote_daemon.go index 5213854ac88e1..a81bff75b2a8e 100644 --- a/libcontainerd/supervisor/remote_daemon.go +++ b/libcontainerd/supervisor/remote_daemon.go @@ -6,6 +6,7 @@ import ( "os" "os/exec" "path/filepath" + "runtime" "strconv" "strings" "time" @@ -200,18 +201,34 @@ func (r *remote) startContainerd() error { cmd.Env = append(cmd.Env, e) } } - if err := cmd.Start(); err != nil { - return err - } - r.daemonWaitCh = make(chan struct{}) + startedCh := make(chan error) go func() { + // On Linux, when cmd.SysProcAttr.Pdeathsig is set, + // the signal is sent to the subprocess when the creating thread + // terminates. The runtime terminates a thread if a goroutine + // exits while locked to it. Prevent the containerd process + // from getting killed prematurely by ensuring that the thread + // used to start it remains alive until it or the daemon process + // exits. See https://go.dev/issue/27505 for more details. + runtime.LockOSThread() + defer runtime.UnlockOSThread() + err := cmd.Start() + startedCh <- err + if err != nil { + return + } + + r.daemonWaitCh = make(chan struct{}) // Reap our child when needed if err := cmd.Wait(); err != nil { r.logger.WithError(err).Errorf("containerd did not exit successfully") } close(r.daemonWaitCh) }() + if err := <-startedCh; err != nil { + return err + } r.daemonPid = cmd.Process.Pid diff --git a/libnetwork/portmapper/proxy_linux.go b/libnetwork/portmapper/proxy_linux.go index fa0d11f88456b..7df0ab5157d4e 100644 --- a/libnetwork/portmapper/proxy_linux.go +++ b/libnetwork/portmapper/proxy_linux.go @@ -6,6 +6,7 @@ import ( "net" "os" "os/exec" + "runtime" "strconv" "syscall" "time" @@ -37,16 +38,18 @@ func newProxyCommand(proto string, hostIP net.IP, hostPort int, containerIP net. Path: path, Args: args, SysProcAttr: &syscall.SysProcAttr{ - Pdeathsig: syscall.SIGTERM, // send a sigterm to the proxy if the daemon process dies + Pdeathsig: syscall.SIGTERM, // send a sigterm to the proxy if the creating thread in the daemon process dies (https://go.dev/issue/27505) }, }, + wait: make(chan error, 1), }, nil } // proxyCommand wraps an exec.Cmd to run the userland TCP and UDP // proxies as separate processes. type proxyCommand struct { - cmd *exec.Cmd + cmd *exec.Cmd + wait chan error } func (p *proxyCommand) Start() error { @@ -56,7 +59,29 @@ func (p *proxyCommand) Start() error { } defer r.Close() p.cmd.ExtraFiles = []*os.File{w} - if err := p.cmd.Start(); err != nil { + + // As p.cmd.SysProcAttr.Pdeathsig is set, the signal will be sent to the + // process when the OS thread on which p.cmd.Start() was executed dies. + // If the thread is allowed to be released back into the goroutine + // thread pool, the thread could get terminated at any time if a + // goroutine gets scheduled onto it which calls runtime.LockOSThread() + // and exits without a matching number of runtime.UnlockOSThread() + // calls. Ensure that the thread from which Start() is called stays + // alive until the proxy or the daemon process exits to prevent the + // proxy from getting terminated early. See https://go.dev/issue/27505 + // for more details. + started := make(chan error) + go func() { + runtime.LockOSThread() + defer runtime.UnlockOSThread() + err := p.cmd.Start() + started <- err + if err != nil { + return + } + p.wait <- p.cmd.Wait() + }() + if err := <-started; err != nil { return err } w.Close() @@ -92,7 +117,7 @@ func (p *proxyCommand) Stop() error { if err := p.cmd.Process.Signal(os.Interrupt); err != nil { return err } - return p.cmd.Wait() + return <-p.wait } return nil } diff --git a/pkg/chrootarchive/archive_unix.go b/pkg/chrootarchive/archive_unix.go index b3a8ae1135ab4..aa90255abb9e6 100644 --- a/pkg/chrootarchive/archive_unix.go +++ b/pkg/chrootarchive/archive_unix.go @@ -95,6 +95,12 @@ func invokeUnpack(decompressedArchive io.Reader, dest string, options *archive.T cmd.Stdout = output cmd.Stderr = output + // reexec.Command() sets cmd.SysProcAttr.Pdeathsig on Linux, which + // causes the started process to be signaled when the creating OS thread + // dies. Ensure that the reexec is not prematurely signaled. See + // https://go.dev/issue/27505 for more information. + runtime.LockOSThread() + defer runtime.UnlockOSThread() if err := cmd.Start(); err != nil { w.Close() return fmt.Errorf("Untar error on re-exec cmd: %v", err) @@ -188,6 +194,12 @@ func invokePack(srcPath string, options *archive.TarOptions, root string) (io.Re return nil, errors.Wrap(err, "error getting options pipe for tar process") } + // reexec.Command() sets cmd.SysProcAttr.Pdeathsig on Linux, which + // causes the started process to be signaled when the creating OS thread + // dies. Ensure that the reexec is not prematurely signaled. See + // https://go.dev/issue/27505 for more information. + runtime.LockOSThread() + defer runtime.UnlockOSThread() if err := cmd.Start(); err != nil { return nil, errors.Wrap(err, "tar error on re-exec cmd") } diff --git a/pkg/chrootarchive/diff_unix.go b/pkg/chrootarchive/diff_unix.go index e1bf74d1d5edd..554f934a2b96f 100644 --- a/pkg/chrootarchive/diff_unix.go +++ b/pkg/chrootarchive/diff_unix.go @@ -115,6 +115,12 @@ func applyLayerHandler(dest string, layer io.Reader, options *archive.TarOptions outBuf, errBuf := new(bytes.Buffer), new(bytes.Buffer) cmd.Stdout, cmd.Stderr = outBuf, errBuf + // reexec.Command() sets cmd.SysProcAttr.Pdeathsig on Linux, which + // causes the started process to be signaled when the creating OS thread + // dies. Ensure that the reexec is not prematurely signaled. See + // https://go.dev/issue/27505 for more information. + runtime.LockOSThread() + defer runtime.UnlockOSThread() if err = cmd.Run(); err != nil { return 0, fmt.Errorf("ApplyLayer %s stdout: %s stderr: %s", err, outBuf, errBuf) }