make image listing more resilient

Handle more TOCTOUs operating on listed images.  Also pull in
containers/common/pull/1520 and containers/common/pull/1522 which do the
same on the internal layer tree.

Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=2216700
Signed-off-by: Valentin Rothberg <vrothberg@redhat.com>

go.mod

@@ -13,7 +13,7 @@ require (
 	github.com/containernetworking/cni v1.1.2
 	github.com/containernetworking/plugins v1.3.0
 	github.com/containers/buildah v1.30.1-0.20230504052500-e925b5852e07
-	github.com/containers/common v0.53.1-0.20230621174116-586a3be4e1fc
+	github.com/containers/common v0.53.1-0.20230626115555-370c89881624
 	github.com/containers/conmon v2.0.20+incompatible
 	github.com/containers/image/v5 v5.25.1-0.20230613183705-07ced6137083
 	github.com/containers/libhvee v0.0.5

go.sum

@@ -239,8 +239,8 @@ github.com/containernetworking/plugins v1.3.0 h1:QVNXMT6XloyMUoO2wUOqWTC1hWFV62Q
 github.com/containernetworking/plugins v1.3.0/go.mod h1:Pc2wcedTQQCVuROOOaLBPPxrEXqqXBFt3cZ+/yVg6l0=
 github.com/containers/buildah v1.30.1-0.20230504052500-e925b5852e07 h1:Bs2sNFh/fSYr4J6JJLFqzyn3dp6HhlA6ewFwRYUpeIE=
 github.com/containers/buildah v1.30.1-0.20230504052500-e925b5852e07/go.mod h1:6A/BK0YJLXL8+AqlbceKJrhUT+NtEgsvAc51F7TAllc=
-github.com/containers/common v0.53.1-0.20230621174116-586a3be4e1fc h1:6yxDNgJGrddAWKeeAH7m0GUzCFRuvc2BqXund52Ui7k=
+github.com/containers/common v0.53.1-0.20230626115555-370c89881624 h1:YBgjfoo0G3tR8vm225ghJnqOZOVv3tH1L1GbyRM9320=
-github.com/containers/common v0.53.1-0.20230621174116-586a3be4e1fc/go.mod h1:qE1MzGl69IoK7ZNCCH51+aLVjyQtnH0LiZe0wG32Jy0=
+github.com/containers/common v0.53.1-0.20230626115555-370c89881624/go.mod h1:qE1MzGl69IoK7ZNCCH51+aLVjyQtnH0LiZe0wG32Jy0=
 github.com/containers/conmon v2.0.20+incompatible h1:YbCVSFSCqFjjVwHTPINGdMX1F6JXHGTUje2ZYobNrkg=
 github.com/containers/conmon v2.0.20+incompatible/go.mod h1:hgwZ2mtuDrppv78a/cOBNiCm6O0UMWGx1mu7P00nu5I=
 github.com/containers/image/v5 v5.25.1-0.20230613183705-07ced6137083 h1:6Pbnll97ls6G0U3DSxaTqp7Sd8Fykc4gd7BUJm7Bpn8=

libpod/container_internal.go

@@ -2094,7 +2094,7 @@ func (c *Container) postDeleteHooks(ctx context.Context) error {
 				hook := hook
 				logrus.Debugf("container %s: invoke poststop hook %d, path %s", c.ID(), i, hook.Path)
 				var stderr, stdout bytes.Buffer
-				hookErr, err := exec.Run(ctx, &hook, state, &stdout, &stderr, exec.DefaultPostKillTimeout)
+				hookErr, err := exec.Run(ctx, &hook, state, &stdout, &stderr, exec.DefaultPostKillTimeout) //nolint:staticcheck
 				if err != nil {
 					logrus.Warnf("Container %s: poststop hook %d: %v", c.ID(), i, err)
 					if hookErr != err {
@@ -2223,7 +2223,7 @@ func (c *Container) setupOCIHooks(ctx context.Context, config *spec.Spec) (map[s
 		}
 	}
 
-	hookErr, err := exec.RuntimeConfigFilter(ctx, allHooks["precreate"], config, exec.DefaultPostKillTimeout)
+	hookErr, err := exec.RuntimeConfigFilter(ctx, allHooks["precreate"], config, exec.DefaultPostKillTimeout) //nolint:staticcheck
 	if err != nil {
 		logrus.Warnf("Container %s: precreate hook: %v", c.ID(), err)
 		if hookErr != nil && hookErr != err {

pkg/domain/infra/abi/images_list.go

@@ -28,13 +28,14 @@ func (ir *ImageEngine) List(ctx context.Context, opts entities.ImageListOptions)
 
 	summaries := []*entities.ImageSummary{}
 	for _, img := range images {
+		summary, err := func() (*entities.ImageSummary, error) {
 			repoDigests, err := img.RepoDigests()
 			if err != nil {
 				return nil, fmt.Errorf("getting repoDigests from image %q: %w", img.ID(), err)
 			}
 
 			if img.ListData.IsDangling == nil { // Sanity check
-			return nil, fmt.Errorf("%w: ListData.IsDangling is nil but should not", define.ErrInternal)
+				return nil, fmt.Errorf("%w: ListData.IsDangling is nil but should not be", define.ErrInternal)
 			}
 			isDangling := *img.ListData.IsDangling
 			parentID := ""
@@ -42,7 +43,7 @@ func (ir *ImageEngine) List(ctx context.Context, opts entities.ImageListOptions)
 				parentID = img.ListData.Parent.ID()
 			}
 
-		e := entities.ImageSummary{
+			s := &entities.ImageSummary{
 				ID:          img.ID(),
 				Created:     img.Created().Unix(),
 				Dangling:    isDangling,
@@ -55,7 +56,7 @@ func (ir *ImageEngine) List(ctx context.Context, opts entities.ImageListOptions)
 				RepoTags:    img.Names(), // may include tags and digests
 				ParentId:    parentID,
 			}
-		e.Labels, err = img.Labels(ctx)
+			s.Labels, err = img.Labels(ctx)
 			if err != nil {
 				return nil, fmt.Errorf("retrieving label for image %q: you may need to remove the image to resolve the error: %w", img.ID(), err)
 			}
@@ -64,18 +65,26 @@ func (ir *ImageEngine) List(ctx context.Context, opts entities.ImageListOptions)
 			if err != nil {
 				return nil, fmt.Errorf("retrieving containers for image %q: you may need to remove the image to resolve the error: %w", img.ID(), err)
 			}
-		e.Containers = len(ctnrs)
+			s.Containers = len(ctnrs)
 
 			sz, err := img.Size()
 			if err != nil {
 				return nil, fmt.Errorf("retrieving size of image %q: you may need to remove the image to resolve the error: %w", img.ID(), err)
 			}
-		e.Size = sz
+			s.Size = sz
 			// This is good enough for now, but has to be
 			// replaced later with correct calculation logic
-		e.VirtualSize = sz
+			s.VirtualSize = sz
-
+			return s, nil
-		summaries = append(summaries, &e)
+		}()
+		if err != nil {
+			if libimage.ErrorIsImageUnknown(err) {
+				// The image may have been (partially) removed in the meantime
+				continue
+			}
+			return nil, err
+		}
+		summaries = append(summaries, summary)
 	}
 	return summaries, nil
 }

test/system/010-images.bats

@@ -359,4 +359,40 @@ EOF
     run_podman --root $imstore/root rmi --all
 }
 
+@test "podman images with concurrent removal" {
+    skip_if_remote "following test is not supported for remote clients"
+    local count=5
+
+    # First build $count images
+    for i in $(seq --format '%02g' 1 $count); do
+        cat >$PODMAN_TMPDIR/Containerfile <<EOF
+FROM $IMAGE
+RUN echo $i
+EOF
+        run_podman build -q -t i$i $PODMAN_TMPDIR
+    done
+
+    run_podman images
+    # Now remove all images in parallel and in the background and make sure
+    # that listing all images does not fail (see BZ 2216700).
+    for i in $(seq --format '%02g' 1 $count); do
+        timeout --foreground -v --kill=10 60 \
+                $PODMAN rmi i$i &
+    done
+
+    tries=100
+    while [[ ${#lines[*]} -gt 1 ]] && [[ $tries -gt 0 ]]; do
+        # Prior to #18980, 'podman images' during rmi could fail with 'image not known'
+        run_podman images --format "{{.ID}} {{.Names}}"
+        tries=$((tries - 1))
+    done
+
+    if [[ $tries -eq 0 ]]; then
+        die "Timed out waiting for images to be removed"
+    fi
+
+    wait
+}
+
+
 # vim: filetype=sh

vendor/github.com/containers/common/libimage/image.go

@@ -402,7 +402,7 @@ func (i *Image) removeRecursive(ctx context.Context, rmMap map[string]*RemoveIma
 	// have a closer look at the errors.  On top, image removal should be
 	// tolerant toward corrupted images.
 	handleError := func(err error) error {
-		if errors.Is(err, storage.ErrImageUnknown) || errors.Is(err, storage.ErrNotAnImage) || errors.Is(err, storage.ErrLayerUnknown) {
+		if ErrorIsImageUnknown(err) {
 			// The image or layers of the image may already have been removed
 			// in which case we consider the image to be removed.
 			return nil

vendor/github.com/containers/common/libimage/layer_tree.go

@@ -2,8 +2,10 @@ package libimage
 
 import (
 	"context"
+	"errors"
 
 	"github.com/containers/storage"
+	storageTypes "github.com/containers/storage/types"
 	ociv1 "github.com/opencontainers/image-spec/specs-go/v1"
 	"github.com/sirupsen/logrus"
 )
@@ -30,7 +32,19 @@ func (t *layerTree) node(layerID string) *layerNode {
 	return node
 }
 
+// ErrorIsImageUnknown returns true if the specified error indicates that an
+// image is unknown or has been partially removed (e.g., a missing layer).
+func ErrorIsImageUnknown(err error) bool {
+	return errors.Is(err, storage.ErrImageUnknown) ||
+		errors.Is(err, storageTypes.ErrLayerUnknown) ||
+		errors.Is(err, storageTypes.ErrSizeUnknown) ||
+		errors.Is(err, storage.ErrNotAnImage)
+}
+
 // toOCI returns an OCI image for the specified image.
+//
+// WARNING: callers are responsible for handling cases where the target image
+// has been (partially) removed and can use `ErrorIsImageUnknown` to detect it.
 func (t *layerTree) toOCI(ctx context.Context, i *Image) (*ociv1.Image, error) {
 	var err error
 	oci, exists := t.ociCache[i.ID()]
@@ -155,6 +169,9 @@ func (t *layerTree) children(ctx context.Context, parent *Image, all bool) ([]*I
 	parentID := parent.ID()
 	parentOCI, err := t.toOCI(ctx, parent)
 	if err != nil {
+		if ErrorIsImageUnknown(err) {
+			return nil, nil
+		}
 		return nil, err
 	}
 
@@ -165,6 +182,9 @@ func (t *layerTree) children(ctx context.Context, parent *Image, all bool) ([]*I
 		}
 		childOCI, err := t.toOCI(ctx, child)
 		if err != nil {
+			if ErrorIsImageUnknown(err) {
+				return false, nil
+			}
 			return false, err
 		}
 		// History check.
@@ -255,6 +275,9 @@ func (t *layerTree) parent(ctx context.Context, child *Image) (*Image, error) {
 	childID := child.ID()
 	childOCI, err := t.toOCI(ctx, child)
 	if err != nil {
+		if ErrorIsImageUnknown(err) {
+			return nil, nil
+		}
 		return nil, err
 	}
 
@@ -268,6 +291,9 @@ func (t *layerTree) parent(ctx context.Context, child *Image) (*Image, error) {
 			}
 			emptyOCI, err := t.toOCI(ctx, empty)
 			if err != nil {
+				if ErrorIsImageUnknown(err) {
+					return nil, nil
+				}
 				return nil, err
 			}
 			// History check.
@@ -300,6 +326,9 @@ func (t *layerTree) parent(ctx context.Context, child *Image) (*Image, error) {
 		}
 		parentOCI, err := t.toOCI(ctx, parent)
 		if err != nil {
+			if ErrorIsImageUnknown(err) {
+				return nil, nil
+			}
 			return nil, err
 		}
 		// History check.

vendor/github.com/containers/common/libnetwork/slirp4netns/slirp4netns.go

@@ -69,8 +69,6 @@ type SetupOptions struct {
 	ContainerID string
 	// Netns path to the netns
 	Netns string
-	// ContainerPID is the pid of container process
-	ContainerPID int
 	// Ports the should be forwarded
 	Ports []types.PortMapping
 	// ExtraOptions for slirp4netns that were set on the cli
@@ -84,6 +82,9 @@ type SetupOptions struct {
 	// RootlessPortSyncPipe pipe used to exit the rootlessport process.
 	// Same as Slirp4netnsExitPipeR, except this is only used when ports are given.
 	RootlessPortExitPipeR *os.File
+	// Pdeathsig is the signal which is send to slirp4netns process if the calling thread
+	// exits. The caller is responsible for locking the thread with runtime.LockOSThread().
+	Pdeathsig syscall.Signal
 }
 
 // SetupResult return type from Setup()
@@ -310,6 +311,7 @@ func Setup(opts *SetupOptions) (*SetupResult, error) {
 	logrus.Debugf("slirp4netns command: %s", strings.Join(cmd.Args, " "))
 	cmd.SysProcAttr = &syscall.SysProcAttr{
 		Setpgid:   true,
+		Pdeathsig: opts.Pdeathsig,
 	}
 
 	// workaround for https://github.com/rootless-containers/slirp4netns/pull/153

vendor/github.com/containers/common/pkg/hooks/exec/exec.go

@@ -16,16 +16,50 @@ import (
 // DefaultPostKillTimeout is the recommended default post-kill timeout.
 const DefaultPostKillTimeout = time.Duration(10) * time.Second
 
+type RunOptions struct {
+	// The hook to run
+	Hook *rspec.Hook
+	// The workdir to change when invoking the hook
+	Dir string
+	// The container state data to pass into the hook process
+	State []byte
+	// Stdout from the hook process
+	Stdout io.Writer
+	// Stderr from the hook process
+	Stderr io.Writer
+	// Timeout for waiting process killed
+	PostKillTimeout time.Duration
+}
+
 // Run executes the hook and waits for it to complete or for the
 // context or hook-specified timeout to expire.
+//
+// Deprecated: Too many arguments, has been refactored and replaced by RunWithOptions instead
 func Run(ctx context.Context, hook *rspec.Hook, state []byte, stdout io.Writer, stderr io.Writer, postKillTimeout time.Duration) (hookErr, err error) {
+	return RunWithOptions(
+		ctx,
+		RunOptions{
+			Hook:            hook,
+			State:           state,
+			Stdout:          stdout,
+			Stderr:          stderr,
+			PostKillTimeout: postKillTimeout,
+		},
+	)
+}
+
+// RunWithOptions executes the hook and waits for it to complete or for the
+// context or hook-specified timeout to expire.
+func RunWithOptions(ctx context.Context, options RunOptions) (hookErr, err error) {
+	hook := options.Hook
 	cmd := osexec.Cmd{
 		Path:   hook.Path,
 		Args:   hook.Args,
 		Env:    hook.Env,
-		Stdin:  bytes.NewReader(state),
+		Dir:    options.Dir,
-		Stdout: stdout,
+		Stdin:  bytes.NewReader(options.State),
-		Stderr: stderr,
+		Stdout: options.Stdout,
+		Stderr: options.Stderr,
 	}
 	if cmd.Env == nil {
 		cmd.Env = []string{}
@@ -57,11 +91,11 @@ func Run(ctx context.Context, hook *rspec.Hook, state []byte, stdout io.Writer,
 		if err := cmd.Process.Kill(); err != nil {
 			logrus.Errorf("Failed to kill pid %v", cmd.Process)
 		}
-		timer := time.NewTimer(postKillTimeout)
+		timer := time.NewTimer(options.PostKillTimeout)
 		defer timer.Stop()
 		select {
 		case <-timer.C:
-			err = fmt.Errorf("failed to reap process within %s of the kill signal", postKillTimeout)
+			err = fmt.Errorf("failed to reap process within %s of the kill signal", options.PostKillTimeout)
 		case err = <-exit:
 		}
 		return err, ctx.Err()

vendor/github.com/containers/common/pkg/hooks/exec/runtimeconfigfilter.go

@@ -21,19 +21,44 @@ var spewConfig = spew.ConfigState{
 	SortKeys:                true,
 }
 
+type RuntimeConfigFilterOptions struct {
+	// The hooks to run
+	Hooks []spec.Hook
+	// The workdir to change when invoking the hook
+	Dir string
+	// The container config spec to pass into the hook processes and potentially get modified by them
+	Config *spec.Spec
+	// Timeout for waiting process killed
+	PostKillTimeout time.Duration
+}
+
 // RuntimeConfigFilter calls a series of hooks.  But instead of
 // passing container state on their standard input,
 // RuntimeConfigFilter passes the proposed runtime configuration (and
 // reads back a possibly-altered form from their standard output).
+//
+// Deprecated: Too many arguments, has been refactored and replaced by RuntimeConfigFilterWithOptions instead
 func RuntimeConfigFilter(ctx context.Context, hooks []spec.Hook, config *spec.Spec, postKillTimeout time.Duration) (hookErr, err error) {
-	data, err := json.Marshal(config)
+	return RuntimeConfigFilterWithOptions(ctx, RuntimeConfigFilterOptions{
+		Hooks:           hooks,
+		Config:          config,
+		PostKillTimeout: postKillTimeout,
+	})
+}
+
+// RuntimeConfigFilterWithOptions calls a series of hooks.  But instead of
+// passing container state on their standard input,
+// RuntimeConfigFilterWithOptions passes the proposed runtime configuration (and
+// reads back a possibly-altered form from their standard output).
+func RuntimeConfigFilterWithOptions(ctx context.Context, options RuntimeConfigFilterOptions) (hookErr, err error) {
+	data, err := json.Marshal(options.Config)
 	if err != nil {
 		return nil, err
 	}
-	for i, hook := range hooks {
+	for i, hook := range options.Hooks {
 		hook := hook
 		var stdout bytes.Buffer
-		hookErr, err = Run(ctx, &hook, data, &stdout, nil, postKillTimeout)
+		hookErr, err = RunWithOptions(ctx, RunOptions{Hook: &hook, Dir: options.Dir, State: data, Stdout: &stdout, PostKillTimeout: options.PostKillTimeout})
 		if err != nil {
 			return hookErr, err
 		}
@@ -46,8 +71,8 @@ func RuntimeConfigFilter(ctx context.Context, hooks []spec.Hook, config *spec.Sp
 			return nil, fmt.Errorf("unmarshal output from config-filter hook %d: %w", i, err)
 		}
 
-		if !reflect.DeepEqual(config, &newConfig) {
+		if !reflect.DeepEqual(options.Config, &newConfig) {
-			oldConfig := spewConfig.Sdump(config)
+			oldConfig := spewConfig.Sdump(options.Config)
 			newConfig := spewConfig.Sdump(&newConfig)
 			diff, err := difflib.GetUnifiedDiffString(difflib.UnifiedDiff{
 				A:        difflib.SplitLines(oldConfig),
@@ -65,7 +90,7 @@ func RuntimeConfigFilter(ctx context.Context, hooks []spec.Hook, config *spec.Sp
 			}
 		}
 
-		*config = newConfig
+		*options.Config = newConfig
 	}
 
 	return nil, nil

vendor/modules.txt

@@ -125,7 +125,7 @@ github.com/containers/buildah/pkg/rusage
 github.com/containers/buildah/pkg/sshagent
 github.com/containers/buildah/pkg/util
 github.com/containers/buildah/util
-# github.com/containers/common v0.53.1-0.20230621174116-586a3be4e1fc
+# github.com/containers/common v0.53.1-0.20230626115555-370c89881624
 ## explicit; go 1.18
 github.com/containers/common/libimage
 github.com/containers/common/libimage/define