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]>

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.20230621174116-586a3be4e1fc/go.mod h1:qE1MzGl69IoK7ZNCCH51+aLVjyQtnH0LiZe0wG32Jy0=
+github.com/containers/common v0.53.1-0.20230626115555-370c89881624 h1:YBgjfoo0G3tR8vm225ghJnqOZOVv3tH1L1GbyRM9320=
+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,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
 }

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