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 <[email protected]>
ashley-cui · Jul 13, 2023 · f701899 · f701899
1 parent 62fc35c
commit f701899
Show file tree

Hide file tree

Showing 3 changed files with 89 additions and 44 deletions.
diff --git a/libpod/container_internal.go b/libpod/container_internal.go
@@ -2101,7 +2101,7 @@ func (c *Container) postDeleteHooks(ctx context.Context) error {
 					Stderr:          &stderr,
 					PostKillTimeout: exec.DefaultPostKillTimeout,
 				}
-				hookErr, err := exec.RunWithOptions(ctx, opts)
+				hookErr, err := exec.RunWithOptions(ctx, opts) //nolint:staticcheck
 				if err != nil {
 					logrus.Warnf("Container %s: poststop hook %d: %v", c.ID(), i, err)
 					if hookErr != err {
@@ -2234,7 +2234,7 @@ func (c *Container) setupOCIHooks(ctx context.Context, config *spec.Spec) (map[s
 		Config:          config,
 		PostKillTimeout: exec.DefaultPostKillTimeout,
 	}
-	hookErr, err := exec.RuntimeConfigFilterWithOptions(ctx, opts)
+	hookErr, err := exec.RuntimeConfigFilterWithOptions(ctx, opts) //nolint:staticcheck
 	if err != nil {
 		logrus.Warnf("Container %s: precreate hook: %v", c.ID(), err)
 		if hookErr != nil && hookErr != err {

diff --git a/pkg/domain/infra/abi/images_list.go b/pkg/domain/infra/abi/images_list.go
@@ -28,54 +28,63 @@ func (ir *ImageEngine) List(ctx context.Context, opts entities.ImageListOptions)
 
 	summaries := []*entities.ImageSummary{}
 	for _, img := range images {
-		repoDigests, err := img.RepoDigests()
-		if err != nil {
-			return nil, fmt.Errorf("getting repoDigests from image %q: %w", img.ID(), err)
-		}
+		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)
-		}
-		isDangling := *img.ListData.IsDangling
-		parentID := ""
-		if img.ListData.Parent != nil {
-			parentID = img.ListData.Parent.ID()
-		}
+			if img.ListData.IsDangling == nil { // Sanity check
+				return nil, fmt.Errorf("%w: ListData.IsDangling is nil but should not be", define.ErrInternal)
+			}
+			isDangling := *img.ListData.IsDangling
+			parentID := ""
+			if img.ListData.Parent != nil {
+				parentID = img.ListData.Parent.ID()
+			}
 
-		e := entities.ImageSummary{
-			ID:          img.ID(),
-			Created:     img.Created().Unix(),
-			Dangling:    isDangling,
-			Digest:      string(img.Digest()),
-			RepoDigests: repoDigests,
-			History:     img.NamesHistory(),
-			Names:       img.Names(),
-			ReadOnly:    img.IsReadOnly(),
-			SharedSize:  0,
-			RepoTags:    img.Names(), // may include tags and digests
-			ParentId:    parentID,
-		}
-		e.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)
-		}
+			s := &entities.ImageSummary{
+				ID:          img.ID(),
+				Created:     img.Created().Unix(),
+				Dangling:    isDangling,
+				Digest:      string(img.Digest()),
+				RepoDigests: repoDigests,
+				History:     img.NamesHistory(),
+				Names:       img.Names(),
+				ReadOnly:    img.IsReadOnly(),
+				SharedSize:  0,
+				RepoTags:    img.Names(), // may include tags and digests
+				ParentId:    parentID,
+			}
+			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)
+			}
 
-		ctnrs, err := img.Containers()
-		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)
+			ctnrs, err := img.Containers()
+			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)
+			}
+			s.Containers = len(ctnrs)
 
-		sz, err := img.Size()
+			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)
+			}
+			s.Size = sz
+			// This is good enough for now, but has to be
+			// replaced later with correct calculation logic
+			s.VirtualSize = sz
+			return s, nil
+		}()
 		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)
+			if libimage.ErrorIsImageUnknown(err) {
+				// The image may have been (partially) removed in the meantime
+				continue
+			}
+			return nil, err
 		}
-		e.Size = sz
-		// This is good enough for now, but has to be
-		// replaced later with correct calculation logic
-		e.VirtualSize = sz
-
-		summaries = append(summaries, &e)
+		summaries = append(summaries, summary)
 	}
 	return summaries, nil
 }
diff --git a/test/system/010-images.bats b/test/system/010-images.bats
@@ -363,4 +363,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