ForwardAllSignals: check if channel is closed, and remove warning

[thaJeztah]

Commit fff164c (#2929) 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 (when the channel was closed), causing spurious (but otherwise harmless) warnings to be printed;

Unsupported signal: <nil>. 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;

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 `<nil>` signal only happens "sometimes"):
./build/docker run --rm alpine/git clone https://github.com/docker/getting-started.git
Cloning into 'getting-started'...
Signal: <nil>
# 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: <nil>
Signal: <nil>

When running a macOS binary built with Go 1.15 (DISABLE_WARN_OUTSIDE_CONTAINER=1 make binary):

# regular "docker run" (note that the `<nil>` 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: <nil>
# 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: <nil>
Signal: <nil>

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)

As a follow-up, we should consider removing the "validation" step altogether, as it looks like the code performs client-side validation based on signals known by the client platform, which may not match the server platform

For example, there are various signals not defined on darwin (macOS); https://github.com/moby/moby/blob/v20.10.4/pkg/signal/signal_darwin.go#L7-L41 That are supported on Linux: https://github.com/moby/moby/blob/v20.10.4/pkg/signal/signal_linux.go#L17-L83

Given that for this particular use, translation from signal names to numbers is handled daemon-side, so no validation should be needed.

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":

cli/cli/command/container/signals.go

Line 55 in 35f023a

gosignal.Notify(sigc)

- How to verify it

- Description for the changelog

Fix spurious `Unsupported signal: <nil>. Discarding.` messages when running containers.

- A picture of a cute animal (not mandatory but encouraged)

[thaJeztah]

Updated description, because we actually do some conversion from signal (number) to signal (name) on the client side, so we can't remove the validation, but we should have a look at how to handle signals that are unknown to the client, but have to be sent over the API (by name)

[thaJeztah]

Related to my previous comment; looking at the daemon code; https://github.com/moby/moby/blob/8a4671fb1f89d222bd65cb07ea6a0de1ec7a1a2b/api/server/router/container/container_routes.go#L248-L254

// If we have a signal, look at it. Otherwise, do nothingifsigStr:=r.Form.Get("signal"); sigStr!="" {
varerrerrorifsig, err=signal.ParseSignal(sigStr); err!=nil {
returnerrdefs.InvalidParameter(err)
}
}

It looks like signal.ParseSignal() actually handles either named signals or numeric signals; https://github.com/moby/moby/blob/46cdcd206c56172b95ba5c77b827a722dab426c5/pkg/signal/signal.go#L32-L38

// ParseSignal translates a string to a valid syscall signal.// It returns an error if the signal map doesn't include the given signal.funcParseSignal(rawSignalstring) (syscall.Signal, error) {
s, err:=strconv.Atoi(rawSignal)
iferr==nil {
ifs==0 {
return-1, fmt.Errorf("Invalid signal: %s", rawSignal)
}
returnsyscall.Signal(s), nil
}
signal, ok:=SignalMap[strings.TrimPrefix(strings.ToUpper(rawSignal), "SIG")]
if!ok {
return-1, fmt.Errorf("Invalid signal: %s", rawSignal)
}
returnsignal, nil
}

Which means we could

• remove client-side validation
• skip conversion of signal-number to signal-name

The API will still return an error if it encounters an unknown signal (we could limit to numSig); https://github.com/golang/go/blob/d4b26382342c98a95b85140b2863bc30c48edd68/src/os/signal/signal_unix.go#L32-L47

[thaJeztah]

skip conversion of signal-number to signal-name

Hmmm.... so os.Signal is an interface, and while (affaics) all implementations use an integer for the underlying type, officially that's not guaranteed. Guess we could cast it to a syscall.Signal and/or unix.Signal and use that 🤔

[tiborvass]

Confirming that this no longer prints the message but I'm still not sure I understand what is going on.

[thaJeztah]

@tonistiigi@tiborvass updated; PTAL

[cpuguy83]

Oh dang, yes. Good catch. LGTM

[tonistiigi]

Need some analysis on what closes this channel. The fix is correct anyway but need to make sure there isn't something more as well.

[tiborvass]

pkg/signal.StopCatch closes the channel. StopCatch is called in a defer in attach.go, start.go, and run.go

[tiborvass]

That is the only function that closes the channel.

[thaJeztah]

Looks like there were other regressions introduced by #2929 / #2960); see moby/moby#42093

I opened a PR to revert the change in the 20.10 branch: #2987

Meanwhile we can continue looking into the problem on master

[tiborvass]

@thaJeztah do you know why #2929 changed signal.CatchAll to gosignal.Notify?

[silvin-lubecki]

LGTM