Bound symlink blob reads

internal/fusefs/fuse_unix.go

@@ -6,7 +6,6 @@ import (
 	"context"
 	"errors"
 	"fmt"
-	"io"
 	iofs "io/fs"
 	"os"
 	"path/filepath"
@@ -475,26 +474,6 @@ func (fs *ArtifactFuse) Rename(ctx context.Context, op *fuseops.RenameOp) error
 // symlink can point at anyway.
 const maxSymlinkTargetBytes = 4096
 
-// readSymlinkTarget reads a cached Git blob and returns its contents as a
-// symlink target. Git stores the target as the raw blob body, so nothing
-// stops a repo from parking a very large payload behind a mode 120000 entry.
-// We read with a bounded LimitReader and reject anything past PATH_MAX.
-func readSymlinkTarget(cachePath string) (string, error) {
-	f, err := os.Open(cachePath)
-	if err != nil {
-		return "", err
-	}
-	defer f.Close()
-	data, err := io.ReadAll(io.LimitReader(f, maxSymlinkTargetBytes+1))
-	if err != nil {
-		return "", err
-	}
-	if len(data) > maxSymlinkTargetBytes {
-		return "", syscall.ENAMETOOLONG
-	}
-	return string(data), nil
-}
-
 func (fs *ArtifactFuse) ReadSymlink(ctx context.Context, op *fuseops.ReadSymlinkOp) error {
 	ref, err := fs.requireInode(op.Inode, syscall.ESTALE)
 	if err != nil {
@@ -505,26 +484,35 @@ func (fs *ArtifactFuse) ReadSymlink(ctx context.Context, op *fuseops.ReadSymlink
 		return syscall.ENOENT
 	}
 	if n.Base.ObjectOID != "" {
-		if n.Base.SizeState == "known" && n.Base.SizeBytes > maxSymlinkTargetBytes {
-			return syscall.ENAMETOOLONG
-		}
-		cachePath, _, err := fs.engine.Hydrator.EnsureHydrated(ctx, fs.repo, n.Base)
-		if err != nil {
-			return syscall.EIO
+		if err := validateKnownSymlinkTargetSize(n.Base); err != nil {
+			return err
 		}
-		target, err := readSymlinkTarget(cachePath)
+		data, err := fs.engine.Hydrator.ReadBlob(ctx, fs.repo, n.Base, maxSymlinkTargetBytes)
 		if err != nil {
-			if errors.Is(err, syscall.ENAMETOOLONG) {
+			if errors.Is(err, model.ErrBlobTooLarge) {
 				return syscall.ENAMETOOLONG
 			}
 			return syscall.EIO
 		}
-		op.Target = target
+		op.Target = string(data)
 		return nil
 	}
 	return syscall.ENOENT
 }
 
+func validateKnownSymlinkTargetSize(node model.BaseNode) error {
+	if node.SizeState != "known" {
+		return nil
+	}
+	if node.SizeBytes < 0 {
+		return syscall.EIO
+	}
+	if node.SizeBytes > maxSymlinkTargetBytes {
+		return syscall.ENAMETOOLONG
+	}
+	return nil
+}
+
 func (fs *ArtifactFuse) FlushFile(_ context.Context, _ *fuseops.FlushFileOp) error {
 	return nil
 }

internal/fusefs/readsymlink_unix_test.go

@@ -5,9 +5,6 @@ package fusefs
 import (
 	"context"
 	"errors"
-	"os"
-	"path/filepath"
-	"strings"
 	"syscall"
 	"testing"
 
@@ -16,10 +13,13 @@ import (
 )
 
 type fakeSymlinkHydrator struct {
-	calls     int
-	cachePath string
-	size      int64
-	err       error
+	calls         int
+	readBlobCalls int
+	cachePath     string
+	size          int64
+	err           error
+	readBlobData  []byte
+	readBlobErr   error
 }
 
 func (f *fakeSymlinkHydrator) Enqueue(model.HydrationTask) {}
@@ -29,91 +29,93 @@ func (f *fakeSymlinkHydrator) EnsureHydrated(_ context.Context, _ model.RepoConf
 	return f.cachePath, f.size, f.err
 }
 
+func (f *fakeSymlinkHydrator) ReadBlob(_ context.Context, _ model.RepoConfig, _ model.BaseNode, _ int64) ([]byte, error) {
+	f.readBlobCalls++
+	return f.readBlobData, f.readBlobErr
+}
+
 func (f *fakeSymlinkHydrator) QueueDepth(model.RepoID) int { return 0 }
 
-func writeBlob(t *testing.T, dir, name string, data []byte) string {
-	t.Helper()
-	p := filepath.Join(dir, name)
-	if err := os.WriteFile(p, data, 0o644); err != nil {
-		t.Fatalf("write %s: %v", p, err)
-	}
-	return p
-}
+func TestReadSymlinkRejectsKnownOversizedBlobBeforeHydration(t *testing.T) {
+	hydrator := &fakeSymlinkHydrator{}
+	repoID := model.RepoID("repo")
+	resolver := newResolver(
+		&fakeSnapshot{nodes: map[string]model.BaseNode{
+			"link": {
+				RepoID:    repoID,
+				Path:      "link",
+				Type:      "symlink",
+				Mode:      0o120000,
+				ObjectOID: "blob",
+				SizeState: "known",
+				SizeBytes: int64(maxSymlinkTargetBytes + 1),
+			},
+		}},
+		&fakeOverlay{entries: map[string]model.OverlayEntry{}},
+	)
+	fs := NewArtifactFuse(model.RepoConfig{ID: repoID}, resolver, &Engine{Hydrator: hydrator})
 
-func TestReadSymlinkTarget_EmptyTarget(t *testing.T) {
-	dir := t.TempDir()
-	p := writeBlob(t, dir, "empty", nil)
-	got, err := readSymlinkTarget(p)
-	if err != nil {
-		t.Fatalf("unexpected error: %v", err)
-	}
-	if got != "" {
-		t.Fatalf("target = %q, want empty string", got)
-	}
-}
+	fs.mu.Lock()
+	ref := fs.allocInode("link", "symlink", 0o120000)
+	fs.mu.Unlock()
 
-func TestReadSymlinkTarget_ShortTarget(t *testing.T) {
-	dir := t.TempDir()
-	p := writeBlob(t, dir, "short", []byte("../relative/path"))
-	got, err := readSymlinkTarget(p)
-	if err != nil {
-		t.Fatalf("unexpected error: %v", err)
+	op := &fuseops.ReadSymlinkOp{Inode: ref.ID}
+	err := fs.ReadSymlink(context.Background(), op)
+	if !errors.Is(err, syscall.ENAMETOOLONG) {
+		t.Fatalf("err = %v, want ENAMETOOLONG", err)
 	}
-	if got != "../relative/path" {
-		t.Fatalf("target = %q, want %q", got, "../relative/path")
+	if hydrator.calls != 0 {
+		t.Fatalf("EnsureHydrated calls = %d, want 0", hydrator.calls)
 	}
-}
-
-func TestReadSymlinkTarget_AtLimit(t *testing.T) {
-	dir := t.TempDir()
-	data := []byte(strings.Repeat("a", maxSymlinkTargetBytes))
-	p := writeBlob(t, dir, "at-limit", data)
-	got, err := readSymlinkTarget(p)
-	if err != nil {
-		t.Fatalf("unexpected error at %d bytes: %v", maxSymlinkTargetBytes, err)
+	if hydrator.readBlobCalls != 0 {
+		t.Fatalf("ReadBlob calls = %d, want 0", hydrator.readBlobCalls)
 	}
-	if len(got) != maxSymlinkTargetBytes {
-		t.Fatalf("target length = %d, want %d", len(got), maxSymlinkTargetBytes)
+	if op.Target != "" {
+		t.Fatalf("target = %q, want empty", op.Target)
 	}
 }
 
-func TestReadSymlinkTarget_OverLimit(t *testing.T) {
-	dir := t.TempDir()
-	data := []byte(strings.Repeat("a", maxSymlinkTargetBytes+1))
-	p := writeBlob(t, dir, "over-limit", data)
-	_, err := readSymlinkTarget(p)
-	if !errors.Is(err, syscall.ENAMETOOLONG) {
-		t.Fatalf("err = %v, want ENAMETOOLONG", err)
-	}
-}
+func TestReadSymlinkRejectsNegativeKnownBlobBeforeHydration(t *testing.T) {
+	hydrator := &fakeSymlinkHydrator{}
+	repoID := model.RepoID("repo")
+	resolver := newResolver(
+		&fakeSnapshot{nodes: map[string]model.BaseNode{
+			"link": {
+				RepoID:    repoID,
+				Path:      "link",
+				Type:      "symlink",
+				Mode:      0o120000,
+				ObjectOID: "blob",
+				SizeState: "known",
+				SizeBytes: -1,
+			},
+		}},
+		&fakeOverlay{entries: map[string]model.OverlayEntry{}},
+	)
+	fs := NewArtifactFuse(model.RepoConfig{ID: repoID}, resolver, &Engine{Hydrator: hydrator})
+
+	fs.mu.Lock()
+	ref := fs.allocInode("link", "symlink", 0o120000)
+	fs.mu.Unlock()
 
-func TestReadSymlinkTarget_FarOverLimit(t *testing.T) {
-	// A blob that's orders of magnitude past PATH_MAX should still be read
-	// into a bounded slice and rejected, not slurped whole.
-	dir := t.TempDir()
-	data := make([]byte, 1<<20) // 1 MiB
-	for i := range data {
-		data[i] = 'x'
+	op := &fuseops.ReadSymlinkOp{Inode: ref.ID}
+	err := fs.ReadSymlink(context.Background(), op)
+	if !errors.Is(err, syscall.EIO) {
+		t.Fatalf("err = %v, want EIO", err)
 	}
-	p := writeBlob(t, dir, "huge", data)
-	_, err := readSymlinkTarget(p)
-	if !errors.Is(err, syscall.ENAMETOOLONG) {
-		t.Fatalf("err = %v, want ENAMETOOLONG", err)
+	if hydrator.calls != 0 {
+		t.Fatalf("EnsureHydrated calls = %d, want 0", hydrator.calls)
 	}
-}
-
-func TestReadSymlinkTarget_MissingFile(t *testing.T) {
-	_, err := readSymlinkTarget(filepath.Join(t.TempDir(), "does-not-exist"))
-	if err == nil {
-		t.Fatal("expected error for missing cache file, got nil")
+	if hydrator.readBlobCalls != 0 {
+		t.Fatalf("ReadBlob calls = %d, want 0", hydrator.readBlobCalls)
 	}
-	if errors.Is(err, syscall.ENAMETOOLONG) {
-		t.Fatalf("err = %v, want non-ENAMETOOLONG for missing file", err)
+	if op.Target != "" {
+		t.Fatalf("target = %q, want empty", op.Target)
 	}
 }
 
-func TestReadSymlinkRejectsKnownOversizedBlobBeforeHydration(t *testing.T) {
-	hydrator := &fakeSymlinkHydrator{}
+func TestReadSymlinkRejectsUnknownOversizedBlobWithoutHydration(t *testing.T) {
+	hydrator := &fakeSymlinkHydrator{readBlobErr: model.ErrBlobTooLarge}
 	repoID := model.RepoID("repo")
 	resolver := newResolver(
 		&fakeSnapshot{nodes: map[string]model.BaseNode{
@@ -123,8 +125,7 @@ func TestReadSymlinkRejectsKnownOversizedBlobBeforeHydration(t *testing.T) {
 				Type:      "symlink",
 				Mode:      0o120000,
 				ObjectOID: "blob",
-				SizeState: "known",
-				SizeBytes: int64(maxSymlinkTargetBytes + 1),
+				SizeState: "unknown",
 			},
 		}},
 		&fakeOverlay{entries: map[string]model.OverlayEntry{}},
@@ -143,7 +144,48 @@ func TestReadSymlinkRejectsKnownOversizedBlobBeforeHydration(t *testing.T) {
 	if hydrator.calls != 0 {
 		t.Fatalf("EnsureHydrated calls = %d, want 0", hydrator.calls)
 	}
+	if hydrator.readBlobCalls != 1 {
+		t.Fatalf("ReadBlob calls = %d, want 1", hydrator.readBlobCalls)
+	}
 	if op.Target != "" {
 		t.Fatalf("target = %q, want empty", op.Target)
 	}
 }
+
+func TestReadSymlinkReadsUnknownBlobThroughBoundedRead(t *testing.T) {
+	hydrator := &fakeSymlinkHydrator{readBlobData: []byte("../target")}
+	repoID := model.RepoID("repo")
+	resolver := newResolver(
+		&fakeSnapshot{nodes: map[string]model.BaseNode{
+			"link": {
+				RepoID:    repoID,
+				Path:      "link",
+				Type:      "symlink",
+				Mode:      0o120000,
+				ObjectOID: "blob",
+				SizeState: "unknown",
+			},
+		}},
+		&fakeOverlay{entries: map[string]model.OverlayEntry{}},
+	)
+	fs := NewArtifactFuse(model.RepoConfig{ID: repoID}, resolver, &Engine{Hydrator: hydrator})
+
+	fs.mu.Lock()
+	ref := fs.allocInode("link", "symlink", 0o120000)
+	fs.mu.Unlock()
+
+	op := &fuseops.ReadSymlinkOp{Inode: ref.ID}
+	err := fs.ReadSymlink(context.Background(), op)
+	if err != nil {
+		t.Fatalf("ReadSymlink: %v", err)
+	}
+	if hydrator.calls != 0 {
+		t.Fatalf("EnsureHydrated calls = %d, want 0", hydrator.calls)
+	}
+	if hydrator.readBlobCalls != 1 {
+		t.Fatalf("ReadBlob calls = %d, want 1", hydrator.readBlobCalls)
+	}
+	if op.Target != "../target" {
+		t.Fatalf("target = %q, want ../target", op.Target)
+	}
+}