From d78b26ea6eeb1253489b9e91c9402bcd65f34bf2 Mon Sep 17 00:00:00 2001 From: lifubang Date: Wed, 19 Mar 2025 10:50:33 +0000 Subject: [PATCH 1/4] fix a ambiguity of err in defer func We should use a named return value of error, or else we can't catch all errors when calling defer function, for example we used a block scope var name `err` for `setupPidfdSocket`. Signed-off-by: lifubang --- utils_linux.go | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/utils_linux.go b/utils_linux.go index bd45bf05937..b6c863beaf7 100644 --- a/utils_linux.go +++ b/utils_linux.go @@ -218,14 +218,13 @@ type runner struct { subCgroupPaths map[string]string } -func (r *runner) run(config *specs.Process) (int, error) { - var err error +func (r *runner) run(config *specs.Process) (_ int, retErr error) { defer func() { - if err != nil { + if retErr != nil { r.destroy() } }() - if err = r.checkTerminal(config); err != nil { + if err := r.checkTerminal(config); err != nil { return -1, err } process, err := newProcess(config) From cfbdb2a4ae8e371f3b4b94530b518547d2bd476e Mon Sep 17 00:00:00 2001 From: lifubang Date: Sat, 22 Mar 2025 00:08:16 +0000 Subject: [PATCH 2/4] move process terminate operation to defer function Signed-off-by: lifubang --- utils_linux.go | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/utils_linux.go b/utils_linux.go index b6c863beaf7..80a241a4ebc 100644 --- a/utils_linux.go +++ b/utils_linux.go @@ -281,22 +281,23 @@ func (r *runner) run(config *specs.Process) (_ int, retErr error) { if err != nil { return -1, err } + defer func() { + // We should terminate the process once we got an error. + if retErr != nil { + r.terminate(process) + } + }() if err = tty.waitConsole(); err != nil { - r.terminate(process) return -1, err } tty.ClosePostStart() if r.pidFile != "" { if err = createPidFile(r.pidFile, process); err != nil { - r.terminate(process) return -1, err } } handler := <-handlerCh status, err := handler.forward(process, tty, detach) - if err != nil { - r.terminate(process) - } if detach { return 0, nil } From f6f8db797221698842f80e3868a798d945be6326 Mon Sep 17 00:00:00 2001 From: lifubang Date: Tue, 18 Mar 2025 14:25:02 +0000 Subject: [PATCH 3/4] move notifySocket out of signalHandler In fact, notifySocket has no relationship to signalHandler, we can move it out of signalHandler to make the code more clear. Signed-off-by: lifubang --- notify_socket.go | 16 ++++++++++++++++ signals.go | 21 ++++----------------- utils_linux.go | 7 ++++++- 3 files changed, 26 insertions(+), 18 deletions(-) diff --git a/notify_socket.go b/notify_socket.go index afebf4e77e4..728f41e312d 100644 --- a/notify_socket.go +++ b/notify_socket.go @@ -151,6 +151,22 @@ func (s *notifySocket) run(pid1 int) error { } } +// forward reads systemd notifications from the container and forwards them +// to notifySocketHost. +func (s *notifySocket) forward(process *libcontainer.Process, detach bool) error { + if detach { + pid, err := process.Pid() + if err != nil { + return err + } + _ = s.run(pid) + } else { + _ = s.run(os.Getpid()) + go func() { _ = s.run(0) }() + } + return nil +} + // notifyHost tells the host (usually systemd) that the container reported READY. // Also sends MAINPID and BARRIER. func notifyHost(client *net.UnixConn, ready []byte, pid1 int) error { diff --git a/signals.go b/signals.go index 936d751f61f..15f16de756c 100644 --- a/signals.go +++ b/signals.go @@ -16,9 +16,7 @@ const signalBufferSize = 2048 // newSignalHandler returns a signal handler for processing SIGCHLD and SIGWINCH signals // while still forwarding all other signals to the process. -// If notifySocket is present, use it to read systemd notifications from the container and -// forward them to notifySocketHost. -func newSignalHandler(enableSubreaper bool, notifySocket *notifySocket) chan *signalHandler { +func newSignalHandler(enableSubreaper bool) chan *signalHandler { if enableSubreaper { // set us as the subreaper before registering the signal handler for the container if err := system.SetSubreaper(1); err != nil { @@ -37,8 +35,7 @@ func newSignalHandler(enableSubreaper bool, notifySocket *notifySocket) chan *si // handle all signals for the process. signal.Notify(s) handler <- &signalHandler{ - signals: s, - notifySocket: notifySocket, + signals: s, } }() return handler @@ -52,8 +49,7 @@ type exit struct { } type signalHandler struct { - signals chan os.Signal - notifySocket *notifySocket + signals chan os.Signal } // forward handles the main signal event loop forwarding, resizing, or reaping depending @@ -61,7 +57,7 @@ type signalHandler struct { func (h *signalHandler) forward(process *libcontainer.Process, tty *tty, detach bool) (int, error) { // make sure we know the pid of our main process so that we can return // after it dies. - if detach && h.notifySocket == nil { + if detach { return 0, nil } @@ -70,15 +66,6 @@ func (h *signalHandler) forward(process *libcontainer.Process, tty *tty, detach return -1, err } - if h.notifySocket != nil { - if detach { - _ = h.notifySocket.run(pid1) - return 0, nil - } - _ = h.notifySocket.run(os.Getpid()) - go func() { _ = h.notifySocket.run(0) }() - } - // Perform the initial tty resize. Always ignore errors resizing because // stdout might have disappeared (due to races with when SIGHUP is sent). _ = tty.resize() diff --git a/utils_linux.go b/utils_linux.go index 80a241a4ebc..004ff404b12 100644 --- a/utils_linux.go +++ b/utils_linux.go @@ -253,7 +253,7 @@ func (r *runner) run(config *specs.Process) (_ int, retErr error) { // Setting up IO is a two stage process. We need to modify process to deal // with detaching containers, and then we get a tty after the container has // started. - handlerCh := newSignalHandler(r.enableSubreaper, r.notifySocket) + handlerCh := newSignalHandler(r.enableSubreaper) tty, err := setupIO(process, r.container, config.Terminal, detach, r.consoleSocket) if err != nil { return -1, err @@ -296,6 +296,11 @@ func (r *runner) run(config *specs.Process) (_ int, retErr error) { return -1, err } } + if r.notifySocket != nil { + if err = r.notifySocket.forward(process, detach); err != nil { + return -1, err + } + } handler := <-handlerCh status, err := handler.forward(process, tty, detach) if detach { From 7c8bf79798aeb65ef8f838deb8b0a74f7e4168af Mon Sep 17 00:00:00 2001 From: lifubang Date: Tue, 18 Mar 2025 14:29:46 +0000 Subject: [PATCH 4/4] skip setup signal notifier for detached container For detached container, we don't need to setup signal notifier, because there is no customer to consume the signals in `forward()`. Signed-off-by: lifubang --- signals.go | 15 ++------------- utils_linux.go | 27 ++++++++++++++++++--------- 2 files changed, 20 insertions(+), 22 deletions(-) diff --git a/signals.go b/signals.go index 15f16de756c..fc0033caa25 100644 --- a/signals.go +++ b/signals.go @@ -5,7 +5,6 @@ import ( "os/signal" "github.com/opencontainers/runc/libcontainer" - "github.com/opencontainers/runc/libcontainer/system" "github.com/opencontainers/runc/libcontainer/utils" "github.com/sirupsen/logrus" @@ -16,13 +15,7 @@ const signalBufferSize = 2048 // newSignalHandler returns a signal handler for processing SIGCHLD and SIGWINCH signals // while still forwarding all other signals to the process. -func newSignalHandler(enableSubreaper bool) chan *signalHandler { - if enableSubreaper { - // set us as the subreaper before registering the signal handler for the container - if err := system.SetSubreaper(1); err != nil { - logrus.Warn(err) - } - } +func newSignalHandler() chan *signalHandler { handler := make(chan *signalHandler) // signal.Notify is actually quite expensive, as it has to configure the // signal mask and add signal handlers for all signals (all ~65 of them). @@ -54,13 +47,9 @@ type signalHandler struct { // forward handles the main signal event loop forwarding, resizing, or reaping depending // on the signal received. -func (h *signalHandler) forward(process *libcontainer.Process, tty *tty, detach bool) (int, error) { +func (h *signalHandler) forward(process *libcontainer.Process, tty *tty) (int, error) { // make sure we know the pid of our main process so that we can return // after it dies. - if detach { - return 0, nil - } - pid1, err := process.Pid() if err != nil { return -1, err diff --git a/utils_linux.go b/utils_linux.go index 004ff404b12..2477408969b 100644 --- a/utils_linux.go +++ b/utils_linux.go @@ -19,6 +19,7 @@ import ( "github.com/opencontainers/runc/libcontainer" "github.com/opencontainers/runc/libcontainer/configs" "github.com/opencontainers/runc/libcontainer/specconv" + "github.com/opencontainers/runc/libcontainer/system" "github.com/opencontainers/runc/libcontainer/system/kernelversion" "github.com/opencontainers/runc/libcontainer/utils" ) @@ -219,8 +220,11 @@ type runner struct { } func (r *runner) run(config *specs.Process) (_ int, retErr error) { + detach := r.detach || (r.action == CT_ACT_CREATE) defer func() { - if retErr != nil { + // For a non-detached container, or we get an error, we + // should destroy the container. + if !detach || retErr != nil { r.destroy() } }() @@ -249,11 +253,19 @@ func (r *runner) run(config *specs.Process) (_ int, retErr error) { } process.ExtraFiles = append(process.ExtraFiles, os.NewFile(uintptr(i), "PreserveFD:"+strconv.Itoa(i))) } - detach := r.detach || (r.action == CT_ACT_CREATE) // Setting up IO is a two stage process. We need to modify process to deal // with detaching containers, and then we get a tty after the container has // started. - handlerCh := newSignalHandler(r.enableSubreaper) + if r.enableSubreaper { + // set us as the subreaper before registering the signal handler for the container + if err := system.SetSubreaper(1); err != nil { + logrus.Warn(err) + } + } + var handlerCh chan *signalHandler + if !detach { + handlerCh = newSignalHandler() + } tty, err := setupIO(process, r.container, config.Terminal, detach, r.consoleSocket) if err != nil { return -1, err @@ -301,15 +313,12 @@ func (r *runner) run(config *specs.Process) (_ int, retErr error) { return -1, err } } - handler := <-handlerCh - status, err := handler.forward(process, tty, detach) if detach { return 0, nil } - if err == nil { - r.destroy() - } - return status, err + // For non-detached container, we should forward signals to the container. + handler := <-handlerCh + return handler.forward(process, tty) } func (r *runner) destroy() {