install sigproxy before start/attach

Install the signal proxy before attaching to/starting the container to make sure there's no race-condition as revealed in the failing start/run tests in containers#16901. The tests had the valid expectation that signal forwarding works once the container is running. Further update the tests to account for the attach test where the expectation is that signal forwarding works once Podman has attached to container (or even before). Fixes: containers#16901 Signed-off-by: Valentin Rothberg <[email protected]> Signed-off-by: Ed Santiago <[email protected]>
vrothberg · Feb 13, 2023 · d2ac85f · d2ac85f
1 parent f099c1f
commit d2ac85f
Show file tree

Hide file tree

Showing 2 changed files with 32 additions and 14 deletions.
diff --git a/pkg/domain/infra/abi/terminal/terminal_common.go b/pkg/domain/infra/abi/terminal/terminal_common.go
@@ -84,11 +84,13 @@ func StartAttachCtr(ctx context.Context, ctr *libpod.Container, stdout, stderr,
 		streams.AttachInput = false
 	}
 
-	if !startContainer {
-		if sigProxy {
-			ProxySignals(ctr)
-		}
+	if sigProxy {
+		// To prevent a race condition, install the signal handler
+		// before starting/attaching to the container.
+		ProxySignals(ctr)
+	}
 
+	if !startContainer {
 		return ctr.Attach(streams, detachKeys, resize)
 	}
 
@@ -97,10 +99,6 @@ func StartAttachCtr(ctx context.Context, ctr *libpod.Container, stdout, stderr,
 		return err
 	}
 
-	if sigProxy {
-		ProxySignals(ctr)
-	}
-
 	if stdout == nil && stderr == nil {
 		fmt.Printf("%s\n", ctr.ID())
 	}

diff --git a/test/system/032-sig-proxy.bats b/test/system/032-sig-proxy.bats
@@ -3,7 +3,14 @@
 load helpers
 
 # Command to run in each of the tests.
-SLEEPLOOP='trap "echo BYE;exit 0" INT;echo READY;while :;do sleep 0.1;done'
+SLEEPLOOP='trap "echo BYE;exit 0" INT;echo READY;while :;do echo RUNNING;sleep 0.1;done'
+
+function setup() {
+    basic_setup
+
+    TESTLOG=$PODMAN_TMPDIR/container-stdout
+}
+
 
 # Main test code: wait for container to exist and be ready, send it a
 # signal, wait for container to acknowledge and exit.
@@ -26,8 +33,21 @@ function _test_sigproxy() {
           fi
     done
 
-    # Now that container exists, wait for it to declare itself READY
-    wait_for_ready $cname
+    # Now that container exists, wait for it to declare itself RUNNING
+    timeout=10
+    while :;do
+          sleep 0.5
+          if grep -q RUNNING $TESTLOG; then
+              break
+          fi
+          timeout=$((timeout - 1))
+          if [[ $timeout -eq 0 ]]; then
+              run_podman ps -a
+              echo "log from container:"
+              cat $TESTLOG
+              die "Timed out waiting for container $cname to start"
+          fi
+    done
 
     # Signal, and wait for container to exit
     kill -INT $kidpid
@@ -52,7 +72,7 @@ function _test_sigproxy() {
 
 @test "podman sigproxy test: run" {
     # We're forced to use $PODMAN because run_podman cannot be backgrounded
-    $PODMAN run -i --name c_run $IMAGE sh -c "$SLEEPLOOP" &
+    $PODMAN run -i --name c_run $IMAGE sh -c "$SLEEPLOOP" >$TESTLOG &
     local kidpid=$!
 
     _test_sigproxy c_run $kidpid
@@ -62,7 +82,7 @@ function _test_sigproxy() {
     run_podman create --name c_start $IMAGE sh -c "$SLEEPLOOP"
 
     # See above comments regarding $PODMAN and backgrounding
-    $PODMAN start --attach c_start &
+    $PODMAN start --attach c_start >$TESTLOG &
     local kidpid=$!
 
     _test_sigproxy c_start $kidpid
@@ -72,7 +92,7 @@ function _test_sigproxy() {
     run_podman run -d --name c_attach $IMAGE sh -c "$SLEEPLOOP"
 
     # See above comments regarding $PODMAN and backgrounding
-    $PODMAN attach c_attach &
+    $PODMAN attach c_attach >$TESTLOG &
     local kidpid=$!
 
     _test_sigproxy c_attach $kidpid