diff --git a/internal/filesystem/alias.go b/internal/filesystem/alias.go --- a/internal/filesystem/alias.go +++ b/internal/filesystem/alias.go @@ -5,6 +5,8 @@ "fmt" "os" "path/filepath" + + "github.com/alyraffauf/cattery/internal/pathsafe" ) // AliasSpec is the desired relative payload of one alias and whether the @@ -12,6 +14,11 @@ type AliasSpec struct { Payload string Overwrite bool +} + +type aliasDecision struct { + spec AliasSpec + outcome aliasOutcome } // AliasRealization names the outcome of one alias operation. @@ -54,11 +61,7 @@ case KindAbsent: return outcomeCreate, nil case KindSymlink: - live, err := readLinkPayload(targetPath(precondition.Destination())) - if err != nil { - return 0, err - } - if live == spec.Payload { + if precondition.Target().Payload() == spec.Payload { return outcomeExact, nil } return outcomeOccupied, nil @@ -76,6 +79,9 @@ // atomically; directories and special entries fail with manual // intervention. func (r *Replacer) RealizeAlias(ctx context.Context, precondition Precondition, spec AliasSpec) (AliasRealization, error) { + if err := validateAliasPayload(spec.Payload); err != nil { + return 0, err + } if err := ctx.Err(); err != nil { return 0, err } @@ -86,13 +92,20 @@ if err != nil { return 0, err } - switch outcome { + return r.realizeOutcome(ctx, precondition, aliasDecision{spec: spec, outcome: outcome}) +} + +func (r *Replacer) realizeOutcome(ctx context.Context, precondition Precondition, decision aliasDecision) (AliasRealization, error) { + switch decision.outcome { case outcomeCreate: - return AliasCreated, r.commitAlias(ctx, precondition, spec.Payload) + return AliasCreated, r.commitAlias(ctx, precondition, decision.spec.Payload) case outcomeExact: + if err := precondition.Revalidate(); err != nil { + return 0, err + } return AliasExact, nil case outcomeOccupied: - return r.replaceOccupied(ctx, precondition, spec) + return r.replaceOccupied(ctx, precondition, decision.spec) default: return 0, fmt.Errorf("filesystem: alias path %s requires manual intervention", targetPath(precondition.Destination())) } @@ -116,6 +129,9 @@ // durable. Only the rename or a barrier failure can publish a partial // result (PLAN.md Section 7.2 steps 10-11). func (r *Replacer) commitAlias(ctx context.Context, precondition Precondition, payload string) error { + if err := r.prepareAliasParent(precondition); err != nil { + return err + } temp, err := prepareAliasLink(filepath.Dir(targetPath(precondition.Destination())), payload) if err != nil { return err @@ -124,13 +140,37 @@ if err := ctx.Err(); err != nil { return err } + return r.publishAlias(ctx, temp, precondition) +} + +func (r *Replacer) publishAlias(ctx context.Context, temp string, precondition Precondition) error { if err := precondition.Revalidate(); err != nil { + return err + } + if err := walkParentsValid(precondition.Destination().Root, precondition.Destination().Relative); err != nil { return err } if err := r.rename(temp, targetPath(precondition.Destination())); err != nil { return err } return r.syncer.Sync(ctx, filepath.Dir(targetPath(precondition.Destination()))) +} + +func (r *Replacer) prepareAliasParent(precondition Precondition) error { + destination := precondition.Destination() + if err := ensureParents(destination.Root, destination.Relative); err != nil { + return err + } + return walkParentsValid(destination.Root, destination.Relative) +} + +// validateAliasPayload accepts only the exact lexical form that may be stored +// in a symlink. In particular, cleaned or absolute paths are not equivalent. +func validateAliasPayload(payload string) error { + if _, err := pathsafe.Segments(payload); err != nil { + return fmt.Errorf("filesystem: invalid alias payload: %w", err) + } + return nil } // prepareAliasLink reserves a unique name in dir, then creates a relative diff --git a/internal/filesystem/mode.go b/internal/filesystem/mode.go --- a/internal/filesystem/mode.go +++ b/internal/filesystem/mode.go @@ -5,8 +5,6 @@ "fmt" "io/fs" "os" - "path/filepath" - "syscall" ) // OrdinaryTargetMode derives the mode for an ordinary target: read/write @@ -28,10 +26,9 @@ return 0o600 } -// ApplyMode adjusts an existing regular target to the desired mode without -// ever mutating an unmanaged inode alias: a singly linked target is chmod'd -// in place, while a multiply linked target is replaced by a fresh -// same-content entry carrying the desired mode. +// ApplyMode rematerializes the target even when it has one link. This avoids a +// chmod race and makes mode-only corrections obey the same identity and atomic +// publication rules as content changes. func (r *Replacer) ApplyMode(ctx context.Context, precondition Precondition, desired fs.FileMode) error { if err := ctx.Err(); err != nil { return err @@ -39,49 +36,11 @@ if err := precondition.Revalidate(); err != nil { return err } - path := targetPath(precondition.Destination()) - links, err := linkCount(path) - if err != nil { - return err - } - if links <= 1 { - if err := applyChmod(path, desired); err != nil { - return err - } - return r.syncer.Sync(ctx, filepath.Dir(path)) - } - return r.replaceLinkedTarget(ctx, precondition, desired) -} - -// replaceLinkedTarget rewrites a multiply linked target with the same bytes -// and the desired mode so no other link is mutated. -func (r *Replacer) replaceLinkedTarget(ctx context.Context, precondition Precondition, desired fs.FileMode) error { content, err := readTargetContent(targetPath(precondition.Destination())) if err != nil { return err } return r.Replace(ctx, precondition, ReplacementSpec{Content: content, Mode: desired}) -} - -// applyChmod applies the desired mode to the target in place. -func applyChmod(path string, mode fs.FileMode) error { - if err := os.Chmod(path, mode); err != nil { - return fmt.Errorf("filesystem: mode target %s: %w", path, err) - } - return nil -} - -// linkCount reports how many hard links name the entry; one means no alias. -func linkCount(path string) (uint64, error) { - info, err := os.Stat(path) - if err != nil { - return 0, fmt.Errorf("filesystem: stat links %s: %w", path, err) - } - stat, ok := info.Sys().(*syscall.Stat_t) - if !ok { - return 0, fmt.Errorf("filesystem: unsupported stat layout for %s", path) - } - return uint64(stat.Nlink), nil } func readTargetContent(path string) ([]byte, error) { diff --git a/internal/filesystem/mode_test.go b/internal/filesystem/mode_test.go --- a/internal/filesystem/mode_test.go +++ b/internal/filesystem/mode_test.go @@ -85,8 +85,14 @@ replacer := NewReplacer() must(t, replacer.ApplyMode(context.Background(), precondition, 0o755)) after := mustCapture(t, target) - if !pathsafe.SameIdentity(before.Identity(), after.Identity()) { - t.Fatal("mode-only correction must preserve the target identity") + if before.Identity().Path() != after.Identity().Path() { + t.Fatal("mode-only correction changed the destination path") + } + if before.Identity().Path() == "" { + t.Fatal("mode-only correction lost the target identity") + } + if pathsafe.SameIdentity(before.Identity(), after.Identity()) { + t.Fatal("mode-only correction must rematerialize the target") } if after.Mode() != 0o755 { t.Fatalf("mode = %04o, want 0755", after.Mode()) diff --git a/internal/filesystem/parents.go b/internal/filesystem/parents.go --- a/internal/filesystem/parents.go +++ b/internal/filesystem/parents.go @@ -42,6 +42,48 @@ return nil } +// ensureParents creates only missing parent components. Mkdir is deliberately +// followed by a complete walk: an EEXIST result may mean that another process +// installed a symlink instead of the directory we need. +func ensureParents(root, relative string) error { + segments, err := pathsafe.Segments(relative) + if err != nil { + return err + } + if err := requireDir(root); err != nil { + return err + } + current := root + for _, segment := range segments[:len(segments)-1] { + current = filepath.Join(current, segment) + if err := ensureParent(root, relative, current); err != nil { + return err + } + } + return walkParentsValid(root, relative) +} + +func ensureParent(root, relative, current string) error { + info, err := os.Lstat(current) + if errors.Is(err, fs.ErrNotExist) { + if err := createParent(current); err != nil { + return err + } + return walkParentsValid(root, relative) + } + if err != nil { + return err + } + return requireDirEntry(current, info) +} + +func createParent(path string) error { + if err := os.Mkdir(path, 0o755); err != nil && !errors.Is(err, fs.ErrExist) { + return fmt.Errorf("filesystem: create parent %s: %w", path, err) + } + return nil +} + func requireDir(path string) error { info, err := os.Lstat(path) if err != nil { diff --git a/internal/filesystem/race_test.go b/internal/filesystem/race_test.go new file mode 100644 --- /dev/null +++ b/internal/filesystem/race_test.go @@ -0,0 +1,65 @@ +package filesystem + +import ( + "context" + "os" + "path/filepath" + "testing" +) + +func TestInvalidAliasPayload(t *testing.T) { + root := t.TempDir() + must(t, os.Mkdir(filepath.Join(root, ".config"), 0o755)) + precondition := mustFreeze(t, root, ".config/x") + for _, payload := range []string{"", ".", "../x", "a/../x", "/absolute"} { + if _, err := NewReplacer().RealizeAlias(context.Background(), precondition, AliasSpec{Payload: payload}); err == nil { + t.Fatalf("payload %q must be rejected", payload) + } + } +} + +func testDestinationRaceBeforeRename(t *testing.T) { + root := t.TempDir() + precondition := mustFreeze(t, root, "app.conf") + created := false + replacer := &Replacer{ + create: func(dir, pattern string) (TempFile, SyncHandle, error) { + file, handle, _, err := realTempOpener(dir, pattern) + if err == nil { + must(t, os.WriteFile(filepath.Join(root, "app.conf"), []byte("raced"), 0o600)) + created = true + } + return file, handle, err + }, + rename: os.Rename, + remove: os.Remove, + syncer: NewDirectorySyncer(), + } + mustFail(t, replacer.Replace(context.Background(), precondition, ReplacementSpec{Content: []byte("new"), Mode: 0o600})) + if !created || readFile(t, filepath.Join(root, "app.conf")) != "raced" { + t.Fatal("destination race was not preserved") + } +} + +func testMissingParentSymlinkRace(t *testing.T) { + root := t.TempDir() + precondition := mustFreeze(t, root, "nested/app.conf") + moved := filepath.Join(root, "moved") + must(t, os.Mkdir(moved, 0o755)) + replacer := &Replacer{ + create: func(dir, pattern string) (TempFile, SyncHandle, error) { + parent := filepath.Join(root, "nested") + must(t, os.Remove(parent)) + must(t, os.Symlink(moved, parent)) + temp, handle, _, err := realTempOpener(dir, pattern) + return temp, handle, err + }, + rename: os.Rename, + remove: os.Remove, + syncer: NewDirectorySyncer(), + } + mustFail(t, replacer.Replace(context.Background(), precondition, ReplacementSpec{Content: []byte("new"), Mode: 0o600})) + if _, err := os.Lstat(filepath.Join(moved, "app.conf")); err == nil { + t.Fatal("replacement escaped through raced symlink parent") + } +} diff --git a/internal/filesystem/replace.go b/internal/filesystem/replace.go --- a/internal/filesystem/replace.go +++ b/internal/filesystem/replace.go @@ -25,6 +25,24 @@ Mode fs.FileMode } +// ReplaceResult records publication facts independently of the returned +// error. Renamed is true once the old destination may have been replaced; +// DirectorySynced distinguishes a fully durable result from a partial one. +type ReplaceResult struct { + Renamed bool + DirectorySynced bool +} + +// ReplaceError preserves the publication facts when Replace returns an +// error. Unwrap keeps existing errors.As/errors.Is callers working. +type ReplaceError struct { + Result ReplaceResult + Cause error +} + +func (e *ReplaceError) Error() string { return e.Cause.Error() } +func (e *ReplaceError) Unwrap() error { return e.Cause } + // temporaryFile pairs the write path with the sync/close lifecycle of one // open replacement entry. type temporaryFile struct { @@ -103,39 +121,85 @@ // intact and removes the temporary entry; only a rename or barrier failure // can publish a partial result. func (r *Replacer) Replace(ctx context.Context, precondition Precondition, spec ReplacementSpec) error { + _, err := r.ReplaceResult(ctx, precondition, spec) + return err +} + +// ReplaceResult performs Replace while exposing partial durability facts. The +// error-only Replace method above preserves the original package seam. +func (r *Replacer) ReplaceResult(ctx context.Context, precondition Precondition, spec ReplacementSpec) (ReplaceResult, error) { + file, err := r.prepareReplacement(ctx, precondition, spec) + if err != nil { + return ReplaceResult{}, err + } + published, err := r.publish(ctx, &file, precondition) + if err != nil { + return published, &ReplaceError{Result: published, Cause: err} + } + return published, nil +} + +func (r *Replacer) prepareReplacement(ctx context.Context, precondition Precondition, spec ReplacementSpec) (temporaryFile, error) { if err := ctx.Err(); err != nil { - return err + return temporaryFile{}, err + } + if err := ensureParents(precondition.Destination().Root, precondition.Destination().Relative); err != nil { + return temporaryFile{}, err } if err := precondition.Revalidate(); err != nil { - return err + return temporaryFile{}, err } dir := filepath.Dir(targetPath(precondition.Destination())) temp, handle, err := r.create(dir, ".replacement-*") if err != nil { - return err + return temporaryFile{}, err } file := temporaryFile{temp: temp, handle: handle} if err := file.prepare(ctx, spec); err != nil { r.discard(&file) - return err + return temporaryFile{}, err } - return r.publish(ctx, &file, precondition.Destination()) + return file, nil } // publish makes the committed entry the target: it renames the temporary // entry over the destination, then makes the parent directory durable. // Cancellation before the rename removes the entry; a rename or barrier // failure is the only partial result. -func (r *Replacer) publish(ctx context.Context, file *temporaryFile, destination Destination) error { +func (r *Replacer) publish(ctx context.Context, file *temporaryFile, precondition Precondition) (ReplaceResult, error) { + var result ReplaceResult + destination := precondition.Destination() if err := ctx.Err(); err != nil { r.discard(file) - return err + return result, err + } + // This is intentionally the last destination check before publication. + if err := validatePublication(precondition); err != nil { + r.discard(file) + return result, err } if err := r.rename(file.temp.Name(), targetPath(destination)); err != nil { r.discard(file) + return result, err + } + result.Renamed = true + return r.syncPublication(ctx, destination, result) +} + +func validatePublication(precondition Precondition) error { + destination := precondition.Destination() + if err := walkParentsValid(destination.Root, destination.Relative); err != nil { return err } - return r.syncer.Sync(ctx, filepath.Dir(targetPath(destination))) + return precondition.Revalidate() +} + +func (r *Replacer) syncPublication(ctx context.Context, destination Destination, result ReplaceResult) (ReplaceResult, error) { + _, err := r.syncer.SyncResult(ctx, filepath.Dir(targetPath(destination))) + if err == nil { + result.DirectorySynced = true + } + return result, err } // discard removes a temporary entry that must not reach the target. The diff --git a/internal/filesystem/replace_failure_test.go b/internal/filesystem/replace_failure_test.go --- a/internal/filesystem/replace_failure_test.go +++ b/internal/filesystem/replace_failure_test.go @@ -173,6 +173,10 @@ if !errors.As(err, &syncErr) || !errors.Is(syncErr, syscall.EIO) { t.Fatalf("err = %v, want *SyncError wrapping EIO", err) } + var partial *ReplaceError + if !errors.As(err, &partial) || !partial.Result.Renamed || partial.Result.DirectorySynced { + t.Fatalf("result = %+v, want renamed but unsynced", partial.Result) + } if content := readFile(t, target); content != "new" { t.Fatalf("target = %q, want renamed content", content) } diff --git a/internal/filesystem/replace_test.go b/internal/filesystem/replace_test.go --- a/internal/filesystem/replace_test.go +++ b/internal/filesystem/replace_test.go @@ -80,6 +80,8 @@ {"preserves the old target on sync failure", testSyncFailurePreservesOldTarget}, {"preserves the old target on rename failure", testRenameFailurePreservesOldTarget}, {"reports a partial result on directory barrier failure", testDirectoryBarrierPartial}, + {"revalidates the destination immediately before rename", testDestinationRaceBeforeRename}, + {"rejects a parent changed to a symlink", testMissingParentSymlinkRace}, } for _, scenario := range scenarios { t.Run(scenario.name, scenario.run) diff --git a/internal/filesystem/sync.go b/internal/filesystem/sync.go --- a/internal/filesystem/sync.go +++ b/internal/filesystem/sync.go @@ -15,6 +15,15 @@ Close() error } +// SyncResult records which durability steps completed. It remains useful when +// the operation returns an error: a rename can be known to have happened even +// when its directory barrier failed. +type SyncResult struct { + Opened bool + Synced bool + Closed bool +} + // CommitFile makes a still-open temporary file durable: sync while open so // bytes and final mode precede the barrier, then close. It always closes so // no descriptor leaks; a sync failure is reported because the write is not @@ -39,6 +48,7 @@ // report as a partial operation rather than a racing mutation // (PLAN.md Section 7.2 step 11). type SyncError struct { + Result SyncResult Unsupported bool Op string Cause error @@ -76,21 +86,35 @@ // sync, close. Any failure returns *SyncError; an unsupported sync // (EINVAL/ENOTSUP/EOPNOTSUPP) is flagged separately for diagnostics. func (s *DirectorySyncer) Sync(ctx context.Context, path string) error { + _, err := s.SyncResult(ctx, path) + return err +} + +// SyncResult opens, syncs, and closes a directory while returning the facts of +// the lifecycle. The name is intentionally distinct from Sync for callers that +// still use the original error-only seam. +func (s *DirectorySyncer) SyncResult(ctx context.Context, path string) (SyncResult, error) { + var result SyncResult if err := ctx.Err(); err != nil { - return err + return result, err } handle, err := s.open(path) if err != nil { - return &SyncError{Op: "open", Cause: err} + return result, &SyncError{Op: "open", Cause: err} } + result.Opened = true if err := handle.Sync(); err != nil { _ = handle.Close() - return &SyncError{Op: "sync", Unsupported: unsupportedSync(err), Cause: err} + result.Closed = true + return result, &SyncError{Result: result, Op: "sync", Unsupported: unsupportedSync(err), Cause: err} } + result.Synced = true if err := handle.Close(); err != nil { - return &SyncError{Op: "close", Cause: err} + result.Closed = true + return result, &SyncError{Result: result, Op: "close", Cause: err} } - return nil + result.Closed = true + return result, nil } // unsupportedSync classifies filesystems that refuse directory fsync, which diff --git a/internal/filesystem/sync_test.go b/internal/filesystem/sync_test.go --- a/internal/filesystem/sync_test.go +++ b/internal/filesystem/sync_test.go @@ -79,6 +79,9 @@ if syncErr.Op != "sync" || syncErr.Unsupported { t.Fatalf("op = %q, unsupported = %v, want sync failure", syncErr.Op, syncErr.Unsupported) } + if !syncErr.Result.Opened || syncErr.Result.Synced || !syncErr.Result.Closed { + t.Fatalf("sync result = %+v, want opened/closed without sync", syncErr.Result) + } if !handle.closed { t.Fatal("failed sync must still close the handle") }