From b52be6e1e2701121ac4a52163c99647589df6930 Mon Sep 17 00:00:00 2001
From: Paul Holzinger <pholzing@redhat.com>
Date: Tue, 3 Dec 2024 14:09:02 +0100
Subject: [PATCH] store: correctly remove incomplete layers on load.

In go one should never modify a slice while also iterating over it at
the same time. This causes weird side effects as the underlying array
elements are shifted around without the range loop index knowing.
So if you delete a element the loop will then actually skip the next one
and theoretically access out of bounds on the last element which does
not panic but rather return the default zero type, nil here which then
causes the panic on layer.Flags == nil.

Here is a simple example to show the behavior:
func main() {
	slice := []int{1, 2, 3, 4, 5, 6, 7, 8, 9}
	for _, num := range slice {
		if num == 5 {
			slice = slices.DeleteFunc(slice, func(n int) bool {
				return n == 5
			})
		}
		fmt.Println(num)
	}
}

The loop will not print 6, but then as last number it prints 0 (the
default zero type for an int).

Fixes #2184

Signed-off-by: Paul Holzinger <pholzing@redhat.com>
---
 layers.go | 27 ++++++++++++++++++---------
 1 file changed, 18 insertions(+), 9 deletions(-)

diff --git a/layers.go b/layers.go
index 6fe1a08035..e78af6cbfd 100644
--- a/layers.go
+++ b/layers.go
@@ -905,6 +905,7 @@ func (r *layerStore) load(lockedForWriting bool) (bool, error) {
 		// and possibly loadMounts() again.
 	}
 
+	var layersToDeleteIDs []string
 	if errorToResolveBySaving != nil {
 		if !r.lockfile.IsReadWrite() {
 			return false, fmt.Errorf("internal error: layerStore.load has shouldSave but !r.lockfile.IsReadWrite")
@@ -918,18 +919,26 @@ func (r *layerStore) load(lockedForWriting bool) (bool, error) {
 				layer.Flags = make(map[string]interface{})
 			}
 			if layerHasIncompleteFlag(layer) {
-				logrus.Warnf("Found incomplete layer %#v, deleting it", layer.ID)
-				err := r.deleteInternal(layer.ID)
-				if err != nil {
-					// Don't return the error immediately, because deleteInternal does not saveLayers();
-					// Even if deleting one incomplete layer fails, call saveLayers() so that other possible successfully
-					// deleted incomplete layers have their metadata correctly removed.
-					incompleteDeletionErrors = multierror.Append(incompleteDeletionErrors,
-						fmt.Errorf("deleting layer %#v: %w", layer.ID, err))
-				}
+				// Important: Do not call r.deleteInternal() here. It modifies r.layers
+				// which causes unexpected side effects while iterating over r.layers here.
+				// The range loop has no idea that the underlying elements where shifted
+				// around.
+				layersToDeleteIDs = append(layersToDeleteIDs, layer.ID)
 				modifiedLocations |= layerLocation(layer)
 			}
 		}
+		// Now actually delete the layer
+		for _, layerID := range layersToDeleteIDs {
+			logrus.Warnf("Found incomplete layer %q, deleting it", layerID)
+			err := r.deleteInternal(layerID)
+			if err != nil {
+				// Don't return the error immediately, because deleteInternal does not saveLayers();
+				// Even if deleting one incomplete layer fails, call saveLayers() so that other possible successfully
+				// deleted incomplete layers have their metadata correctly removed.
+				incompleteDeletionErrors = multierror.Append(incompleteDeletionErrors,
+					fmt.Errorf("deleting layer %#v: %w", layerID, err))
+			}
+		}
 		if err := r.saveLayers(modifiedLocations); err != nil {
 			return false, err
 		}