Merge pull request #3107 from thaJeztah/20.10_backport_sigurg_handling

[20.10 backport] Ignore SIGURG on Unix (including Darwin)

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)
 	}
 

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
+}

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)
 	}
 

cli/command/container/signals.go

@@ -0,0 +1,61 @@
+package container
+
+import (
+	"context"
+	"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
+		ok bool
+	)
+	for {
+		select {
+		case s, ok = <-sigc:
+			if !ok {
+				return
+			}
+		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 == "" {
+			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
+}

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")
+	}
+
+}

cli/command/container/signals_unix.go

@@ -0,0 +1,13 @@
+// +build !windows
+
+package container
+
+import (
+	"os"
+
+	"golang.org/x/sys/unix"
+)
+
+func isRuntimeSig(s os.Signal) bool {
+	return s == unix.SIGURG
+}

cli/command/container/signals_unix_test.go

@@ -0,0 +1,59 @@
+// +build !windows
+
+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")
+		})
+	}
+}

cli/command/container/signals_windows.go

@@ -0,0 +1,7 @@
+package container
+
+import "os"
+
+func isRuntimeSig(_ os.Signal) bool {
+	return false
+}

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()
+			go ForwardAllSignals(ctx, dockerCli, c.ID, sigc)
 			defer signal.StopCatch(sigc)
 		}
 

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
-}