From 706ca7985bfb6a7360ef95581063712d04466c8d Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Tue, 25 May 2021 12:30:31 +0200 Subject: [PATCH 1/4] Revert "[20.10] Revert "Ignore SIGURG on Linux."" This reverts commit f33a69f6eef118195e741d158cfb8ee7a7391ddb. Signed-off-by: Sebastiaan van Stijn --- cli/command/container/attach.go | 3 +- cli/command/container/client_test.go | 8 +++ cli/command/container/run.go | 3 +- cli/command/container/signals.go | 57 +++++++++++++++++++++ cli/command/container/signals_linux.go | 11 ++++ cli/command/container/signals_linux_test.go | 57 +++++++++++++++++++++ cli/command/container/signals_notlinux.go | 9 ++++ cli/command/container/signals_test.go | 48 +++++++++++++++++ cli/command/container/start.go | 3 +- cli/command/container/tty.go | 29 ----------- 10 files changed, 196 insertions(+), 32 deletions(-) create mode 100644 cli/command/container/signals.go create mode 100644 cli/command/container/signals_linux.go create mode 100644 cli/command/container/signals_linux_test.go create mode 100644 cli/command/container/signals_notlinux.go create mode 100644 cli/command/container/signals_test.go diff --git a/cli/command/container/attach.go b/cli/command/container/attach.go index 854aa73952..8470855992 100644 --- a/cli/command/container/attach.go +++ b/cli/command/container/attach.go @@ -97,7 +97,8 @@ func runAttach(dockerCli command.Cli, opts *attachOptions) error { } if opts.proxy && !c.Config.Tty { - sigc := ForwardAllSignals(ctx, dockerCli, opts.container) + sigc := notfiyAllSignals() + go ForwardAllSignals(ctx, dockerCli, opts.container, sigc) defer signal.StopCatch(sigc) } diff --git a/cli/command/container/client_test.go b/cli/command/container/client_test.go index 3643550b92..6aa7d9991e 100644 --- a/cli/command/container/client_test.go +++ b/cli/command/container/client_test.go @@ -32,6 +32,7 @@ type fakeClient struct { containerExportFunc func(string) (io.ReadCloser, error) containerExecResizeFunc func(id string, options types.ResizeOptions) error containerRemoveFunc func(ctx context.Context, container string, options types.ContainerRemoveOptions) error + containerKillFunc func(ctx context.Context, container, signal string) error Version string } @@ -154,3 +155,10 @@ func (f *fakeClient) ContainerExecResize(_ context.Context, id string, options t } return nil } + +func (f *fakeClient) ContainerKill(ctx context.Context, container, signal string) error { + if f.containerKillFunc != nil { + return f.containerKillFunc(ctx, container, signal) + } + return nil +} diff --git a/cli/command/container/run.go b/cli/command/container/run.go index 74bee8cd12..fec9995841 100644 --- a/cli/command/container/run.go +++ b/cli/command/container/run.go @@ -131,7 +131,8 @@ func runContainer(dockerCli command.Cli, opts *runOptions, copts *containerOptio return runStartContainerErr(err) } if opts.sigProxy { - sigc := ForwardAllSignals(ctx, dockerCli, createResponse.ID) + sigc := notfiyAllSignals() + go ForwardAllSignals(ctx, dockerCli, createResponse.ID, sigc) defer signal.StopCatch(sigc) } diff --git a/cli/command/container/signals.go b/cli/command/container/signals.go new file mode 100644 index 0000000000..06e4d9eb66 --- /dev/null +++ b/cli/command/container/signals.go @@ -0,0 +1,57 @@ +package container + +import ( + "context" + "fmt" + "os" + gosignal "os/signal" + + "github.com/docker/cli/cli/command" + "github.com/docker/docker/pkg/signal" + "github.com/sirupsen/logrus" +) + +// ForwardAllSignals forwards signals to the container +// +// The channel you pass in must already be setup to receive any signals you want to forward. +func ForwardAllSignals(ctx context.Context, cli command.Cli, cid string, sigc <-chan os.Signal) { + var s os.Signal + for { + select { + case s = <-sigc: + case <-ctx.Done(): + return + } + + if s == signal.SIGCHLD || s == signal.SIGPIPE { + continue + } + + // In go1.14+, the go runtime issues SIGURG as an interrupt to support pre-emptable system calls on Linux. + // Since we can't forward that along we'll check that here. + if isRuntimeSig(s) { + continue + } + var sig string + for sigStr, sigN := range signal.SignalMap { + if sigN == s { + sig = sigStr + break + } + } + if sig == "" { + fmt.Fprintf(cli.Err(), "Unsupported signal: %v. Discarding.\n", s) + continue + } + + if err := cli.Client().ContainerKill(ctx, cid, sig); err != nil { + logrus.Debugf("Error sending signal: %s", err) + } + } +} + +func notfiyAllSignals() chan os.Signal { + sigc := make(chan os.Signal, 128) + gosignal.Notify(sigc) + return sigc +} diff --git a/cli/command/container/signals_linux.go b/cli/command/container/signals_linux.go new file mode 100644 index 0000000000..7eeb919850 --- /dev/null +++ b/cli/command/container/signals_linux.go @@ -0,0 +1,11 @@ +package container + +import ( + "os" + + "golang.org/x/sys/unix" +) + +func isRuntimeSig(s os.Signal) bool { + return s == unix.SIGURG +} diff --git a/cli/command/container/signals_linux_test.go b/cli/command/container/signals_linux_test.go new file mode 100644 index 0000000000..1b2eaff9b3 --- /dev/null +++ b/cli/command/container/signals_linux_test.go @@ -0,0 +1,57 @@ +package container + +import ( + "context" + "os" + "syscall" + "testing" + "time" + + "github.com/docker/cli/internal/test" + "golang.org/x/sys/unix" + "gotest.tools/v3/assert" +) + +func TestIgnoredSignals(t *testing.T) { + ignoredSignals := []syscall.Signal{unix.SIGPIPE, unix.SIGCHLD, unix.SIGURG} + + for _, s := range ignoredSignals { + t.Run(unix.SignalName(s), func(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + var called bool + client := &fakeClient{containerKillFunc: func(ctx context.Context, container, signal string) error { + called = true + return nil + }} + + cli := test.NewFakeCli(client) + sigc := make(chan os.Signal) + defer close(sigc) + + done := make(chan struct{}) + go func() { + ForwardAllSignals(ctx, cli, t.Name(), sigc) + close(done) + }() + + timer := time.NewTimer(30 * time.Second) + defer timer.Stop() + + select { + case <-timer.C: + t.Fatal("timeout waiting to send signal") + case sigc <- s: + case <-done: + } + + // cancel the context so ForwardAllSignals will exit after it has processed the signal we sent. + // This is how we know the signal was actually processed and are not introducing a flakey test. + cancel() + <-done + + assert.Assert(t, !called, "kill was called") + }) + } +} diff --git a/cli/command/container/signals_notlinux.go b/cli/command/container/signals_notlinux.go new file mode 100644 index 0000000000..9e8412f66a --- /dev/null +++ b/cli/command/container/signals_notlinux.go @@ -0,0 +1,9 @@ +// +build !linux + +package container + +import "os" + +func isRuntimeSig(_ os.Signal) bool { + return false +} diff --git a/cli/command/container/signals_test.go b/cli/command/container/signals_test.go new file mode 100644 index 0000000000..39eaab1c5d --- /dev/null +++ b/cli/command/container/signals_test.go @@ -0,0 +1,48 @@ +package container + +import ( + "context" + "os" + "testing" + "time" + + "github.com/docker/cli/internal/test" + "github.com/docker/docker/pkg/signal" +) + +func TestForwardSignals(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + called := make(chan struct{}) + client := &fakeClient{containerKillFunc: func(ctx context.Context, container, signal string) error { + close(called) + return nil + }} + + cli := test.NewFakeCli(client) + sigc := make(chan os.Signal) + defer close(sigc) + + go ForwardAllSignals(ctx, cli, t.Name(), sigc) + + timer := time.NewTimer(30 * time.Second) + defer timer.Stop() + + select { + case <-timer.C: + t.Fatal("timeout waiting to send signal") + case sigc <- signal.SignalMap["TERM"]: + } + if !timer.Stop() { + <-timer.C + } + timer.Reset(30 * time.Second) + + select { + case <-called: + case <-timer.C: + t.Fatal("timeout waiting for signal to be processed") + } + +} diff --git a/cli/command/container/start.go b/cli/command/container/start.go index 5ab05ae68b..9029061e09 100644 --- a/cli/command/container/start.go +++ b/cli/command/container/start.go @@ -74,7 +74,8 @@ func runStart(dockerCli command.Cli, opts *startOptions) error { // We always use c.ID instead of container to maintain consistency during `docker start` if !c.Config.Tty { - sigc := ForwardAllSignals(ctx, dockerCli, c.ID) + sigc := notfiyAllSignals() + ForwardAllSignals(ctx, dockerCli, c.ID, sigc) defer signal.StopCatch(sigc) } diff --git a/cli/command/container/tty.go b/cli/command/container/tty.go index b7003f1a04..4060c0a19f 100644 --- a/cli/command/container/tty.go +++ b/cli/command/container/tty.go @@ -95,32 +95,3 @@ func MonitorTtySize(ctx context.Context, cli command.Cli, id string, isExec bool } return nil } - -// ForwardAllSignals forwards signals to the container -func ForwardAllSignals(ctx context.Context, cli command.Cli, cid string) chan os.Signal { - sigc := make(chan os.Signal, 128) - signal.CatchAll(sigc) - go func() { - for s := range sigc { - if s == signal.SIGCHLD || s == signal.SIGPIPE { - continue - } - var sig string - for sigStr, sigN := range signal.SignalMap { - if sigN == s { - sig = sigStr - break - } - } - if sig == "" { - fmt.Fprintf(cli.Err(), "Unsupported signal: %v. Discarding.\n", s) - continue - } - - if err := cli.Client().ContainerKill(ctx, cid, sig); err != nil { - logrus.Debugf("Error sending signal: %s", err) - } - } - }() - return sigc -} From 88de81ff21f673771a720b6ad272f033fe1cac95 Mon Sep 17 00:00:00 2001 From: Brian Goff Date: Tue, 2 Mar 2021 00:54:13 +0000 Subject: [PATCH 2/4] Fix `docker start` blocking on signal handling We refactorted `ForwardAllSignals` so it blocks but did not update the call in `start` to call it in a goroutine. Signed-off-by: Brian Goff (cherry picked from commit e1a7517514fbde432f4316f64959fbcff5036a1a) Signed-off-by: Sebastiaan van Stijn --- cli/command/container/start.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cli/command/container/start.go b/cli/command/container/start.go index 9029061e09..bec948225f 100644 --- a/cli/command/container/start.go +++ b/cli/command/container/start.go @@ -75,7 +75,7 @@ func runStart(dockerCli command.Cli, opts *startOptions) error { // We always use c.ID instead of container to maintain consistency during `docker start` if !c.Config.Tty { sigc := notfiyAllSignals() - ForwardAllSignals(ctx, dockerCli, c.ID, sigc) + go ForwardAllSignals(ctx, dockerCli, c.ID, sigc) defer signal.StopCatch(sigc) } From 032e485e1cb4aff44847c5dba12af3d7382bff6b Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Mon, 1 Mar 2021 14:06:19 +0100 Subject: [PATCH 3/4] ForwardAllSignals: check if channel is closed, and remove warning Commit fff164c22e8dc904291fecb62307312fd4ca153e modified ForwardAllSignals to take `SIGURG` signals into account, which can be generated by the Go runtime on Go 1.14 and up as an interrupt to support pre-emptable system calls on Linux. With the updated code, the signal (`s`) would sometimes be `nil`, causing spurious (but otherwise harmless) warnings to be printed; Unsupported signal: . Discarding. To debug this issue, I patched v20.10.4 to handle `nil`, and added a debug line to print the signal in all cases; ```patch diff --git a/cli/command/container/signals.go b/cli/command/container/signals.go index 06e4d9eb6..0cb53ef06 100644 --- a/cli/command/container/signals.go +++ b/cli/command/container/signals.go @@ -22,8 +22,9 @@ func ForwardAllSignals(ctx context.Context, cli command.Cli, cid string, sigc <- case <-ctx.Done(): return } + fmt.Fprintf(cli.Err(), "Signal: %v\n", s) if s == signal.SIGCHLD || s == signal.SIGPIPE { ``` When running a cross-compiled macOS binary with Go 1.13 (`make -f docker.Makefile binary-osx`): # regular "docker run" (note that the `` signal only happens "sometimes"): ./build/docker run --rm alpine/git clone https://github.com/docker/getting-started.git Cloning into 'getting-started'... Signal: # when cancelling with CTRL-C: ./build/docker run --rm alpine/git clone https://github.com/docker/getting-started.git ^CSignal: interrupt Cloning into 'getting-started'... error: could not lock config file /git/getting-started/.git/config: No such file or directory fatal: could not set 'core.repositoryformatversion' to '0' Signal: Signal: When running a macOS binary built with Go 1.15 (`DISABLE_WARN_OUTSIDE_CONTAINER=1 make binary`): # regular "docker run" (note that the `` signal only happens "sometimes"): # this is the same as on Go 1.13 ./build/docker run --rm alpine/git clone https://github.com/docker/getting-started.git Cloning into 'getting-started'... Signal: # when cancelling with CTRL-C: ./build/docker run --rm alpine/git clone https://github.com/docker/getting-started.git Cloning into 'getting-started'... ^CSignal: interrupt Signal: urgent I/O condition Signal: urgent I/O condition fatal: --stdin requires a git repository fatal: index-pack failed Signal: Signal: This patch checks if the channel is closed, and removes the warning (to prevent warnings if new signals are added that are not in our known list of signals) We should also consider updating `notfiyAllSignals()`, which currently forwards _all_ signals (`signal.Notify(sigc)` without passing a list of signals), and instead pass it "all signals _minus_ the signals we don't want forwarded": https://github.com/docker/cli/blob/35f023a7c22a51867fb099d29006ef27379bc7fe/cli/command/container/signals.go#L55 Signed-off-by: Sebastiaan van Stijn (cherry picked from commit 9342ec6b71404ad1dde2477bdc06febcaf49f47c) Signed-off-by: Sebastiaan van Stijn --- cli/command/container/signals.go | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/cli/command/container/signals.go b/cli/command/container/signals.go index 06e4d9eb66..1929a9b191 100644 --- a/cli/command/container/signals.go +++ b/cli/command/container/signals.go @@ -2,7 +2,6 @@ package container import ( "context" - "fmt" "os" gosignal "os/signal" @@ -15,10 +14,16 @@ import ( // // The channel you pass in must already be setup to receive any signals you want to forward. func ForwardAllSignals(ctx context.Context, cli command.Cli, cid string, sigc <-chan os.Signal) { - var s os.Signal + var ( + s os.Signal + ok bool + ) for { select { - case s = <-sigc: + case s, ok = <-sigc: + if !ok { + return + } case <-ctx.Done(): return } @@ -40,7 +45,6 @@ func ForwardAllSignals(ctx context.Context, cli command.Cli, cid string, sigc <- } } if sig == "" { - fmt.Fprintf(cli.Err(), "Unsupported signal: %v. Discarding.\n", s) continue } From 2945ba4f7a710902ed1729bc73715d4e5bca6726 Mon Sep 17 00:00:00 2001 From: David Scott Date: Fri, 21 May 2021 16:17:57 +0100 Subject: [PATCH 4/4] Ignore SIGURG on Darwin too This extends #2929 to Darwin as well as Linux. Running the example in https://github.com/golang/go/issues/37942 I see lots of: ``` dave@m1 sigurg % uname -ms Darwin arm64 dave@m1 sigurg % go run main.go received urgent I/O condition: 2021-05-21 16:03:03.482211 +0100 BST m=+0.014553751 received urgent I/O condition: 2021-05-21 16:03:03.507171 +0100 BST m=+0.039514459 ``` Signed-off-by: David Scott (cherry picked from commit cedaf44ea2e8b25b5298bac5f432d90033e5ad6a) Signed-off-by: Sebastiaan van Stijn --- cli/command/container/{signals_linux.go => signals_unix.go} | 2 ++ .../container/{signals_linux_test.go => signals_unix_test.go} | 2 ++ .../container/{signals_notlinux.go => signals_windows.go} | 2 -- 3 files changed, 4 insertions(+), 2 deletions(-) rename cli/command/container/{signals_linux.go => signals_unix.go} (86%) rename cli/command/container/{signals_linux_test.go => signals_unix_test.go} (98%) rename cli/command/container/{signals_notlinux.go => signals_windows.go} (82%) diff --git a/cli/command/container/signals_linux.go b/cli/command/container/signals_unix.go similarity index 86% rename from cli/command/container/signals_linux.go rename to cli/command/container/signals_unix.go index 7eeb919850..8db4cfe835 100644 --- a/cli/command/container/signals_linux.go +++ b/cli/command/container/signals_unix.go @@ -1,3 +1,5 @@ +// +build !windows + package container import ( diff --git a/cli/command/container/signals_linux_test.go b/cli/command/container/signals_unix_test.go similarity index 98% rename from cli/command/container/signals_linux_test.go rename to cli/command/container/signals_unix_test.go index 1b2eaff9b3..61ffe64a1c 100644 --- a/cli/command/container/signals_linux_test.go +++ b/cli/command/container/signals_unix_test.go @@ -1,3 +1,5 @@ +// +build !windows + package container import ( diff --git a/cli/command/container/signals_notlinux.go b/cli/command/container/signals_windows.go similarity index 82% rename from cli/command/container/signals_notlinux.go rename to cli/command/container/signals_windows.go index 9e8412f66a..1e51b952fa 100644 --- a/cli/command/container/signals_notlinux.go +++ b/cli/command/container/signals_windows.go @@ -1,5 +1,3 @@ -// +build !linux - package container import "os"