rootless: make sure we only use a single pause process

Currently --tmpdir changes the location of the pause.pid file. this
causes issues because the c code in pkg/rootless does not know about
that. I tried to fix this[1] by fixing the c code to not use the
shortcut. While this fix worked it will result in many pause processes
leaking in the integrration tests.

Commit ab88632 added this behavior but following the disccusion it was
never the intention that we end up having more than one pause process.
The issues that was trying to fix was caused by somthing else AFAICT,
the main problem seems to be that the pause.pid file parent directory
may not be created when we try to create the pid file so it failed with
ENOENT. This patch fixes it by creating this directory always and revert
the change to no longer depend on the tmpdir value.

With this commit we now always use XDG_RUNTIME_DIR/libpod/tmp/pause.pid
for all podman processes. This allows the c shortcut to work reliably
and should therefore improve perfomance over my other approach.

A system test is added to ensure we see the right behavior and that
podman system migrate actually stops the pause process. Thanks to Ed
Santiago for the improved test to make it work for both `catatonit` and
`podman pause`.

This should fix the issues with namespace missmatches that we can see in
CI as flakes.

[1] containers#18057

Fixes containers#18057

Signed-off-by: Paul Holzinger <[email protected]>

libpod/runtime.go

@@ -579,10 +579,17 @@ func makeRuntime(runtime *Runtime) (retErr error) {
 			}
 			unLockFunc()
 			unLockFunc = nil
-			pausePid, err := util.GetRootlessPauseProcessPidPathGivenDir(runtime.config.Engine.TmpDir)
+			pausePid, err := util.GetRootlessPauseProcessPidPath()
 			if err != nil {
 				return fmt.Errorf("could not get pause process pid file path: %w", err)
 			}
