Merge remote-tracking branch 'origin/feat/backup-phase0a-redis-simple' into feat/backup-phase0a-s3

Author: bootjp

internal/backup/disk_full_unix.go

@@ -0,0 +1,15 @@
+//go:build unix
+
+package backup
+
+import (
+	"errors"
+	"syscall"
+)
+
+// isDiskFullError reports whether err is a POSIX ENOSPC anywhere in
+// the wrap chain. os.File.Write surfaces ENOSPC inside an
+// os.PathError which errors.Is unwraps natively.
+func isDiskFullError(err error) bool {
+	return errors.Is(err, syscall.ENOSPC)
+}

internal/backup/disk_full_windows.go

@@ -0,0 +1,27 @@
+//go:build windows
+
+package backup
+
+import (
+	"errors"
+	"syscall"
+
+	"golang.org/x/sys/windows"
+)
+
+// isDiskFullError reports whether err is a Windows disk-full failure.
+// We accept both ERROR_DISK_FULL (Win32 errno 112) and
+// ERROR_HANDLE_DISK_FULL (39): the kernel returns 112 for write
+// failures driven by free-space exhaustion and 39 for the legacy
+// handle-level variant some FS drivers still surface. We also keep
+// syscall.ENOSPC in the chain because Go's os package occasionally
+// translates platform errors into the POSIX value before returning.
+func isDiskFullError(err error) bool {
+	switch {
+	case errors.Is(err, windows.ERROR_DISK_FULL),
+		errors.Is(err, windows.ERROR_HANDLE_DISK_FULL),
+		errors.Is(err, syscall.ENOSPC):
+		return true
+	}
+	return false
+}

internal/backup/redis_string.go

@@ -9,7 +9,7 @@ import (
 	"math"
 	"os"
 	"path/filepath"
-	"syscall"
+	"strings"
 
 	cockroachdberr "github.com/cockroachdb/errors"
 )
