signal parsing - better input validation

The helper function we use for signal name mapping does not
check for negative numbers nor invalid (too-high) ones. This
can yield unexpected error messages:

   # podman kill -s -1 foo
   ERRO[0000] unknown signal "18446744073709551615"

This PR introduces a small wrapper for it that:

  1) Strips off a leading dash, allowing '-1' or '-HUP'
     as valid inputs; and
  2) Rejects numbers <1 or >64 (SIGRTMAX)

Also adds a test suite checking signal handling as well as
ensuring that invalid signals are rejected by the command line.

Fixes: #4746

Signed-off-by: Ed Santiago <santiago@redhat.com>

cmd/podman/kill.go

@@ -3,7 +3,7 @@ package main
 import (
 	"github.com/containers/libpod/cmd/podman/cliconfig"
 	"github.com/containers/libpod/pkg/adapter"
-	"github.com/docker/docker/pkg/signal"
+	"github.com/containers/libpod/pkg/util"
 	"github.com/opentracing/opentracing-go"
 	"github.com/pkg/errors"
 	"github.com/spf13/cobra"
@@ -54,7 +54,7 @@ func killCmd(c *cliconfig.KillValues) error {
 
 	// Check if the signalString provided by the user is valid
 	// Invalid signals will return err
-	killSignal, err := signal.ParseSignal(c.Signal)
+	killSignal, err := util.ParseSignal(c.Signal)
 	if err != nil {
 		return err
 	}

cmd/podman/pod_kill.go

@@ -6,7 +6,7 @@ import (
 
 	"github.com/containers/libpod/cmd/podman/cliconfig"
 	"github.com/containers/libpod/pkg/adapter"
-	"github.com/docker/docker/pkg/signal"
+	"github.com/containers/libpod/pkg/util"
 	"github.com/pkg/errors"
 	"github.com/sirupsen/logrus"
 	"github.com/spf13/cobra"
@@ -60,7 +60,7 @@ func podKillCmd(c *cliconfig.PodKillValues) error {
 	if c.Signal != "" {
 		// Check if the signalString provided by the user is valid
 		// Invalid signals will return err
-		sysSignal, err := signal.ParseSignal(c.Signal)
+		sysSignal, err := util.ParseSignal(c.Signal)
 		if err != nil {
 			return err
 		}

cmd/podman/shared/create.go

@@ -24,7 +24,6 @@ import (
 	"github.com/containers/libpod/pkg/rootless"
 	cc "github.com/containers/libpod/pkg/spec"
 	"github.com/containers/libpod/pkg/util"
-	"github.com/docker/docker/pkg/signal"
 	"github.com/docker/go-connections/nat"
 	"github.com/docker/go-units"
 	"github.com/opentracing/opentracing-go"
@@ -464,7 +463,7 @@ func ParseCreateOpts(ctx context.Context, c *GenericCLIResults, runtime *libpod.
 		signalString = c.String("stop-signal")
 	}
 	if signalString != "" {
-		stopSignal, err = signal.ParseSignal(signalString)
+		stopSignal, err = util.ParseSignal(signalString)
 		if err != nil {
 			return nil, err
 		}
@@ -624,7 +623,7 @@ func ParseCreateOpts(ctx context.Context, c *GenericCLIResults, runtime *libpod.
 	}
 	if systemd {
 		if signalString == "" {
-			stopSignal, err = signal.ParseSignal("RTMIN+3")
+			stopSignal, err = util.ParseSignal("RTMIN+3")
 			if err != nil {
 				return nil, errors.Wrapf(err, "error parsing systemd signal")
 			}

pkg/util/utils.go

@@ -9,6 +9,7 @@ import (
 	"strconv"
 	"strings"
 	"sync"
+	"syscall"
 	"time"
 
 	"github.com/BurntSushi/toml"
@@ -284,9 +285,7 @@ func GetImageConfig(changes []string) (ImageConfig, error) {
 			config.Labels[key] = val
 		case "STOPSIGNAL":
 			// Check the provided signal for validity.
-			// TODO: Worth checking range? ParseSignal allows
+			killSignal, err := ParseSignal(value)
-			// negative numbers.
-			killSignal, err := signal.ParseSignal(value)
 			if err != nil {
 				return ImageConfig{}, errors.Wrapf(err, "invalid change %q - KILLSIGNAL must be given a valid signal", change)
 			}
@@ -305,6 +304,22 @@ func GetImageConfig(changes []string) (ImageConfig, error) {
 	return config, nil
 }
 
+// Parse and validate a signal name or number
+func ParseSignal(rawSignal string) (syscall.Signal, error) {
+	// Strip off leading dash, to allow -1 or -HUP
+	basename := strings.TrimPrefix(rawSignal, "-")
+
+	signal, err := signal.ParseSignal(basename)
+	if err != nil {
+		return -1, err
+	}
+	// 64 is SIGRTMAX; wish we could get this from a standard Go library
+	if signal < 1 || signal > 64 {
+		return -1, errors.Errorf("valid signals are 1 through 64")
+	}
+	return signal, nil
+}
+
 // ParseIDMapping takes idmappings and subuid and subgid maps and returns a storage mapping
 func ParseIDMapping(mode namespaces.UsernsMode, UIDMapSlice, GIDMapSlice []string, subUIDMap, subGIDMap string) (*storage.IDMappingOptions, error) {
 	options := storage.IDMappingOptions{

test/system/130-kill.bats

@@ -0,0 +1,92 @@
+#!/usr/bin/env bats   -*- bats -*-
+#
+# tests for podman kill
+#
+
+load helpers
+
+@test "podman kill - test signal handling in containers" {
+    # Start a container that will handle all signals by emitting 'got: N'
+    local -a signals=(1 2 3 4 5 6 8 10 12 13 14 15 16 20 21 22 23 24 25 26 64)
+    run_podman run -d $IMAGE sh -c "for i in ${signals[*]}; do trap \"echo got: \$i\" \$i; done; echo READY; while ! test -e /stop; do sleep 0.05; done;echo DONE"
+    cid="$output"
+
+    # Run 'logs -f' on that container, but run it in the background with
+    # redirection to a named pipe from which we (foreground job) read
+    # and confirm that signals are received. We can't use run_podman here.
+    local fifo=${PODMAN_TMPDIR}/podman-kill-fifo.$(random_string 10)
+    mkfifo $fifo
+    $PODMAN logs -f $cid >$fifo &
+    podman_log_pid=$!
+    # First container emits READY when ready; wait for it.
+    read -t 10 ready <$fifo
+    is "$ready" "READY" "first log message from container"
+
+    # Helper function: send the given signal, verify that it's received.
+    kill_and_check() {
+        local signal=$1
+        local signum=${2:-$1}       # e.g. if signal=HUP, we expect to see '1'
+
+        run_podman kill -s $signal $cid
+        read -t 10 actual <$fifo
+        is "$actual" "got: $signum" "Signal $signal handled by container"
+    }
+
+    # Send signals in random order; make sure each one is received
+    for s in $(fmt --width=2 <<< "${signals[*]}" | sort --random-sort);do
+        kill_and_check $s
+    done
+
+    # Variations: with leading dash; by name, with/without dash or SIG
+    kill_and_check -1        1
+    kill_and_check -INT      2
+    kill_and_check  FPE      8
+    kill_and_check -SIGUSR1 10
+    kill_and_check  SIGUSR2 12
+
+    # Done. Tell the container to stop, and wait for final DONE
+    run_podman exec $cid touch /stop
+    read -t 5 done <$fifo
+    is "$done" "DONE" "final log message from container"
+
+    # Clean up
+    run_podman wait $cid
+    run_podman rm $cid
+    wait $podman_log_pid
+}
+
+@test "podman kill - rejects invalid args" {
+    # These errors are thrown by the imported docker/signal.ParseSignal()
+    local -a bad_signal_names=(0 SIGBADSIG SIG BADSIG %% ! "''" '""' " ")
+    for s in ${bad_signal_names[@]}; do
+        # 'nosuchcontainer' is fine: podman should bail before it gets there
+        run_podman 125 kill -s $s nosuchcontainer
+        is "$output" "Error: Invalid signal: $s" "Error from kill -s $s"
+
+        run_podman 125 pod kill -s $s nosuchpod
+        is "$output" "Error: Invalid signal: $s" "Error from pod kill -s $s"
+    done
+
+    # Special case: these too are thrown by docker/signal.ParseSignal(),
+    # but the dash sign is stripped by our wrapper in utils, so the
+    # error message doesn't include the dash.
+    local -a bad_dash_signals=(-0 -SIGBADSIG -SIG -BADSIG -)
+    for s in ${bad_dash_signals[@]}; do
+        run_podman 125 kill -s $s nosuchcontainer
+        is "$output" "Error: Invalid signal: ${s##-}" "Error from kill -s $s"
+    done
+
+    # This error (signal out of range) is thrown by our wrapper
+    local -a bad_signal_nums=(65 -65 96 999 99999999)
+    for s in ${bad_signal_nums[@]}; do
+        run_podman 125 kill -s $s nosuchcontainer
+        is "$output" "Error: valid signals are 1 through 64" \
+           "Error from kill -s $s"
+    done
+
+    # 'podman create' uses the same parsing code
+    run_podman 125 create --stop-signal=99 $IMAGE
+    is "$output" "Error: valid signals are 1 through 64" "podman create"
+}
+
+# vim: filetype=sh