+
+			// create the path in case it does not already exists
+			// https://github.com/containers/podman/issues/8539
+			if err := os.MkdirAll(filepath.Dir(pausePid), 0o700); err != nil {
+				return fmt.Errorf("could not create pause process pid file directory: %w", err)
+			}
+
 			became, ret, err := rootless.BecomeRootInUserNS(pausePid)
 			if err != nil {
 				return err

libpod/runtime_migrate.go

@@ -18,7 +18,7 @@ import (
 
 func (r *Runtime) stopPauseProcess() error {
 	if rootless.IsRootless() {
-		pausePidPath, err := util.GetRootlessPauseProcessPidPathGivenDir(r.config.Engine.TmpDir)
+		pausePidPath, err := util.GetRootlessPauseProcessPidPath()
 		if err != nil {
 			return fmt.Errorf("could not get pause process pid file path: %w", err)
 		}

pkg/domain/infra/abi/system.go

@@ -107,11 +107,7 @@ func (ic *ContainerEngine) SetupRootless(_ context.Context, noMoveProcess bool)
 		return nil
 	}
 
-	tmpDir, err := ic.Libpod.TmpDir()
-	if err != nil {
-		return err
-	}
-	pausePidPath, err := util.GetRootlessPauseProcessPidPathGivenDir(tmpDir)
+	pausePidPath, err := util.GetRootlessPauseProcessPidPath()
 	if err != nil {
 		return fmt.Errorf("could not get pause process pid file path: %w", err)
 	}

pkg/util/utils_supported.go

@@ -107,21 +107,13 @@ func GetRootlessConfigHomeDir() (string, error) {
 
 // GetRootlessPauseProcessPidPath returns the path to the file that holds the pid for
 // the pause process.
-// DEPRECATED - switch to GetRootlessPauseProcessPidPathGivenDir
 func GetRootlessPauseProcessPidPath() (string, error) {
 	runtimeDir, err := GetRuntimeDir()
 	if err != nil {
 		return "", err
 	}
-	return filepath.Join(runtimeDir, "libpod", "pause.pid"), nil
-}
-
-// GetRootlessPauseProcessPidPathGivenDir returns the path to the file that
-// holds the PID of the pause process, given the location of Libpod's temporary
-// files.
-func GetRootlessPauseProcessPidPathGivenDir(libpodTmpDir string) (string, error) {
-	if libpodTmpDir == "" {
-		return "", errors.New("must provide non-empty temporary directory")
-	}
-	return filepath.Join(libpodTmpDir, "pause.pid"), nil
+	// Note this path must be kept in sync with pkg/rootless/rootless_linux.go
+	// We only want a single pause process per user, so we do not want to use
+	// the tmpdir which can be changed via --tmpdir.
+	return filepath.Join(runtimeDir, "libpod", "tmp", "pause.pid"), nil
 }

pkg/util/utils_windows.go

@@ -30,12 +30,6 @@ func GetRootlessPauseProcessPidPath() (string, error) {
 	return "", fmt.Errorf("GetRootlessPauseProcessPidPath: %w", errNotImplemented)
 }
 
-// GetRootlessPauseProcessPidPath returns the path to the file that holds the pid for
-// the pause process
-func GetRootlessPauseProcessPidPathGivenDir(unused string) (string, error) {
-	return "", fmt.Errorf("GetRootlessPauseProcessPidPath: %w", errNotImplemented)
-}
-
 // GetRuntimeDir returns the runtime directory
 func GetRuntimeDir() (string, error) {
 	data, err := homedir.GetDataHome()

test/system/550-pause-process.bats

@@ -0,0 +1,79 @@
+#!/usr/bin/env bats   -*- bats -*-
+#
+# test to make sure we use the correct podman pause process
+#
+
+load helpers
+
+function _check_pause_process() {
+    pause_pid=
+    if [[ -z "$pause_pid_file" ]]; then
+        return
+    fi
+
+    test -e $pause_pid_file || die "Pause pid file $pause_pid_file missing"
+
+    # do not mark this variable as local; our parent expects it
+    pause_pid=$(<$pause_pid_file)
+    test -d /proc/$pause_pid || die "Pause process $pause_pid (from $pause_pid_file) is not running"
+
+    assert "$(</proc/$pause_pid/comm)" =~ 'catatonit|podman pause' \
+           "Pause process $pause_pid has an unexpected name"
+}
+
+# Test for https://github.com/containers/podman/issues/17903
+@test "rootless podman only ever uses single pause process" {
+    skip_if_not_rootless "pause process is only used as rootless"
+    skip_if_remote "--tmpdir not supported via remote"
+
+    # There are nasty bugs when we are not in the correct userns,
+    # we have good reproducer to see how things can go wrong here:
+    # https://github.com/containers/podman/issues/17903#issuecomment-1497232184
+
+    # To prevent any issues we should only ever have a single pause process running,
+    # regardless of any --root/-runroot/--tmpdir values.
+
+    # System tests can execute in contexts without XDG; in those, we have to
+    # skip the pause-pid-file checks.
+    local pause_pid_file
+    if [[ -n "$XDG_RUNTIME_DIR" ]]; then
+        pause_pid_file="$XDG_RUNTIME_DIR/libpod/tmp/pause.pid"
+    fi
+
+    # Baseline: get the current userns (one will be created on demand)
+    local getns="unshare readlink /proc/self/ns/user"
+    run_podman $getns
+    local baseline_userns="$output"
+
+    # A pause process will now be running
+    _check_pause_process
+
+    # Use podman system migrate to stop the currently running pause process
+    run_podman system migrate
+
+    # After migrate, there must be no pause process
+    if [[ -n "$pause_pid_file" ]]; then
+        test -e $pause_pid_file && die "Pause pid file $pause_pid_file still exists, even after podman system migrate"
+
+        run kill -0 $pause_pid
+        test $status -eq 0 && die "Pause process $pause_pid is still running even after podman system migrate"
+    fi
+
+    run_podman --root    $PODMAN_TMPDIR/root \
+               --runroot $PODMAN_TMPDIR/runroot \
+               --tmpdir  $PODMAN_TMPDIR/tmp \
+               $getns
+    tmpdir_userns="$output"
+
+    # And now we should once again have a pause process
+    _check_pause_process
+
+    # and all podmans, with & without --tmpdir, should use the same ns
+    run_podman $getns
+    assert "$output" == "$tmpdir_userns" \
+           "podman should use the same userns created using a tmpdir"
+
+    run_podman --tmpdir $PODMAN_TMPDIR/tmp2 $getns
+    assert "$output" == "$tmpdir_userns" \
+           "podman with tmpdir2 should use the same userns created using a tmpdir"
+}