From 510d2ec49e8011f10e1c7c428993946a04b646a2 Mon Sep 17 00:00:00 2001 From: Giuseppe Scrivano Date: Thu, 12 Mar 2026 13:40:31 +0100 Subject: [PATCH 1/3] storage, overlay: replace os.IsNotExist with errors.Is Signed-off-by: Giuseppe Scrivano --- storage/drivers/overlay/overlay.go | 36 +++++++++++++++--------------- 1 file changed, 18 insertions(+), 18 deletions(-) diff --git a/storage/drivers/overlay/overlay.go b/storage/drivers/overlay/overlay.go index 00974c890c..10cf7dfa06 100644 --- a/storage/drivers/overlay/overlay.go +++ b/storage/drivers/overlay/overlay.go @@ -648,10 +648,10 @@ func SupportsNativeOverlay(home, runhome string) (bool, error) { // Do nothing. default: needsMountProgram, err := scanForMountProgramIndicators(home) - if err != nil && !os.IsNotExist(err) { + if err != nil && !errors.Is(err, fs.ErrNotExist) { return false, err } - if err := os.WriteFile(getMountProgramFlagFile(home), []byte(fmt.Sprintf("%t", needsMountProgram)), 0o600); err != nil && !os.IsNotExist(err) { + if err := os.WriteFile(getMountProgramFlagFile(home), []byte(fmt.Sprintf("%t", needsMountProgram)), 0o600); err != nil && !errors.Is(err, fs.ErrNotExist) { return false, err } if needsMountProgram { @@ -1193,7 +1193,7 @@ func (d *Driver) getLower(parent string) (string, error) { // Read Parent link fileA parentLink, err := os.ReadFile(path.Join(parentDir, "link")) if err != nil { - if !os.IsNotExist(err) { + if !errors.Is(err, fs.ErrNotExist) { return "", err } logrus.Warnf("Can't read parent link %q because it does not exist. Going through storage to recreate the missing links.", path.Join(parentDir, "link")) @@ -1266,7 +1266,7 @@ func (d *Driver) getLowerDirs(id string) ([]string, error) { // if the link does not exist, we lost the symlinks during a sudden reboot. // Let's go ahead and recreate those symlinks. if err != nil { - if os.IsNotExist(err) { + if errors.Is(err, fs.ErrNotExist) { logrus.Warnf("Can't read link %q because it does not exist. A storage corruption might have occurred, attempting to recreate the missing symlinks. It might be best wipe the storage to avoid further errors due to storage corruption.", lower) if err := d.recreateSymlinks(); err != nil { return nil, fmt.Errorf("recreating the missing symlinks: %w", err) @@ -1282,7 +1282,7 @@ func (d *Driver) getLowerDirs(id string) ([]string, error) { } lowersArray = append(lowersArray, path.Clean(d.dir(path.Join("link", lp)))) } - } else if !os.IsNotExist(err) { + } else if !errors.Is(err, fs.ErrNotExist) { return nil, err } return lowersArray, nil @@ -1335,7 +1335,7 @@ func (d *Driver) removeCommon(id string, cleanup func(string) error) error { d.releaseAdditionalLayerByID(id) - if err := cleanup(dir); err != nil && !os.IsNotExist(err) { + if err := cleanup(dir); err != nil && !errors.Is(err, fs.ErrNotExist) { return err } if d.quotaCtl != nil { @@ -1430,7 +1430,7 @@ func (d *Driver) recreateSymlinks() error { // Check if the symlink exists, and if it doesn't, create it again with the // name we got from the "link" file err = fileutils.Lexists(linkPath) - if err != nil && os.IsNotExist(err) { + if err != nil && errors.Is(err, fs.ErrNotExist) { if err := os.Symlink(path.Join("..", dir.Name(), "diff"), linkPath); err != nil { errs = errors.Join(errs, err) continue @@ -1464,7 +1464,7 @@ func (d *Driver) recreateSymlinks() error { errs = errors.Join(errs, fmt.Errorf("link target of %q looks weird: %q", link, target)) // force the link to be recreated on the next pass if err := os.Remove(filepath.Join(linkDirFullPath, link.Name())); err != nil { - if !os.IsNotExist(err) { + if !errors.Is(err, fs.ErrNotExist) { errs = errors.Join(errs, fmt.Errorf("removing link %q: %w", link, err)) } // else don’t report any error, but also don’t set madeProgress. continue @@ -1590,7 +1590,7 @@ func (d *Driver) get(id string, disableShifting bool, options graphdriver.MountO } lowers, err := os.ReadFile(path.Join(dir, lowerFile)) - if err != nil && !os.IsNotExist(err) { + if err != nil && !errors.Is(err, fs.ErrNotExist) { return "", err } splitLowers := strings.Split(string(lowers), ":") @@ -1645,7 +1645,7 @@ func (d *Driver) get(id string, disableShifting bool, options graphdriver.MountO maybeAddComposefsMount := func(lowerID string, i int, readWrite bool) (string, error) { composefsBlob := d.getComposefsData(lowerID) if err := fileutils.Exists(composefsBlob); err != nil { - if os.IsNotExist(err) { + if errors.Is(err, fs.ErrNotExist) { return "", nil } return "", err @@ -1706,7 +1706,7 @@ func (d *Driver) get(id string, disableShifting bool, options graphdriver.MountO // if it is a "not found" error, that means the symlinks were lost in a sudden reboot // so call the recreateSymlinks function to go through all the layer dirs and recreate // the symlinks with the name from their respective "link" files - if lower == "" && os.IsNotExist(err) { + if lower == "" && errors.Is(err, fs.ErrNotExist) { logrus.Warnf("Can't stat lower layer %q because it does not exist. Going through storage to recreate the missing symlinks.", newpath) if err := d.recreateSymlinks(); err != nil { return "", fmt.Errorf("recreating the missing symlinks: %w", err) @@ -1983,7 +1983,7 @@ func (d *Driver) Put(id string) error { if count := d.ctr.Decrement(mountpoint); count > 0 { return nil } - if err := fileutils.Exists(path.Join(dir, lowerFile)); err != nil && !os.IsNotExist(err) { + if err := fileutils.Exists(path.Join(dir, lowerFile)); err != nil && !errors.Is(err, fs.ErrNotExist) { return err } @@ -2028,7 +2028,7 @@ func (d *Driver) Put(id string) error { } if !unmounted { - if err := unix.Unmount(mountpoint, unix.MNT_DETACH); err != nil && !os.IsNotExist(err) { + if err := unix.Unmount(mountpoint, unix.MNT_DETACH); err != nil && !errors.Is(err, fs.ErrNotExist) { logrus.Debugf("Failed to unmount %s overlay: %s - %v", id, mountpoint, err) if !errors.Is(err, unix.EINVAL) { return fmt.Errorf("unmounting %q: %w", mountpoint, err) @@ -2205,7 +2205,7 @@ func (d *Driver) DiffGetter(id string) (_ graphdriver.FileGetCloser, Err error) if Err != nil { for _, f := range composefsMounts { f.Close() - if err := unix.Rmdir(f.Name()); err != nil && !os.IsNotExist(err) { + if err := unix.Rmdir(f.Name()); err != nil && !errors.Is(err, fs.ErrNotExist) { logrus.Warnf("Failed to remove %s: %v", f.Name(), err) } } @@ -2378,7 +2378,7 @@ func (d *Driver) ApplyDiffFromStagingDirectory(id, parent string, diffOutput *gr return err } } - if err := os.RemoveAll(diffPath); err != nil && !os.IsNotExist(err) { + if err := os.RemoveAll(diffPath); err != nil && !errors.Is(err, fs.ErrNotExist) { return err } @@ -2750,7 +2750,7 @@ func (d *Driver) getAdditionalLayerPath(tocDigest digest.Digest, ref string) (st func (d *Driver) releaseAdditionalLayerByID(id string) { if al, err := d.getAdditionalLayerPathByID(id); err == nil { notifyReleaseAdditionalLayer(al) - } else if !os.IsNotExist(err) { + } else if !errors.Is(err, fs.ErrNotExist) { logrus.Warnf("Unexpected error on reading Additional Layer Store pointer %v", err) } } @@ -2827,7 +2827,7 @@ func notifyUseAdditionalLayer(al string) { } useFile := path.Join(al, "use") f, err := os.Create(useFile) - if os.IsNotExist(err) { + if errors.Is(err, fs.ErrNotExist) { return } else if err == nil { f.Close() @@ -2849,7 +2849,7 @@ func notifyReleaseAdditionalLayer(al string) { } // tell the additional layer store that we don't use this layer anymore. err := unix.Rmdir(al) - if os.IsNotExist(err) { + if errors.Is(err, fs.ErrNotExist) { return } logrus.Warnf("Unexpected error by Additional Layer Store %v during release; GC doesn't seem to be supported", err) From c1a2337b276fd247a7352cb89e761f5b06a0ffab Mon Sep 17 00:00:00 2001 From: Giuseppe Scrivano Date: Thu, 12 Mar 2026 10:25:35 +0000 Subject: [PATCH 2/3] storage: release additional layer handler on LookupAdditionalLayer errors When LookupAdditionalLayer successfully looks up a layer from the additional layer store but then fails on Info() or JSON decoding, the drivers.AdditionalLayer is never released. This leaks the reference counter that was incremented during lookup, preventing the additional layer store from garbage collecting the layer. Call Release() on error paths. Co-Authored-By: Claude Opus 4.6 Signed-off-by: Giuseppe Scrivano --- storage/store.go | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/storage/store.go b/storage/store.go index 3d8ea50759..8b2a699805 100644 --- a/storage/store.go +++ b/storage/store.go @@ -3499,6 +3499,12 @@ func (s *store) LookupAdditionalLayer(tocDigest digest.Digest, imageref string) } return nil, err } + succeeded := false + defer func() { + if !succeeded { + al.Release() + } + }() info, err := al.Info() if err != nil { return nil, err @@ -3508,6 +3514,7 @@ func (s *store) LookupAdditionalLayer(tocDigest digest.Digest, imageref string) if err := json.NewDecoder(info).Decode(&layer); err != nil { return nil, err } + succeeded = true return &additionalLayer{&layer, al, s}, nil } From adbda5abfb2a238f14819016c6b8e58718a4c5d2 Mon Sep 17 00:00:00 2001 From: Giuseppe Scrivano Date: Thu, 12 Mar 2026 11:38:54 +0000 Subject: [PATCH 3/3] storage: add tests for LookupAdditionalLayer Co-Authored-By: Claude Opus 4.6 Signed-off-by: Giuseppe Scrivano --- storage/additional_layer_test.go | 83 ++++++++++++++++++++++++++++++++ 1 file changed, 83 insertions(+) create mode 100644 storage/additional_layer_test.go diff --git a/storage/additional_layer_test.go b/storage/additional_layer_test.go new file mode 100644 index 0000000000..933caabf46 --- /dev/null +++ b/storage/additional_layer_test.go @@ -0,0 +1,83 @@ +//go:build linux + +package storage + +import ( + "encoding/base64" + "os" + "path/filepath" + "testing" + + digest "github.com/opencontainers/go-digest" + "github.com/sirupsen/logrus" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// setupAdditionalLayerStore creates a temporary additional layer store directory +// with the expected structure for the given tocDigest and image reference. +// The info file contains the provided content. +func setupAdditionalLayerStore(t *testing.T, tocDigest digest.Digest, imageRef string, infoContent string) string { + t.Helper() + alsRoot := t.TempDir() + + refDir := base64.StdEncoding.EncodeToString([]byte(imageRef)) + layerDir := filepath.Join(alsRoot, refDir, tocDigest.String()) + require.NoError(t, os.MkdirAll(layerDir, 0o755)) + + require.NoError(t, os.MkdirAll(filepath.Join(layerDir, "diff"), 0o755)) + require.NoError(t, os.WriteFile(filepath.Join(layerDir, "info"), []byte(infoContent), 0o644)) + require.NoError(t, os.WriteFile(filepath.Join(layerDir, "blob"), []byte{}, 0o644)) + + return alsRoot +} + +func TestLookupAdditionalLayerSuccess(t *testing.T) { + prevLevel := logrus.GetLevel() + logrus.SetLevel(logrus.ErrorLevel) + t.Cleanup(func() { logrus.SetLevel(prevLevel) }) + + tocDigest := digest.FromString("test-layer") + imageRef := "fedora" + + info := Layer{ + ID: "test-id", + CompressedSize: 42, + TOCDigest: tocDigest, + } + infoJSON, err := json.Marshal(info) + require.NoError(t, err) + + alsPath := setupAdditionalLayerStore(t, tocDigest, imageRef, string(infoJSON)) + store := newTestStore(t, StoreOptions{ + GraphDriverName: "overlay", + GraphDriverOptions: []string{"additionallayerstore=" + alsPath + ":ref"}, + }) + t.Cleanup(func() { _, _ = store.Shutdown(true) }) + + al, err := store.LookupAdditionalLayer(tocDigest, imageRef) + require.NoError(t, err) + defer al.Release() + + assert.Equal(t, tocDigest, al.TOCDigest()) + assert.Equal(t, int64(42), al.CompressedSize()) +} + +func TestLookupAdditionalLayerDecodeError(t *testing.T) { + prevLevel := logrus.GetLevel() + logrus.SetLevel(logrus.ErrorLevel) + t.Cleanup(func() { logrus.SetLevel(prevLevel) }) + + tocDigest := digest.FromString("test-layer") + imageRef := "fedora" + + alsPath := setupAdditionalLayerStore(t, tocDigest, imageRef, "not valid json") + store := newTestStore(t, StoreOptions{ + GraphDriverName: "overlay", + GraphDriverOptions: []string{"additionallayerstore=" + alsPath + ":ref"}, + }) + t.Cleanup(func() { _, _ = store.Shutdown(true) }) + + _, err := store.LookupAdditionalLayer(tocDigest, imageRef) + assert.Error(t, err, "should fail on invalid JSON in info file") +}