From db37d66cd171b07416742fdda3a2a79c7fea6531 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 --- go.mod | 2 +- go.sum | 4 +- libpod/container_internal.go | 4 +- pkg/domain/infra/abi/images_list.go | 101 ++++++++++-------- test/system/010-images.bats | 36 +++++++ .../containers/common/libimage/image.go | 2 +- .../containers/common/libimage/layer_tree.go | 29 +++++ .../libnetwork/slirp4netns/slirp4netns.go | 8 +- .../containers/common/pkg/hooks/exec/exec.go | 44 +++++++- .../pkg/hooks/exec/runtimeconfigfilter.go | 37 +++++-- vendor/modules.txt | 2 +- 11 files changed, 202 insertions(+), 67 deletions(-) diff --git a/go.mod b/go.mod index c3125f9ec6..f2e8d4bf5a 100644 --- a/go.mod +++ b/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 diff --git a/go.sum b/go.sum index f10266a4ad..83189ae016 100644 --- a/go.sum +++ b/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= diff --git a/libpod/container_internal.go b/libpod/container_internal.go index 136902d45a..dd1936ef44 100644 --- a/libpod/container_internal.go +++ b/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 { 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() + 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 be", define.ErrInternal) + } + isDangling := *img.ListData.IsDangling + parentID := "" + if img.ListData.Parent != nil { + parentID = img.ListData.Parent.ID() + } + + 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) + } + 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) + } + 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("getting repoDigests from image %q: %w", img.ID(), err) + if libimage.ErrorIsImageUnknown(err) { + // The image may have been (partially) removed in the meantime + continue + } + return nil, 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() - } - - 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) - } - - 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) - - 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 - // 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 ca3fc78dbc..89f4e61328 100644 --- a/test/system/010-images.bats +++ b/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 <