Skip to content

Commit

Permalink
store: correctly remove incomplete layers on load.
Browse files Browse the repository at this point in the history
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 containers#2184

Signed-off-by: Paul Holzinger <pholzing@redhat.com>
  • Loading branch information
Luap99 authored and kwilczynski committed Dec 11, 2024
1 parent 0399733 commit 485c9e0
Showing 1 changed file with 19 additions and 10 deletions.
29 changes: 19 additions & 10 deletions layers.go
Original file line number Diff line number Diff line change
Expand Up @@ -827,22 +827,31 @@ func (r *layerStore) load(lockedForWriting bool) (bool, error) {
// user of this storage area marked for deletion but didn't manage to
// actually delete.
var incompleteDeletionErrors error // = nil
var layersToDelete []*Layer
for _, layer := range r.layers {
if layer.Flags == nil {
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))
}
modifiedLocations |= layerLocation(layer)
// 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.
layersToDelete = append(layersToDelete, layer)
}
}
// Now actually delete the layers
for _, layer := range layersToDelete {
logrus.Warnf("Found incomplete layer %q, 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))
}
modifiedLocations |= layerLocation(layer)
}
if err := r.saveLayers(modifiedLocations); err != nil {
return false, err
Expand Down

0 comments on commit 485c9e0

Please sign in to comment.