shim: create UDS mount placeholders for the assembled rootfs, not just a bind Source

createSandboxedContainer only created UDS-mount placeholder files by
scanning r.Rootfs for a "bind"-typed entry and writing into its
Source. That misses the common case entirely: a real CRI overlay
snapshotter, `ctr run` with overlayfs, and this repo's own erofs layer
format all present the rootfs as a multi-entry overlay/erofs assembly
(ext4 scratch + erofs layers + a final "format/mkdir/overlay" mount),
never a single "bind" entry, so the loop matched nothing and created
zero placeholders.

In practice this was masked rather than caught by any current test:
the OCI runtime (crun, inside vminitd) auto-creates a missing
bind-mount destination file as long as the underlying rootfs stays
writable, and every current snapshotter/production rootfs is writable
(only a fully-extracted, explicitly read-only bind rootfs -- the
non-root shimtest path -- genuinely needs a pre-created placeholder,
and that's the one shape the old loop happened to handle). The
existing doc comment's premise ("the rootfs will be bind-mounted
read-only") was also simply wrong for the writable overlay/erofs case.

Fix: udsPlaceholderSource (socketforward.go) inspects only the *last*
entry in r.Rootfs -- the one mountutil.All actually mounts at the
final assembled path, since every earlier entry (lower layers, ext4
scratch devices) exists purely to feed that last mount. If it's a
plain read-only bind, its Source is used before ShareRootfs runs (the
only genuinely read-only-after-assembly case, matching prior
behavior). Otherwise -- including the common multi-entry overlay/erofs
shape, where no single entry's Source is the final tree at all -- the
placeholder is written into the assembled rootfs itself
(SharedFS.RootfsHostPath), which stays writable and is only available
once ShareRootfs has run.

Added a regression test (TestCreateRootfsPlaceholders_OverlayShapedRootfs)
that exercises exactly the previously-missed shape, plus table-driven
coverage of udsPlaceholderSource's mount-shape decision
(TestUDSPlaceholderSource).

Verified: go build/vet/gofmt clean; golangci-lint across
linux/darwin x amd64/arm64 (only the 5 known pre-existing gosec
findings); full unit suite; 12/12 integration tests; task test:shim
both non-root and as root (root exercises the erofs/overlay rootfs
shape this fixes) -- only the pre-existing, unrelated
ResourceReleaseOnShutdown flake, identical before and after this
change; cross-platform build (linux/darwin/windows x amd64/arm64);
verify-vendor clean.

Signed-off-by: Derek McGowan <derek@mcg.dev>

Author: dmcgowan

internal/shim/sandbox/sharedfs.go

@@ -101,6 +101,14 @@ func GuestVolumePath(containerID string, n int) string {
 	return path.Join(GuestContainersDir, containerID, "volumes", fmt.Sprintf("%d", n))
 }
 
+// RootfsHostPath returns the host-side path where ShareRootfs assembles the
+// container's rootfs (the same directory GuestRootfsPath(containerID)
+// exposes to the guest via the virtiofs share). Only meaningful after
+// ShareRootfs has returned successfully for containerID.
+func (s *SharedFS) RootfsHostPath(containerID string) string {
+	return filepath.Join(s.root, containerID, "rootfs")
+}
+
 // ShareRootfs resolves the container rootfs from the given containerd mount
 // specs by executing them on the host inside the shim's mount namespace, and
 // exposes the result in the shared filesystem tree so the guest can access it

internal/shim/task/service.go

@@ -351,10 +351,7 @@ func (s *service) createSandboxedContainer(ctx context.Context, r *taskAPI.Creat
 	}
 	sharedNS := &sharedNamespaces{client: vmc}
 
-	// Load the OCI bundle and apply per-container transformers.  This must
-	// happen before ShareRootfs so that UDS mount destinations can be
-	// pre-created in the source rootfs (which is still writable at this
-	// point) before the read-only bind mount is applied.
+	// Load the OCI bundle and apply per-container transformers.
 	var (
 		ctrNetCfg ctrNetConfig
 		svm       = sandboxVolumeMounter{fs: fs, containerID: r.ID}
@@ -391,26 +388,30 @@ func (s *service) createSandboxedContainer(ctx context.Context, r *taskAPI.Creat
 
 	// UDS mounts are rewritten to bind mounts whose source is a socket
 	// file inside the VM and whose destination is a path in the container
-	// rootfs (e.g. /run/shared.sock).  The OCI runtime requires the
-	// destination to already exist as a regular file.  Since the rootfs
-	// will be bind-mounted read-only, we create empty placeholder files in
-	// the SOURCE rootfs directory now, while it is still writable.
-	for _, m := range r.Rootfs {
-		if m.Type == "bind" && m.Source != "" {
-			sfpr.CreateRootfsPlaceholders(ctx, m.Source)
-			break // placeholders are the same regardless of layer; one source suffices
-		}
+	// rootfs (e.g. /run/shared.sock). The OCI runtime requires the
+	// destination to already exist as a regular file. See
+	// udsPlaceholderSource's doc comment for why the correct target
+	// depends on the shape of r.Rootfs: a read-only bind needs its
+	// placeholder written to the still-writable Source before ShareRootfs
+	// mounts it read-only; anything else (in particular the common
+	// overlay/erofs assembly) needs it written to the assembled rootfs
+	// itself, which is only available after ShareRootfs runs.
+	placeholderSrc, placeholderBeforeAssembly := udsPlaceholderSource(r.Rootfs, fs.RootfsHostPath(r.ID))
+	if placeholderBeforeAssembly {
+		sfpr.CreateRootfsPlaceholders(ctx, placeholderSrc)
 	}
 
 	// Assemble the container rootfs on the host inside the shared dir.
-	// Done after bundle loading so UDS placeholders are in place before the
-	// read-only bind mount is applied.
 	guestRootfs, err := fs.ShareRootfs(ctx, r.ID, r.Rootfs)
 	if err != nil {
 		fs.Unshare(ctx, r.ID) //nolint:errcheck
 		return nil, errgrpc.ToGRPC(fmt.Errorf("share rootfs for %s: %w", r.ID, err))
 	}
 
+	if !placeholderBeforeAssembly {
+		sfpr.CreateRootfsPlaceholders(ctx, placeholderSrc)
+	}
+
 	nwJSON, err := json.Marshal(ctrNetCfg)
 	if err != nil {
 		fs.Unshare(ctx, r.ID) //nolint:errcheck

internal/shim/task/socketforward.go

@@ -25,8 +25,10 @@ import (
 	"net"
 	"os"
 	"path/filepath"
+	"slices"
 	"strings"
 
+	"github.com/containerd/containerd/api/types"
 	"github.com/containerd/continuity/fs"
 	"github.com/containerd/log"
 	"github.com/opencontainers/runtime-spec/specs-go"
@@ -171,6 +173,41 @@ func (p *socketForwardsProvider) CreateRootfsPlaceholders(ctx context.Context, s
 	}
 }
 
+// udsPlaceholderSource returns the writable directory where UDS mount
+// placeholder files must be created for a container whose rootfs is
+// assembled from rootfsMounts, and whether that directory is available
+// before SharedFS.ShareRootfs assembles the rootfs (beforeAssembly) or only
+// after (i.e. assembledRootfs, the host path SharedFS.RootfsHostPath
+// returns once ShareRootfs has run).
+//
+// mountutil.All mounts every entry in rootfsMounts, but only the *last*
+// entry ends up at the final assembled path — every other entry (lower
+// layers, ext4 scratch devices, etc.) is mounted elsewhere purely to feed
+// that last mount (e.g. as overlay lowerdir/upperdir sources). So the only
+// mount spec that can tell us anything about the assembled rootfs itself is
+// the last one:
+//
+//   - If it is a plain "bind" mount with the "ro" option, ShareRootfs will
+//     mount its Source read-only at the assembled path, so placeholders
+//     must be written into that still-writable Source *before* ShareRootfs
+//     runs — writing into the assembled path afterward would fail with
+//     EROFS.
+//   - Otherwise — an overlay/erofs assembly with a writable upperdir, a
+//     plain writable bind, or anything else mountutil.All supports — the
+//     assembled path itself stays writable, and is in fact the *only*
+//     correct target: for a multi-entry rootfs (the common overlay/erofs
+//     case) no single entry's Source is the final tree, only the assembled
+//     mountpoint is.
+func udsPlaceholderSource(rootfsMounts []*types.Mount, assembledRootfs string) (path string, beforeAssembly bool) {
+	if len(rootfsMounts) > 0 {
+		last := rootfsMounts[len(rootfsMounts)-1]
+		if last.Type == "bind" && last.Source != "" && slices.Contains(last.Options, "ro") {
+			return last.Source, true
+		}
+	}
+	return assembledRootfs, false
+}
+
 // bindSockets calls the Bind RPC on the VM to set up socket forward
 // listener sockets. This must be called before container creation so that
 // crun can bind-mount the listener sockets into the container.

internal/shim/task/socketforward_test.go

@@ -22,6 +22,7 @@ import (
 	"path/filepath"
 	"testing"
 
+	"github.com/containerd/containerd/api/types"
 	"github.com/opencontainers/runtime-spec/specs-go"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
@@ -176,3 +177,144 @@ func TestCreateRootfsPlaceholders_ConfinesToRootfs(t *testing.T) {
 	// The well-behaved mount's placeholder must still be created normally.
 	assert.FileExists(t, filepath.Join(sourceRootfs, "run", "normal.sock"))
 }
+
+// TestUDSPlaceholderSource covers the mount-shape decision at the heart of
+// the sandboxed UDS placeholder fix: only a rootfs whose *last* mount spec
+// (the one mountutil.All actually mounts at the assembled path) is a
+// read-only bind needs its placeholder written to that mount's Source
+// before ShareRootfs runs. Every other shape — in particular a multi-entry
+// overlay/erofs assembly, which is what a real snapshotter or this repo's
+// erofs layer format actually hands Task.Create — has no single mount
+// whose Source is the final assembled tree, so the assembled rootfs path
+// itself is the only correct, and only available-after-ShareRootfs, target.
+func TestUDSPlaceholderSource(t *testing.T) {
+	const assembled = "/state/containers/ctr-1/rootfs"
+
+	testcases := []struct {
+		name           string
+		mounts         []*types.Mount
+		wantPath       string
+		wantBefore     bool
+		wantPathReason string
+	}{
+		{
+			name:           "no mounts",
+			mounts:         nil,
+			wantPath:       assembled,
+			wantBefore:     false,
+			wantPathReason: "empty rootfs still assembles an (empty) directory at the guest path",
+		},
+		{
+			name: "read-only bind (non-root shimtest / a committed snapshot)",
+			mounts: []*types.Mount{
+				{Type: "bind", Source: "/tmp/extracted-rootfs", Options: []string{"ro", "rbind"}},
+			},
+			wantPath:       "/tmp/extracted-rootfs",
+			wantBefore:     true,
+			wantPathReason: "ShareRootfs will mount this Source read-only at the assembled path",
+		},
+		{
+			name: "writable bind (no ro option)",
+			mounts: []*types.Mount{
+				{Type: "bind", Source: "/tmp/writable-rootfs", Options: []string{"rbind"}},
+			},
+			wantPath:       assembled,
+			wantBefore:     false,
+			wantPathReason: "the bind stays writable, so using the assembled path (equivalent content) after ShareRootfs is correct and simpler",
+		},
+		{
+			name: "bind marked ro but missing Source",
+			mounts: []*types.Mount{
+				{Type: "bind", Source: "", Options: []string{"ro"}},
+			},
+			wantPath:       assembled,
+			wantBefore:     false,
+			wantPathReason: "an empty Source can't be written to before assembly; fall back to the assembled path",
+		},
+		{
+			name: "overlay/erofs multi-layer assembly (the common CRI/erofs shape)",
+			mounts: []*types.Mount{
+				{Type: "ext4", Source: "/state/scratch.ext4", Options: []string{"rw", "loop"}},
+				{Type: "erofs", Source: "/layers/base.erofs", Options: []string{"ro", "loop"}},
+				{
+					Type:   "format/mkdir/overlay",
+					Source: "overlay",
+					Options: []string{
+						"workdir={{ mount 0 }}/work",
+						"upperdir={{ mount 0 }}/upper",
+						"lowerdir={{ mount 1 }}",
+					},
+				},
+			},
+			wantPath:       assembled,
+			wantBefore:     false,
+			wantPathReason: "no single mount's Source is the assembled tree; the overlay's writable upperdir backs the assembled path itself",
+		},
+		{
+			name: "single overlay mount with explicit upperdir",
+			mounts: []*types.Mount{
+				{
+					Type:    "overlay",
+					Source:  "overlay",
+					Options: []string{"lowerdir=/l1:/l2", "upperdir=/upper", "workdir=/work"},
+				},
+			},
+			wantPath:       assembled,
+			wantBefore:     false,
+			wantPathReason: "an overlay mount is never type \"bind\", so it must resolve to the assembled path",
+		},
+	}
+
+	for _, tc := range testcases {
+		t.Run(tc.name, func(t *testing.T) {
+			gotPath, gotBefore := udsPlaceholderSource(tc.mounts, assembled)
+			assert.Equal(t, tc.wantPath, gotPath, tc.wantPathReason)
+			assert.Equal(t, tc.wantBefore, gotBefore)
+		})
+	}
+}
+
+// TestCreateRootfsPlaceholders_OverlayShapedRootfs is an end-to-end
+// regression test for the bug identified in review: previously, placeholder
+// creation only ever scanned r.Rootfs for a "bind"-typed entry, so an
+// overlay/erofs-shaped rootfs (no "bind" entry at all — the shape used by
+// the erofs snapshotter and any real CRI overlay snapshotter) produced zero
+// placeholders, leaving a UDS mount's rewritten bind destination missing.
+//
+// This test drives the same two-call sequence service.go's
+// createSandboxedContainer uses (udsPlaceholderSource to pick a target,
+// then CreateRootfsPlaceholders) against an overlay-shaped mount list and a
+// writable directory standing in for the host path SharedFS.ShareRootfs
+// would have assembled, and asserts the placeholder lands there.
+func TestCreateRootfsPlaceholders_OverlayShapedRootfs(t *testing.T) {
+	ctx := context.Background()
+	assembledRootfs := t.TempDir()
+
+	overlayShapedMounts := []*types.Mount{
+		{Type: "ext4", Source: "/state/scratch.ext4", Options: []string{"rw", "loop"}},
+		{Type: "erofs", Source: "/layers/base.erofs", Options: []string{"ro", "loop"}},
+		{Type: "format/mkdir/overlay", Source: "overlay", Options: []string{
+			"workdir={{ mount 0 }}/work",
+			"upperdir={{ mount 0 }}/upper",
+			"lowerdir={{ mount 1 }}",
+		}},
+	}
+
+	p := &socketForwardsProvider{
+		entries: []socketForwardEntry{
+			{containerPath: "/run/shared.sock"},
+		},
+	}
+
+	placeholderSrc, beforeAssembly := udsPlaceholderSource(overlayShapedMounts, assembledRootfs)
+	require.False(t, beforeAssembly, "an overlay-shaped rootfs has no writable Source available before assembly")
+	require.Equal(t, assembledRootfs, placeholderSrc)
+
+	// Mirror service.go: this call only happens after ShareRootfs would
+	// have assembled the rootfs (here, simply because assembledRootfs
+	// already exists and is writable).
+	p.CreateRootfsPlaceholders(ctx, placeholderSrc)
+
+	assert.FileExists(t, filepath.Join(assembledRootfs, "run", "shared.sock"),
+		"UDS placeholder must be created in the assembled rootfs when no mount entry has a usable pre-assembly Source")
+}