@@ -300,18 +300,92 @@ func intToDecimal(v int) string {
 
 // ensureDir runs MkdirAll once per directory and remembers the result
 // in r.dirsCreated, so repeated calls on the hot path (one per blob
-// record) collapse to a map lookup.
+// record) collapse to a map lookup. After MkdirAll succeeds, every
+// path component under outRoot is Lstat-checked: a pre-existing
+// directory symlink at e.g. `<outRoot>/redis/db_0/strings` would
+// otherwise let `os.MkdirAll` succeed without creating anything,
+// then steer subsequent writes outside outRoot. Codex P1 round 9.
+//
+// This guard is best-effort against TOCTOU (an adversary that can
+// swap a directory for a symlink between this check and the open
+// races us either way); it closes the much more common case of a
+// stale symlink left in the output tree from a prior run or
+// configuration mistake. Hardening to fully race-free traversal
+// would require os.Root / openat-style traversal, which is a
+// larger refactor for marginal benefit at this layer.
 func (r *RedisDB) ensureDir(dir string) error {
 	if _, ok := r.dirsCreated[dir]; ok {
 		return nil
 	}
 	if err := os.MkdirAll(dir, 0o755); err != nil { //nolint:mnd // 0755 == standard dir mode
 		return cockroachdberr.WithStack(err)
 	}
+	if err := assertNoSymlinkAncestors(r.outRoot, dir); err != nil {
+		return err
+	}
 	r.dirsCreated[dir] = struct{}{}
 	return nil
 }
 
+// assertNoSymlinkAncestors walks every path component from rootDir up
+// to (and including) target, Lstat'ing each. Returns ErrSymlinkInPath
+// if any component is a symbolic link. rootDir itself is also
+// Lstat'd: if the dump root is a symlink to somewhere else, all bets
+// are off.
+func assertNoSymlinkAncestors(rootDir, target string) error {
+	cleanRoot := filepath.Clean(rootDir)
+	cleanTarget := filepath.Clean(target)
+	rel, err := filepath.Rel(cleanRoot, cleanTarget)
+	if err != nil {
+		return cockroachdberr.WithStack(err)
+	}
+	// Defensive: if target escapes rootDir (which the callers' path
+	// construction already prevents), refuse rather than silently
+	// validate an unrelated path.
+	if rel == ".." || strings.HasPrefix(rel, ".."+string(filepath.Separator)) {
+		return cockroachdberr.WithStack(cockroachdberr.Newf(
+			"backup: target %s escapes root %s", target, rootDir))
+	}
+	if err := lstatRefuseSymlink(cleanRoot); err != nil {
+		return err
+	}
+	cur := cleanRoot
+	if rel == "." {
+		return nil
+	}
+	for _, seg := range strings.Split(rel, string(filepath.Separator)) {
+		if seg == "" {
+			continue
+		}
+		cur = filepath.Join(cur, seg)
+		if err := lstatRefuseSymlink(cur); err != nil {
+			return err
+		}
+	}
+	return nil
+}
+
+// lstatRefuseSymlink returns an error wrapped over the underlying
+// stat call when path is a symbolic link. A non-existent path is
+// treated as fine: the caller has just MkdirAll'd it, so a missing
+// component is impossible — but if it were, the symlink-check
+// contract is "if it exists, it must not be a symlink", and we
+// return nil rather than synthesize a false positive.
+func lstatRefuseSymlink(path string) error {
+	info, err := os.Lstat(path)
+	if err != nil {
+		if os.IsNotExist(err) {
+			return nil
+		}
+		return cockroachdberr.WithStack(err)
+	}
+	if info.Mode()&os.ModeSymlink != 0 {
+		return cockroachdberr.WithStack(cockroachdberr.Newf(
+			"backup: refusing to traverse symlinked ancestor at %s", path))
+	}
+	return nil
+}
+
 func (r *RedisDB) writeBlob(subdir string, userKey, value []byte) error {
 	encoded := EncodeSegment(userKey)
 	if err := r.recordIfFallback(encoded, userKey); err != nil {
@@ -546,11 +620,14 @@ func IsBlobAtomicWriteRetriable(err error) bool {
 
 // IsBlobAtomicWriteOutOfSpace reports whether err from writeFileAtomic
 // (or any os.File write the master pipeline issues) was driven by a
-// full disk. Tested via syscall.ENOSPC + os.PathError unwrap, which
-// matches what os.File.Write returns on POSIX and Windows.
+// full disk. The platform-specific error codes (POSIX ENOSPC vs.
+// Windows ERROR_DISK_FULL / ERROR_HANDLE_DISK_FULL) live in
+// disk_full_{unix,windows}.go so retry/alarm logic in callers
+// classifies disk-full uniformly across operating systems
+// (Codex P2 round 9).
 func IsBlobAtomicWriteOutOfSpace(err error) bool {
 	if err == nil {
 		return false
 	}
-	return errors.Is(err, syscall.ENOSPC)
+	return isDiskFullError(err)
 }

internal/backup/redis_string_test.go

@@ -199,6 +199,41 @@ func TestRedisDB_NewFormatStringTTLNotDuplicatedByScanIndex(t *testing.T) {
 	assertTTLSidecar(t, filepath.Join(root, "redis", "db_0", "strings_ttl.jsonl"), "expiring", fixedExpireMs)
 }
 
+// TestRedisDB_RefusesSymlinkedAncestor is the regression for Codex P1
+// round 9: O_NOFOLLOW only blocks the final-component symlink. A
+// pre-existing directory symlink anywhere up the path (e.g.
+// `<outRoot>/redis/db_0/strings -> /tmp/outside`) lets MkdirAll
+// silently honor it and steers writeFileAtomic / openSidecarFile
+// outside outRoot. ensureDir now Lstat-walks each ancestor under
+// outRoot and refuses if any is a symlink.
+func TestRedisDB_RefusesSymlinkedAncestor(t *testing.T) {
+	t.Parallel()
+	root := t.TempDir()
+	// Pre-place the symlink trap before constructing the encoder so
+	// the directory tree contains a poisoned ancestor at
+	// <root>/redis/db_0/strings.
+	bait := filepath.Join(root, "bait-tree")
+	if err := os.MkdirAll(bait, 0o755); err != nil {
+		t.Fatal(err)
+	}
+	parent := filepath.Join(root, "redis", "db_0")
+	if err := os.MkdirAll(parent, 0o755); err != nil {
+		t.Fatal(err)
+	}
+	if err := os.Symlink(bait, filepath.Join(parent, "strings")); err != nil {
+		t.Fatal(err)
+	}
+	db := NewRedisDB(root, 0)
+	err := db.HandleString([]byte("k"), encodeNewStringValue(t, []byte("v"), 0))
+	if err == nil || !strings.Contains(err.Error(), "refusing to traverse symlinked ancestor") {
+		t.Fatalf("expected symlinked-ancestor refusal, got %v", err)
+	}
+	// The symlink target must be untouched: no `k.bin` written.
+	if _, statErr := os.Stat(filepath.Join(bait, "k.bin")); !os.IsNotExist(statErr) {
+		t.Fatalf("blob written through ancestor symlink: stat err=%v", statErr)
+	}
+}
+
 // TestRedisDB_OpenJSONLRefusesHardLinkClobber is the regression for
 // Codex P2 round 9: O_NOFOLLOW only blocks symlinks; an adversary
 // who can write the output directory could pre-create