summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorEd Santiago <santiago@redhat.com>2019-12-25 11:51:06 -0700
committerEd Santiago <santiago@redhat.com>2019-12-26 16:50:21 -0700
commit40f55ca3fe06d2e5d0232c1f07911ea728fd1bc1 (patch)
tree53cbd52ac0e6afe56e29b00f24ec3a8217b30533
parentc759c3f78dcbbf5dec462a863ad25cd41a1707b7 (diff)
downloadpodman-40f55ca3fe06d2e5d0232c1f07911ea728fd1bc1.tar.gz
podman-40f55ca3fe06d2e5d0232c1f07911ea728fd1bc1.tar.bz2
podman-40f55ca3fe06d2e5d0232c1f07911ea728fd1bc1.zip
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>
-rw-r--r--cmd/podman/kill.go4
-rw-r--r--cmd/podman/pod_kill.go4
-rw-r--r--cmd/podman/shared/create.go5
-rw-r--r--pkg/util/utils.go21
-rw-r--r--test/system/130-kill.bats92
5 files changed, 116 insertions, 10 deletions
diff --git a/cmd/podman/kill.go b/cmd/podman/kill.go
index aba2008ca..a10546ea9 100644
--- a/cmd/podman/kill.go
+++ b/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
}
diff --git a/cmd/podman/pod_kill.go b/cmd/podman/pod_kill.go
index 064946f72..9f696073d 100644
--- a/cmd/podman/pod_kill.go
+++ b/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
}
diff --git a/cmd/podman/shared/create.go b/cmd/podman/shared/create.go
index c1c5db7cb..58cf56eea 100644
--- a/cmd/podman/shared/create.go
+++ b/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")
}
diff --git a/pkg/util/utils.go b/pkg/util/utils.go
index 5b4dfe9fa..f7d04c73b 100644
--- a/pkg/util/utils.go
+++ b/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
- // negative numbers.
- killSignal, err := signal.ParseSignal(value)
+ killSignal, err := 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{
diff --git a/test/system/130-kill.bats b/test/system/130-kill.bats
new file mode 100644
index 000000000..af3409b9b
--- /dev/null
+++ b/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