make `podman rmi` more robust

The c/storage library is subject to TOCTOUs as the central container and
image storage may be shared by many instances of many tools.  As shown
in #6510, it's fairly easy to have multiple instances of Podman running
in parallel and yield image-lookup errors when removing them.

The underlying issue is the TOCTOU of removal being split into multiple
stages of first reading the local images and then removing them.  Some
images may already have been removed in between the two stages. To make
image removal more robust, handle errors at stage two when a given image
is not present (anymore) in the storage.

Fixes: #6510
Signed-off-by: Valentin Rothberg <rothberg@redhat.com>

2 files changed, 43 insertions, 4 deletions

diff --git a/pkg/domain/infra/abi/images.go b/pkg/domain/infra/abi/images.go
index 8ca93e770..f2d0f2c39 100644
--- a/pkg/domain/infra/abi/images.go
+++ b/pkg/domain/infra/abi/images.go
@@ -580,12 +580,21 @@ func (ir *ImageEngine) Remove(ctx context.Context, images []string, opts entitie
 	// without having to pass all local data around.
 	deleteImage := func(img *image.Image) error {
 		results, err := ir.Libpod.RemoveImage(ctx, img, opts.Force)
-		if err != nil {
+		switch errors.Cause(err) {
+		case nil:
+			// Removal worked, so let's report it.
+			report.Deleted = append(report.Deleted, results.Deleted)
+			report.Untagged = append(report.Untagged, results.Untagged...)
+			return nil
+		case storage.ErrImageUnknown:
+			// The image must have been removed already (see #6510).
+			report.Deleted = append(report.Deleted, img.ID())
+			report.Untagged = append(report.Untagged, img.ID())
+			return nil
+		default:
+			// Fatal error.
 			return err
 		}
-		report.Deleted = append(report.Deleted, results.Deleted)
-		report.Untagged = append(report.Untagged, results.Untagged...)
-		return nil
 	}
 
 	// Delete all images from the local storage.
diff --git a/test/e2e/rmi_test.go b/test/e2e/rmi_test.go
index 1f40e4928..c1fbe0440 100644
--- a/test/e2e/rmi_test.go
+++ b/test/e2e/rmi_test.go
@@ -1,7 +1,9 @@
 package integration
 
 import (
+	"fmt"
 	"os"
+	"sync"
 
 	. "github.com/containers/podman/v2/test/utils"
 	. "github.com/onsi/ginkgo"
@@ -276,4 +278,32 @@ RUN find $LOCAL
 		match, _ := session.ErrorGrepString("image name or ID must be specified")
 		Expect(match).To(BeTrue())
 	})
+
+	It("podman image rm - concurrent with shared layers", func() {
+		// #6510 has shown a fairly simple reproducer to force storage
+		// errors during parallel image removal.  Since it's subject to
+		// a race, we may not hit the condition a 100 percent of times
+		// but ocal reproducers hit it all the time.
+
+		var wg sync.WaitGroup
+
+		buildAndRemove := func(i int) {
+			defer GinkgoRecover()
+			defer wg.Done()
+			imageName := fmt.Sprintf("rmtest:%d", i)
+			containerfile := `FROM quay.io/libpod/cirros:latest
+RUN ` + fmt.Sprintf("touch %s", imageName)
+
+			podmanTest.BuildImage(containerfile, imageName, "false")
+			session := podmanTest.Podman([]string{"rmi", "-f", imageName})
+			session.WaitWithDefaultTimeout()
+			Expect(session).Should(Exit(0))
+		}
+
+		wg.Add(10)
+		for i := 0; i < 10; i++ {
+			go buildAndRemove(i)
+		}
+		wg.Wait()
+	})
 })