From d92088d822ccd00be95e4b4f9c8a511065be45f7 Mon Sep 17 00:00:00 2001 From: Derek McGowan Date: Tue, 1 Sep 2026 19:18:38 -0700 Subject: [PATCH] erofs: refuse to apply into a read-only snapshot The differ found an already-populated snapshot by looking for a symlinked layer.erofs in the layer directory. That check needs a particular on-disk layout, and it needs erofsutils.MountsToLayer to infer the snapshot directory from a mount source that lives inside it. Neither holds for a snapshot whose content is not a local file. Refuse on the signal the unpacker already uses. Mounts handed to Apply that are read-only belong to a snapshot the snapshotter populated itself, whose content is shared with every other snapshot of that layer. This does not need path inference, covers parented and parentless snapshots alike, and stays correct for any content the snapshotter does not store as a plain file in the snapshot directory. The error stays ErrFailedPrecondition. plugins/services/diff/local.go falls through to the next differ on ErrNotImplemented. Another differ would fail on this layer for the same reason. Signed-off-by: Derek McGowan --- internal/erofsutils/mount.go | 17 ------ internal/erofsutils/mount_test.go | 28 ---------- plugins/diff/erofs/differ.go | 25 ++++----- plugins/diff/erofs/differ_test.go | 90 +++++++++++++++++++++++++++++++ 4 files changed, 103 insertions(+), 57 deletions(-) create mode 100644 plugins/diff/erofs/differ_test.go diff --git a/internal/erofsutils/mount.go b/internal/erofsutils/mount.go index ef5718ca72..6c3ef9f648 100644 --- a/internal/erofsutils/mount.go +++ b/internal/erofsutils/mount.go @@ -19,7 +19,6 @@ package erofsutils import ( "bytes" "context" - "errors" "fmt" "io" "os" @@ -209,22 +208,6 @@ func MountsToLayer(mounts []mount.Mount) (string, error) { return layer, nil } -// StagedLayerBlob reports whether the layer directory's blob was staged from a -// layer content cache. A staged blob is a symlink into that operator-owned -// cache and is shared with every other snapshot of the layer, so it must never -// be written to. -func StagedLayerBlob(layer string) (bool, error) { - layerBlob := filepath.Join(layer, "layer.erofs") - fi, err := os.Lstat(layerBlob) - if err != nil { - if errors.Is(err, os.ErrNotExist) { - return false, nil - } - return false, fmt.Errorf("failed to stat layer blob %q: %w", layerBlob, err) - } - return fi.Mode()&os.ModeSymlink != 0, nil -} - // SupportGenerateFromTar checks if the installed version of mkfs.erofs supports // the tar mode (--tar option). func SupportGenerateFromTar() (bool, error) { diff --git a/internal/erofsutils/mount_test.go b/internal/erofsutils/mount_test.go index 6a6ae19fe4..67bf90c0a3 100644 --- a/internal/erofsutils/mount_test.go +++ b/internal/erofsutils/mount_test.go @@ -40,34 +40,6 @@ func TestCacheBlobPath(t *testing.T) { assert.Equal(t, enc[:2], filepath.Base(filepath.Dir(got)), "blob must live under a 2-char prefix dir") } -// TestStagedLayerBlob covers detection of a blob staged from the layer content -// cache. A staged blob is a symlink shared with every other snapshot of that -// layer, so the differ must refuse to write it. -func TestStagedLayerBlob(t *testing.T) { - layer := t.TempDir() - - // No blob yet. - staged, err := StagedLayerBlob(layer) - require.NoError(t, err) - assert.False(t, staged) - - // A regular layer blob, written by the erofs differ. - blob := filepath.Join(layer, "layer.erofs") - require.NoError(t, os.WriteFile(blob, nil, 0644)) - staged, err = StagedLayerBlob(layer) - require.NoError(t, err) - assert.False(t, staged) - - // A staged one. - cached := filepath.Join(t.TempDir(), "cached.erofs") - require.NoError(t, os.WriteFile(cached, []byte("shared blob"), 0644)) - require.NoError(t, os.Remove(blob)) - require.NoError(t, os.Symlink(cached, blob)) - staged, err = StagedLayerBlob(layer) - require.NoError(t, err) - assert.True(t, staged) -} - // TestMountsToLayerStagedBlob covers resolving the layer directory of a // snapshot whose blob is staged. Compare only reads the blob, so it resolves // like any other; the apply path is where a staged blob is refused. diff --git a/plugins/diff/erofs/differ.go b/plugins/diff/erofs/differ.go index 60b4be6435..4e1c8e44f2 100644 --- a/plugins/diff/erofs/differ.go +++ b/plugins/diff/erofs/differ.go @@ -127,6 +127,19 @@ func (s erofsDiff) Apply(ctx context.Context, desc ocispec.Descriptor, mounts [] } }() + // Read-only mounts belong to a snapshot the snapshotter populated itself, + // for example from a layer content cache. The layer is already there and is + // shared with every other snapshot of it. Applying would write to content + // this differ does not own. The unpacker reads the same mounts and skips + // such a layer before it reaches here. + // + // The error is ErrFailedPrecondition. The diff service falls through to the + // next differ only on ErrNotImplemented. Another differ would fail on this + // layer for the same reason. + if len(mounts) > 0 && mounts[len(mounts)-1].ReadOnly() { + return emptyDesc, fmt.Errorf("cannot apply to a read-only snapshot, its content is already populated: %w", errdefs.ErrFailedPrecondition) + } + var ( erofsLayerType string fastcopy bool @@ -179,18 +192,6 @@ func (s erofsDiff) Apply(ctx context.Context, desc ocispec.Descriptor, mounts [] return emptyDesc, err } - // A staged blob is shared with every other snapshot of that layer, so - // applying would truncate the cache entry through the symlink. The unpacker - // reads the read-only mounts the snapshotter hands out and skips such a - // layer before it reaches here. - staged, err := erofsutils.StagedLayerBlob(layer) - if err != nil { - return emptyDesc, err - } - if staged { - return emptyDesc, fmt.Errorf("layer %q is already populated from the layer content cache: %w", layer, errdefs.ErrFailedPrecondition) - } - ra, err := s.store.ReaderAt(ctx, desc) if err != nil { return emptyDesc, fmt.Errorf("failed to get reader from content store: %w", err) diff --git a/plugins/diff/erofs/differ_test.go b/plugins/diff/erofs/differ_test.go new file mode 100644 index 0000000000..56c62da693 --- /dev/null +++ b/plugins/diff/erofs/differ_test.go @@ -0,0 +1,90 @@ +/* + Copyright The containerd Authors. + + Licensed under the Apache License, Version 2.0 (the "License"); + you may not use this file except in compliance with the License. + You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, software + distributed under the License is distributed on an "AS IS" BASIS, + WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + See the License for the specific language governing permissions and + limitations under the License. +*/ + +package erofs + +import ( + "context" + "testing" + + "github.com/containerd/errdefs" + ocispec "github.com/opencontainers/image-spec/specs-go/v1" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/containerd/containerd/v2/core/mount" +) + +// TestApplyReadOnlyMounts covers the differ's refusal to apply into a snapshot +// the snapshotter has already populated, for example from a layer content +// cache. The snapshotter says so by handing out read-only mounts. +// +// The error must be ErrFailedPrecondition and never ErrNotImplemented: +// plugins/services/diff/local.go treats ErrNotImplemented as the signal to try +// the next differ. +func TestApplyReadOnlyMounts(t *testing.T) { + ctx := context.Background() + d := erofsDiff{} + + // All erofs mounts are read-only. + erofsMount := mount.Mount{ + Type: "erofs", + Source: "/var/lib/containerd/snapshots/1/layer.erofs", + Options: []string{"ro", "loop"}, + } + // These mounts are a populated snapshot stacked over its parents. Without + // an upperdir the overlay is read-only. + overlayMounts := []mount.Mount{ + erofsMount, + {Type: "erofs", Source: "/var/lib/containerd/snapshots/2/layer.erofs", Options: []string{"ro", "loop"}}, + { + Type: "format/mkdir/overlay", + Source: "overlay", + Options: []string{"lowerdir={{ overlay 0 1 }}", "ro"}, + }, + } + + for _, tc := range []struct { + name string + mounts []mount.Mount + }{ + {name: "erofs layer", mounts: []mount.Mount{erofsMount}}, + {name: "read-only overlay", mounts: overlayMounts}, + } { + t.Run(tc.name, func(t *testing.T) { + desc := ocispec.Descriptor{MediaType: ocispec.MediaTypeImageLayerGzip} + _, err := d.Apply(ctx, desc, tc.mounts) + require.Error(t, err) + assert.ErrorIs(t, err, errdefs.ErrFailedPrecondition) + assert.NotErrorIs(t, err, errdefs.ErrNotImplemented, + "must not fall through to the next differ") + }) + } + + // A writable snapshot is applied normally. The read-only guard does not + // fire. Apply reaches the media type check and fails there. + t.Run("writable mounts are not refused", func(t *testing.T) { + mounts := []mount.Mount{{ + Type: "bind", + Source: "/var/lib/containerd/snapshots/3/fs", + Options: []string{"rw", "rbind"}, + }} + desc := ocispec.Descriptor{MediaType: "application/vnd.oci.image.layer.v1.tar+unsupported"} + _, err := d.Apply(ctx, desc, mounts) + require.Error(t, err) + assert.NotErrorIs(t, err, errdefs.ErrFailedPrecondition) + }) +}