From f701899918065ef1ae9451fb6129d18ffe44925c Mon Sep 17 00:00:00 2001 From: Valentin Rothberg Date: Fri, 23 Jun 2023 13:43:36 +0200 Subject: [PATCH] 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 --- libpod/container_internal.go | 4 +- pkg/domain/infra/abi/images_list.go | 93 ++++++++++++++++------------- test/system/010-images.bats | 36 +++++++++++ 3 files changed, 89 insertions(+), 44 deletions(-) diff --git a/libpod/container_internal.go b/libpod/container_internal.go index 556ab86b52..441642a0f3 100644 --- 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 index d9661721a3..b980dcc001 100644 --- 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 index 78b83a0e69..4c59bcbc93 100644 --- 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 <