From d97d1cf7ae11a20fe261e5d9e4b401aed8dc1193 Mon Sep 17 00:00:00 2001 From: Ayush7614 Date: Sat, 17 Oct 2026 16:37:57 +0530 Subject: [PATCH 1/3] fix(tools): publish file writes atomically instead of truncating in place commitRootedFileContents' overwrite opened the observed file, verified its identity and preimage through the handle, and then truncated and rewrote it in place. The truncation is the last irreversible step, and it happens before the first byte of the replacement is written: a write failure after it - ENOSPC is the canonical case, and no mid-write failure is reportable otherwise - destroyed the working file instead of failing the command: existing.txt holds "observed\n"; the tool writes with a full disk -> write fails -> existing.txt is now empty The same in-place truncation rewrites a hard link sibling's bytes when the replaced name is reached through another link, and it shows every intermediate state to a concurrent reader. The commit now stages the replacement bytes to a side file created through the root handle (pathjail.CreateTemp, the same mechanism the structured patch path publishes with), carries the observed file's permission bits over, re-checks the observed identity against the path as late as the rename allows, and swaps the path over with root.Rename. A create stages the same way and publishes through the exclusive no-replace link, so a raced create is refused rather than replaced, and a create whose bytes cannot complete leaves no file at all. Failures remove the side file and leave the preimage standing, which the new regression tests pin at both the commit layer and the write/edit tools, each side file accounted for. Identity and preimage validation, the rooted-open containment (a parent swapped for a symlink is still refused by the kernel), the errFileChangedDuringWrite surface, and the before-open race hooks are unchanged. --- internal/tools/file_commit.go | 130 ++++++++++++++---- internal/tools/file_commit_test.go | 63 +++++++++ .../tools/file_commit_write_failure_test.go | 128 +++++++++++++++++ 3 files changed, 292 insertions(+), 29 deletions(-) create mode 100644 internal/tools/file_commit_write_failure_test.go diff --git a/internal/tools/file_commit.go b/internal/tools/file_commit.go index 287ef68ed..14db10d97 100644 --- a/internal/tools/file_commit.go +++ b/internal/tools/file_commit.go @@ -5,6 +5,9 @@ import ( "fmt" "io" "os" + "path/filepath" + + "github.com/Twigpine/zero/internal/pathjail" ) var errFileChangedDuringWrite = errors.New("file changed on disk before the write committed") @@ -19,6 +22,17 @@ var fileWriteBeforeCommit func(path string) // the file descriptor directly. var fileWriteStat = func(file *os.File) (os.FileInfo, error) { return file.Stat() } +// fileWriteTempWrite is a deterministic test seam for a failure while the +// replacement bytes are written to the side file, after the observed file's +// identity and preimage have been accepted. Production writes through +// io.WriteString. The failure it stands in for (a full disk, a failing +// device) is exactly the moment a truncate-then-write has already destroyed +// the observed file. +var fileWriteTempWrite = func(file *os.File, content string) error { + _, err := io.WriteString(file, content) + return err +} + // commitRootedFileContents binds an overwrite to the file identity and bytes // that the caller observed, opening every component through an already-open // *os.Root descriptor rather than re-resolving the absolute path. Because the @@ -26,11 +40,17 @@ var fileWriteStat = func(file *os.File) (os.FileInfo, error) { return file.Stat( // swapped for a symlink that escapes the workspace between validation and this // call is refused by the kernel instead of redirecting the write. // -// A create uses exclusive creation THROUGH THE ROOT. An overwrite opens the -// observed object without truncation, verifies identity/content through that -// handle, then truncates and writes the same handle. relativePath is the target -// expressed relative to root; absolutePath is retained only for the test hook -// and error reporting. +// Both publishes are atomic against a concurrent reader: the replacement bytes +// are written whole to a side file in the target's directory and the target is +// swapped wholesale. A create publishes through an exclusive link, so losing +// that link refuses instead of replacing the raced file; an overwrite +// publishes through a rename, which hands the path over in one step. A failure +// at any point before the publish removes the side file and leaves the +// observed bytes standing — the caller's file outlives a write that could not +// complete, and no partial state is ever visible at the path itself. +// +// relativePath is the target expressed relative to root; absolutePath is +// retained only for the test hook and error reporting. // // expectedInfo nil means the caller observed a missing path. expectedContent // may be nil for an existing but unreadable file; that path may still be @@ -41,16 +61,7 @@ func commitRootedFileContents(root *os.Root, absolutePath, relativePath string, } if expectedInfo == nil { - file, err := root.OpenFile(relativePath, os.O_WRONLY|os.O_CREATE|os.O_EXCL, 0o644) - if err != nil { - return err - } - openedInfo, err := fileWriteStat(file) - if err != nil { - _ = file.Close() - return err - } - return writeAndVerifyRootedFileIdentity(root, relativePath, file, openedInfo, content, false) + return commitRootedFileCreate(root, relativePath, content) } flags := os.O_WRONLY @@ -86,30 +97,91 @@ func commitRootedFileContents(root *os.Root, absolutePath, relativePath string, return errFileChangedDuringWrite } } - return writeAndVerifyRootedFileIdentity(root, relativePath, file, openedInfo, content, true) + mode := openedInfo.Mode().Perm() + if err := file.Close(); err != nil { + return err + } + return commitRootedFileOverwrite(root, relativePath, openedInfo, mode, content) } -func writeAndVerifyRootedFileIdentity(root *os.Root, relativePath string, file *os.File, openedInfo os.FileInfo, content string, truncate bool) error { - if truncate { - if err := file.Truncate(0); err != nil { - _ = file.Close() - return err - } - if _, err := file.Seek(0, io.SeekStart); err != nil { - _ = file.Close() - return err - } +// commitRootedFileCreate publishes a brand-new file whose absence the caller +// observed: the bytes are written to a side file beside the target, which an +// exclusive link then attaches at the path without ever replacing anything. +// A race that creates the target first is refused with the exclusive open's +// own error, and a parent swapped for a symlink that escapes the workspace +// between validation and the publish is refused by the kernel, both the same +// way the exclusive create open they replace was. +func commitRootedFileCreate(root *os.Root, relativePath, content string) error { + tempName, err := writeRootedSideFile(root, relativePath, 0o644, content) + if err != nil { + return err } - if _, err := io.WriteString(file, content); err != nil { - _ = file.Close() + if _, err := publishStructuredPatchNoReplace(root, tempName, relativePath, 0o644); err != nil { + _ = removeWriteSideFile(root, tempName) return err } - if err := file.Close(); err != nil { + return nil +} + +// commitRootedFileOverwrite publishes a replacement for an object whose +// identity and preimage just passed validation: the bytes are written to a +// side file beside the target, the observed identity is re-checked against the +// path as late as the rename allows, and a rename that runs through the root +// handle hands the path over in one step. A failure while the side file is +// being written — ENOSPC, a failing device — therefore costs a removed side +// file, not the observed file. The publish carries the observed file's +// permission bits over so the published file looks like the file it replaced, +// and the rename detaches this name from an inode shared by hard-link +// siblings instead of truncating the bytes out from under them. +func commitRootedFileOverwrite(root *os.Root, relativePath string, openedInfo os.FileInfo, mode os.FileMode, content string) error { + tempName, err := writeRootedSideFile(root, relativePath, mode, content) + if err != nil { return err } pathInfo, err := root.Stat(relativePath) if err != nil || !os.SameFile(openedInfo, pathInfo) { + _ = removeWriteSideFile(root, tempName) return fmt.Errorf("%w: path identity changed", errFileChangedDuringWrite) } + if err := root.Rename(tempName, relativePath); err != nil { + _ = removeWriteSideFile(root, tempName) + return fmt.Errorf("writing %s: %w", relativePath, err) + } return nil } + +// writeRootedSideFile writes content to a fresh side file beside the target, +// relative to the root handle, and leaves it closed and ready to publish. The +// caller owns removing the side file when the publish that follows does not +// commit. mode carries the permission bits the published file should have, +// because the side file is created with its own restrictive mode so a planted +// link at a predictable name would carry nothing usable. +func writeRootedSideFile(root *os.Root, relativePath string, mode os.FileMode, content string) (string, error) { + temp, tempName, err := pathjail.CreateTemp(root, filepath.Dir(relativePath), "zero-write", ".tmp") + if err != nil { + return "", fmt.Errorf("writing %s: %w", relativePath, err) + } + writeErr := func() error { + if err := fileWriteTempWrite(temp, content); err != nil { + _ = temp.Close() + return err + } + if err := temp.Chmod(mode.Perm()); err != nil { + _ = temp.Close() + return err + } + return temp.Close() + }() + if writeErr != nil { + _ = removeWriteSideFile(root, tempName) + return "", fmt.Errorf("writing %s: %w", relativePath, writeErr) + } + return tempName, nil +} + +// removeWriteSideFile removes a side file that never reached (or outlived) its +// publish, through the root handle. It shares the implementation with the +// structured patch side files. +func removeWriteSideFile(root *os.Root, name string) error { + return removeStructuredPatchTemp(root, name) +} diff --git a/internal/tools/file_commit_test.go b/internal/tools/file_commit_test.go index c6baa21b7..5f3527677 100644 --- a/internal/tools/file_commit_test.go +++ b/internal/tools/file_commit_test.go @@ -350,3 +350,66 @@ func TestWriteFileRefusesSameNameSubstitutionWithIdenticalBytes(t *testing.T) { t.Fatalf("substituted object = %q, err=%v; the write must not land in an object the preimage did not observe", got, err) } } + +// AN OVERWRITTEN PATH'S NEIGHBOURS THROUGH ANOTHER NAME KEEP THEIR CONTENT. +// +// The replace-overwrite truncates and rewrites the observed file in place, and +// truncation follows the inode: a sibling reached through a hard link loses +// its content when this path's content is replaced, because both names were +// pointing at the same bytes. Publishing through a rename detaches this name +// from the shared inode instead, so the link sibling keeps what it shares. +func TestOverwriteDoesNotTruncateAHardlinkSibling(t *testing.T) { + root := t.TempDir() + target := filepath.Join(root, "target.txt") + sibling := filepath.Join(root, "sibling.txt") + if err := os.WriteFile(target, []byte("shared\n"), 0o644); err != nil { + t.Fatal(err) + } + if err := os.Link(target, sibling); err != nil { + t.Skipf("hard links unavailable: %v", err) + } + + result := NewScopedWriteFileTool(root, nil).Run(context.Background(), map[string]any{ + "path": "target.txt", "content": "replaced\n", "overwrite": true, + }) + if result.Status != StatusOK { + t.Fatalf("overwrite status = %s: %s", result.Status, result.Output) + } + shared, err := os.ReadFile(sibling) + if err != nil || string(shared) != "shared\n" { + t.Fatalf("hard-link sibling = %q, err=%v; replacing this name must not rewrite the bytes the sibling shares", shared, err) + } +} + +// THE PUBLISHED FILE LOOKS LIKE THE FILE IT REPLACED. The permission bits are +// carried over from the file that was observed, not from the side file the +// bytes were staged in. +func TestOverwriteKeepsTheObservedPermissionBits(t *testing.T) { + want := os.FileMode(0o600) + if runtime.GOOS == "windows" { + // A Windows file carries only the read-only bit, so a 0o600 file is + // reported as 0o666; the write bit, not the exact value, is the fact + // being pinned. + want = os.FileMode(0o666) + } + + root := t.TempDir() + target := filepath.Join(root, "existing.txt") + if err := os.WriteFile(target, []byte("observed\n"), want); err != nil { + t.Fatal(err) + } + + result := NewScopedWriteFileTool(root, nil).Run(context.Background(), map[string]any{ + "path": "existing.txt", "content": "zero\n", "overwrite": true, + }) + if result.Status != StatusOK { + t.Fatalf("overwrite status = %s: %s", result.Status, result.Output) + } + info, err := os.Stat(target) + if err != nil { + t.Fatal(err) + } + if got := info.Mode().Perm(); got != want { + t.Fatalf("published permission bits = %o, want %o", got, want) + } +} diff --git a/internal/tools/file_commit_write_failure_test.go b/internal/tools/file_commit_write_failure_test.go new file mode 100644 index 000000000..640151314 --- /dev/null +++ b/internal/tools/file_commit_write_failure_test.go @@ -0,0 +1,128 @@ +package tools + +import ( + "context" + "errors" + "os" + "path/filepath" + "testing" +) + +func installFileWriteTempWrite(t *testing.T, write func(*os.File, string) error) { + t.Helper() + prior := fileWriteTempWrite + fileWriteTempWrite = write + t.Cleanup(func() { fileWriteTempWrite = prior }) +} + +// A WRITE THAT CANNOT COMPLETE MUST NOT TAKE THE FILE IT REPLACED WITH IT. +// +// Running out of disk space part-way through a write used to be fatal to the +// preimage: the overwrite truncated the observed file first, so the failed +// write left an empty husk where a working file stood. The replacement bytes +// are now written whole to a side file, so a failure there costs the side file +// and the observed bytes survive untouched. Each case reopens the target after +// the tool has run and checks the preimage is what still stands, and that no +// side file was left behind. +func TestOverwriteFailureKeepsThePreimageStanding(t *testing.T) { + for name, run := range map[string]func(string) Result{ + "write overwrite": func(root string) Result { + return NewScopedWriteFileTool(root, nil).Run(context.Background(), map[string]any{ + "path": "existing.txt", "content": "zero\n", "overwrite": true, + }) + }, + "edit": func(root string) Result { + return NewScopedEditFileTool(root, nil).Run(context.Background(), map[string]any{ + "path": "existing.txt", "old_string": "observed", "new_string": "zero", + }) + }, + } { + t.Run(name, func(t *testing.T) { + root := t.TempDir() + target := filepath.Join(root, "existing.txt") + if err := os.WriteFile(target, []byte("observed\n"), 0o644); err != nil { + t.Fatal(err) + } + // The write that stands in for ENOSPC fails after the file was + // observed, accepted and staged — the exact point the old + // truncate-then-write had nothing left to lose. + writeErr := errors.New("injected unrecoverable write failure") + installFileWriteTempWrite(t, func(*os.File, string) error { return writeErr }) + + result := run(root) + if result.Status != StatusError { + t.Fatalf("failed-write overwrite = %s, want error: %s", result.Status, result.Output) + } + got, err := os.ReadFile(target) + if err != nil || string(got) != "observed\n" { + t.Fatalf("preimage = %q, err=%v; the observed file must outlive a write that failed", got, err) + } + entries, err := os.ReadDir(root) + if err != nil { + t.Fatal(err) + } + if len(entries) != 1 || entries[0].Name() != "existing.txt" { + t.Fatalf("root holds %d entries; a failed write must remove its side file", len(entries)) + } + }) + } +} + +// A NEW FILE WHOSE WRITE FAILS MUST NOT APPEAR AT ALL. An exclusive create used +// to leave whatever the write had managed to write behind, half-written and +// complete-looking to anything that did not read past the visible length. +func TestCreateFailureCreatesNothing(t *testing.T) { + root := t.TempDir() + target := filepath.Join(root, "created.txt") + writeErr := errors.New("injected unrecoverable write failure") + installFileWriteTempWrite(t, func(*os.File, string) error { return writeErr }) + + result := NewScopedWriteFileTool(root, nil).Run(context.Background(), map[string]any{ + "path": "created.txt", "content": "zero\n", + }) + if result.Status != StatusError { + t.Fatalf("failed-write create = %s, want error: %s", result.Status, result.Output) + } + if _, statErr := os.Stat(target); !os.IsNotExist(statErr) { + t.Fatalf("target = %v; a create whose bytes never made it must leave no file", statErr) + } + entries, err := os.ReadDir(root) + if err != nil { + t.Fatal(err) + } + if len(entries) != 0 { + t.Fatalf("root holds %d entries; a failed create must remove its side file", len(entries)) + } +} + +// AT THE LAYER THE COMMIT RUNS, the injected failure is reported as the +// failure it injected and nothing else, so a caller can distinguish a failed +// write from a refused write. +func TestCommitRootedOverwriteFailedWriteIsTheWriteError(t *testing.T) { + root := t.TempDir() + target := filepath.Join(root, "existing.txt") + if err := os.WriteFile(target, []byte("observed\n"), 0o644); err != nil { + t.Fatal(err) + } + workspace, err := os.OpenRoot(root) + if err != nil { + t.Fatal(err) + } + defer workspace.Close() + info, err := os.Stat(target) + if err != nil { + t.Fatal(err) + } + preimage := "observed\n" + writeErr := errors.New("injected unrecoverable write failure") + installFileWriteTempWrite(t, func(*os.File, string) error { return writeErr }) + + err = commitRootedFileContents(workspace, filepath.Join(root, "existing.txt"), "existing.txt", info, &preimage, "zero\n") + if !errors.Is(err, writeErr) { + t.Fatalf("commit error = %v, want the injected write failure", err) + } + got, readErr := os.ReadFile(target) + if readErr != nil || string(got) != "observed\n" { + t.Fatalf("preimage = %q, err=%v; the observed file must outlive a write that failed", got, readErr) + } +} From 9b994c1903cb6d9527b4443a2f1cc95da46047b6 Mon Sep 17 00:00:00 2001 From: Ayush7614 Date: Sat, 17 Oct 2026 17:24:05 +0530 Subject: [PATCH 2/3] fix(tools): reserve creates through the exclusive open and carry its granted mode Two findings from the CodeRabbit review on the atomic-overwrite PR: - The create arm published through the structured-patch no-replace publisher, whose exclusive-copy fallback copies into the visible target, so a concurrent reader could see partial content under a filesystem that refuses hard links. The create path now reserves the name with the same exclusive create it has always honored and hands the reserved file the finished side-file bytes in one rename, over an empty file this call itself reserved, so a reader sees only the absent file, the empty reservation, or the whole file. Losing the reservation still refuses the raced create, and cleanup after a failure removes only what the same-file identity check proves this call created. - The publish carried the requested permission bits to the side file with a chmod, which never consults the process umask, so a umask-077 process that used to receive a 0o600 file from the exclusive create received 0o644 - readable by every stranger. The reserved create now supplies the mode: the bits the kernel granted its reservation under the caller's umask are observed from the reservation and carried to the side file before the rename, on every platform, with no umask-syscall dance. The structured-patch publisher keeps its own pinned fallback contract; only the file tool's create arm changes. The restrictive-umask subtest fails on the previous code with file_commit_umask_test.go:49: published mode = 0644, want the 0600 the kernel granted the reservation under this umask --- internal/tools/file_commit.go | 67 ++++++++++++++++++++---- internal/tools/file_commit_umask_test.go | 56 ++++++++++++++++++++ 2 files changed, 112 insertions(+), 11 deletions(-) create mode 100644 internal/tools/file_commit_umask_test.go diff --git a/internal/tools/file_commit.go b/internal/tools/file_commit.go index 14db10d97..e07f2d20a 100644 --- a/internal/tools/file_commit.go +++ b/internal/tools/file_commit.go @@ -42,8 +42,9 @@ var fileWriteTempWrite = func(file *os.File, content string) error { // // Both publishes are atomic against a concurrent reader: the replacement bytes // are written whole to a side file in the target's directory and the target is -// swapped wholesale. A create publishes through an exclusive link, so losing -// that link refuses instead of replacing the raced file; an overwrite +// swapped wholesale. A create reserves the name with the exclusive create it +// has always honored and hands it the finished bytes in one rename, so losing +// that reservation refuses instead of replacing the raced file; an overwrite // publishes through a rename, which hands the path over in one step. A failure // at any point before the publish removes the side file and leaves the // observed bytes standing — the caller's file outlives a write that could not @@ -105,20 +106,64 @@ func commitRootedFileContents(root *os.Root, absolutePath, relativePath string, } // commitRootedFileCreate publishes a brand-new file whose absence the caller -// observed: the bytes are written to a side file beside the target, which an -// exclusive link then attaches at the path without ever replacing anything. -// A race that creates the target first is refused with the exclusive open's -// own error, and a parent swapped for a symlink that escapes the workspace -// between validation and the publish is refused by the kernel, both the same -// way the exclusive create open they replace was. +// observed. Nothing the caller's request produces is ever written to the +// target's bytes in pieces: the name is reserved first with the exclusive +// create this path has always honored — empty, refusing to replace a file the +// race between observation and publication created, and receiving from the +// kernel the permission bits the mode and the process umask together grant — +// and the finished bytes are staged on a private side file beside the target +// that the reserved name is then handed in one rename. A concurrent reader +// sees the absent file, the empty reservation, or the whole file. func commitRootedFileCreate(root *os.Root, relativePath, content string) error { - tempName, err := writeRootedSideFile(root, relativePath, 0o644, content) + tempName, err := writeRootedSideFile(root, relativePath, 0o600, content) if err != nil { return err } - if _, err := publishStructuredPatchNoReplace(root, tempName, relativePath, 0o644); err != nil { + reserved, err := root.OpenFile(relativePath, os.O_WRONLY|os.O_CREATE|os.O_EXCL, 0o644) + if err != nil { _ = removeWriteSideFile(root, tempName) - return err + return fmt.Errorf("creating %s: %w", relativePath, err) + } + reservedInfo, err := reserved.Stat() + if err != nil { + _ = reserved.Close() + // The reservation's identity could not be captured, so the reserved + // name cannot be proved to be this call's own empty file and is left + // alone; the failure is reported either way and the side file is + // always removed. + _ = removeWriteSideFile(root, tempName) + return fmt.Errorf("creating %s: %w", relativePath, err) + } + discard := func(cause error) error { + _ = reserved.Close() + joined := []error{cause, removeWriteSideFile(root, tempName)} + // The reservation is this call's artifact only while the name still + // resolves to the file it opened; a name carrying something else + // belongs to the hand that swapped it in and is not touched. + pathInfo, statErr := root.Stat(relativePath) + if statErr == nil && os.SameFile(reservedInfo, pathInfo) { + if cleanupErr := removeWriteSideFile(root, relativePath); cleanupErr != nil { + joined = append(joined, fmt.Errorf("removing the reserved empty file: %w", cleanupErr)) + } + } + return fmt.Errorf("creating %s: %w", relativePath, errors.Join(joined...)) + } + if err := reserved.Close(); err != nil { + return discard(err) + } + // The reserved create just told us the permissions the kernel granted the + // name for this process; publishing them, instead of assigning the mode + // outright, keeps a process umask as effective as it was for the exclusive + // create this step replaces. + if err := root.Chmod(tempName, reservedInfo.Mode().Perm()); err != nil { + return discard(err) + } + pathInfo, err := root.Stat(relativePath) + if err != nil || !os.SameFile(reservedInfo, pathInfo) { + return discard(errFileChangedDuringWrite) + } + if err := root.Rename(tempName, relativePath); err != nil { + return discard(err) } return nil } diff --git a/internal/tools/file_commit_umask_test.go b/internal/tools/file_commit_umask_test.go new file mode 100644 index 000000000..4039f6e48 --- /dev/null +++ b/internal/tools/file_commit_umask_test.go @@ -0,0 +1,56 @@ +//go:build unix + +package tools + +import ( + "context" + "os" + "path/filepath" + "syscall" + "testing" +) + +// THE PUBLISHED FILE CARRIES THE PERMISSIONS THE PROCESS UMASK PRODUCES, NOT +// THE RAW MODE AN ASSIGNMENT WOULD SET. +// +// The create path reserves the target name with the same exclusive create this +// branch of the write path always honored, and the kernel applies the process +// umask to the reserved file it grants. The publish must carry those granted +// bits — taken from the reserved file itself — because a chmod never consults +// the umask: a process that requests tight defaults on new files would find +// every write it publishes shared with the world. Under the restrictive umask +// a plain mode assignment would publish 0o644 where the reservation was +// granted 0o600, so the test fails before the granted bits are carried. +func TestCreatePublishesTheUmaskedPermissionsOfTheReservedCreate(t *testing.T) { + for name, testCase := range map[string]struct { + umask int + want os.FileMode + }{ + "a restrictive umask keeps every stranger out": {umask: 0o077, want: 0o600}, + "a permissive umask keeps the readable bits": {umask: 0o022, want: 0o644}, + } { + t.Run(name, func(t *testing.T) { + previous := syscall.Umask(testCase.umask) + defer syscall.Umask(previous) + root := t.TempDir() + target := filepath.Join(root, "created.txt") + + result := NewScopedWriteFileTool(root, nil).Run(context.Background(), map[string]any{ + "path": "created.txt", "content": "zero\n", + }) + if result.Status != StatusOK { + t.Fatalf("create status = %s: %s", result.Status, result.Output) + } + info, err := os.Stat(target) + if err != nil { + t.Fatal(err) + } + if got := info.Mode().Perm(); got != testCase.want { + t.Fatalf("published mode = %#o, want the %#o the kernel granted the reservation under this umask; a raw mode assignment would publish %#o and share the file with hands the umask excludes", got, testCase.want, os.FileMode(0o644)) + } + if got, readErr := os.ReadFile(target); readErr != nil || string(got) != "zero\n" { + t.Fatalf("published content = %q, err=%v", got, readErr) + } + }) + } +} From 48f65ca6345730b5a833ca5476ebb7c2f74375f9 Mon Sep 17 00:00:00 2001 From: Ayush7614 Date: Tue, 20 Oct 2026 15:12:29 +0530 Subject: [PATCH 3/3] fix(tools): publish creates without replacement and keep linked targets A create now publishes with a kernel no-replace rename, with a no-replace hard-link fallback for filesystems that cannot support it, so losing the create race can no longer replace the winner. This removes the reservation and its identity-sensitive cleanup entirely. An overwrite resolves a final-component symlink and renames over the file the link names, preserving the link as main did. The destination identity is rechecked as the last operation before the rename; when the platform refuses the rename (notably Windows with an open reader), the write falls back to the previous in-place path only after reopening and revalidating the same object and preimage. The staged file now carries the observed overwrite mode rather than the staging mode, and the mode regression test uses a value distinct from the staging mode. --- internal/observability/rename_noreplace.go | 14 + .../observability/rename_noreplace_darwin.go | 19 - .../observability/rename_noreplace_linux.go | 19 - internal/pathjail/pathjail.go | 14 +- internal/pathjail/rename_noreplace.go | 28 ++ internal/pathjail/rename_noreplace_darwin.go | 22 + internal/pathjail/rename_noreplace_linux.go | 22 + .../rename_noreplace_other.go | 2 +- internal/pathjail/rename_noreplace_test.go | 79 ++++ .../rename_noreplace_windows.go | 2 +- internal/tools/file_commit.go | 405 ++++++++++++++---- internal/tools/file_commit_publish_test.go | 283 ++++++++++++ internal/tools/file_commit_test.go | 6 +- internal/tools/file_commit_umask_test.go | 20 +- 14 files changed, 792 insertions(+), 143 deletions(-) create mode 100644 internal/observability/rename_noreplace.go delete mode 100644 internal/observability/rename_noreplace_darwin.go delete mode 100644 internal/observability/rename_noreplace_linux.go create mode 100644 internal/pathjail/rename_noreplace.go create mode 100644 internal/pathjail/rename_noreplace_darwin.go create mode 100644 internal/pathjail/rename_noreplace_linux.go rename internal/{observability => pathjail}/rename_noreplace_other.go (87%) create mode 100644 internal/pathjail/rename_noreplace_test.go rename internal/{observability => pathjail}/rename_noreplace_windows.go (99%) create mode 100644 internal/tools/file_commit_publish_test.go diff --git a/internal/observability/rename_noreplace.go b/internal/observability/rename_noreplace.go new file mode 100644 index 000000000..ef715cddc --- /dev/null +++ b/internal/observability/rename_noreplace.go @@ -0,0 +1,14 @@ +package observability + +import ( + "os" + + "github.com/Twigpine/zero/internal/pathjail" +) + +// renameNoReplace publishes a crash report without replacing an existing one. +// The implementation lives in pathjail because the file tools need the same +// atomic no-replace guarantee. +func renameNoReplace(root *os.Root, oldname, newname string) error { + return pathjail.RenameNoReplace(root, oldname, newname) +} diff --git a/internal/observability/rename_noreplace_darwin.go b/internal/observability/rename_noreplace_darwin.go deleted file mode 100644 index 7be01852b..000000000 --- a/internal/observability/rename_noreplace_darwin.go +++ /dev/null @@ -1,19 +0,0 @@ -//go:build darwin - -package observability - -import ( - "fmt" - "os" - - "golang.org/x/sys/unix" -) - -func renameNoReplace(root *os.Root, oldname, newname string) error { - directory, err := root.Open(".") - if err != nil { - return fmt.Errorf("open crash directory for publication: %w", err) - } - defer directory.Close() - return unix.RenameatxNp(int(directory.Fd()), oldname, int(directory.Fd()), newname, unix.RENAME_EXCL) -} diff --git a/internal/observability/rename_noreplace_linux.go b/internal/observability/rename_noreplace_linux.go deleted file mode 100644 index bab6dc4b8..000000000 --- a/internal/observability/rename_noreplace_linux.go +++ /dev/null @@ -1,19 +0,0 @@ -//go:build linux - -package observability - -import ( - "fmt" - "os" - - "golang.org/x/sys/unix" -) - -func renameNoReplace(root *os.Root, oldname, newname string) error { - directory, err := root.Open(".") - if err != nil { - return fmt.Errorf("open crash directory for publication: %w", err) - } - defer directory.Close() - return unix.Renameat2(int(directory.Fd()), oldname, int(directory.Fd()), newname, unix.RENAME_NOREPLACE) -} diff --git a/internal/pathjail/pathjail.go b/internal/pathjail/pathjail.go index 407dc71c3..af5ec598a 100644 --- a/internal/pathjail/pathjail.go +++ b/internal/pathjail/pathjail.go @@ -142,12 +142,22 @@ func trimTrailingSeparators(path string) string { var randomBytes = rand.Read // CreateTemp makes an O_EXCL temp file named prefix.suffix inside dir, -// relative to handle, and returns it with its relative path. +// relative to handle, and returns it with its relative path. The file is +// created 0o600 so bytes staged in it are not readable before publication. // // O_EXCL rather than a fixed name: a link planted at a predictable temp path // between the check and the create is the same race this package exists to // close, and O_EXCL makes the kernel refuse it rather than us. func CreateTemp(handle *os.Root, dir, prefix, suffix string) (*os.File, string, error) { + return CreateTempMode(handle, dir, prefix, suffix, 0o600) +} + +// CreateTempMode is CreateTemp with an explicit create mode. The mode is +// passed to the kernel's exclusive create, so the process umask still applies; +// callers that must publish the permissions a normal create would have been +// granted should read the returned file's mode rather than assigning perm +// later, because Chmod does not consult the umask. +func CreateTempMode(handle *os.Root, dir, prefix, suffix string, perm os.FileMode) (*os.File, string, error) { // prefix and suffix are NAME fragments, not path fragments. A separator or a // "."/".." in either would relocate the file out of dir — a prefix of // "../escaped", or a suffix of ".tmp/../evil" that also erases the random @@ -164,7 +174,7 @@ func CreateTemp(handle *os.Root, dir, prefix, suffix string) (*os.File, string, return nil, "", fmt.Errorf("generate a temporary name: %w", err) } relative := filepath.Join(dir, prefix+"."+hex.EncodeToString(noise)+suffix) - file, err := handle.OpenFile(relative, os.O_RDWR|os.O_CREATE|os.O_EXCL, 0o600) + file, err := handle.OpenFile(relative, os.O_RDWR|os.O_CREATE|os.O_EXCL, perm) if err == nil { return file, relative, nil } diff --git a/internal/pathjail/rename_noreplace.go b/internal/pathjail/rename_noreplace.go new file mode 100644 index 000000000..adc4eacc0 --- /dev/null +++ b/internal/pathjail/rename_noreplace.go @@ -0,0 +1,28 @@ +package pathjail + +import ( + "fmt" + "os" + "path/filepath" +) + +// RenameNoReplace renames oldname to newname within the directory bound by +// root, and refuses to replace an existing newname with the same atomic +// guarantee the kernel gives a create's exclusive open. A caller that loses +// the race receives fs.ErrExist and nothing at newname changes. +// +// Both names must be single path components: they are resolved relative to +// root's directory handle, so a separator (or an ancestor link) must not be +// able to redirect the syscall outside the checked root. Callers with a path +// in a subdirectory open that directory with Root.OpenRoot first. +// +// Platforms whose kernel cannot publish without replacing fall back to +// ErrUnsupported, which lets the caller use a no-replace hard link instead. +func RenameNoReplace(root *os.Root, oldname, newname string) error { + for _, name := range []string{oldname, newname} { + if name == "" || name == "." || name == ".." || filepath.Base(name) != name { + return fmt.Errorf("%w: rename name %q must be a single path component", ErrEscapes, name) + } + } + return renameNoReplace(root, oldname, newname) +} diff --git a/internal/pathjail/rename_noreplace_darwin.go b/internal/pathjail/rename_noreplace_darwin.go new file mode 100644 index 000000000..7866d939b --- /dev/null +++ b/internal/pathjail/rename_noreplace_darwin.go @@ -0,0 +1,22 @@ +//go:build darwin + +package pathjail + +import ( + "fmt" + "os" + + "golang.org/x/sys/unix" +) + +func renameNoReplace(root *os.Root, oldname, newname string) error { + directory, err := root.Open(".") + if err != nil { + return fmt.Errorf("open directory for rename: %w", err) + } + defer directory.Close() + if err := unix.RenameatxNp(int(directory.Fd()), oldname, int(directory.Fd()), newname, unix.RENAME_EXCL); err != nil { + return fmt.Errorf("rename %s to %s: %w", oldname, newname, err) + } + return nil +} diff --git a/internal/pathjail/rename_noreplace_linux.go b/internal/pathjail/rename_noreplace_linux.go new file mode 100644 index 000000000..4adb2d599 --- /dev/null +++ b/internal/pathjail/rename_noreplace_linux.go @@ -0,0 +1,22 @@ +//go:build linux + +package pathjail + +import ( + "fmt" + "os" + + "golang.org/x/sys/unix" +) + +func renameNoReplace(root *os.Root, oldname, newname string) error { + directory, err := root.Open(".") + if err != nil { + return fmt.Errorf("open directory for rename: %w", err) + } + defer directory.Close() + if err := unix.Renameat2(int(directory.Fd()), oldname, int(directory.Fd()), newname, unix.RENAME_NOREPLACE); err != nil { + return fmt.Errorf("rename %s to %s: %w", oldname, newname, err) + } + return nil +} diff --git a/internal/observability/rename_noreplace_other.go b/internal/pathjail/rename_noreplace_other.go similarity index 87% rename from internal/observability/rename_noreplace_other.go rename to internal/pathjail/rename_noreplace_other.go index ee43bc1b2..4839ad303 100644 --- a/internal/observability/rename_noreplace_other.go +++ b/internal/pathjail/rename_noreplace_other.go @@ -1,6 +1,6 @@ //go:build !darwin && !linux && !windows -package observability +package pathjail import ( "errors" diff --git a/internal/pathjail/rename_noreplace_test.go b/internal/pathjail/rename_noreplace_test.go new file mode 100644 index 000000000..223d14d2d --- /dev/null +++ b/internal/pathjail/rename_noreplace_test.go @@ -0,0 +1,79 @@ +package pathjail + +import ( + "errors" + "os" + "path/filepath" + "testing" +) + +func openRenameRoot(t *testing.T) (*os.Root, string) { + t.Helper() + base := t.TempDir() + root, err := os.OpenRoot(base) + if err != nil { + t.Fatal(err) + } + t.Cleanup(func() { _ = root.Close() }) + return root, base +} + +// A CREATE'S NAME MUST NOT BE LOST TO A RACE. The no-replace rename reports the +// collision as fs.ErrExist and leaves both files exactly as they were, so the +// caller can refuse the create instead of replacing the other writer. +func TestRenameNoReplaceRefusesAnExistingDestination(t *testing.T) { + root, base := openRenameRoot(t) + if err := os.WriteFile(filepath.Join(base, "source"), []byte("source\n"), 0o644); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(base, "target"), []byte("target\n"), 0o644); err != nil { + t.Fatal(err) + } + + if err := RenameNoReplace(root, "source", "target"); !errors.Is(err, os.ErrExist) { + t.Fatalf("RenameNoReplace over an existing name = %v, want os.ErrExist", err) + } + for name, want := range map[string]string{"source": "source\n", "target": "target\n"} { + got, err := os.ReadFile(filepath.Join(base, name)) + if err != nil || string(got) != want { + t.Fatalf("%s = %q, err=%v; a refused rename must not move either file", name, got, err) + } + } +} + +func TestRenameNoReplacePublishesAnAbsentDestination(t *testing.T) { + root, base := openRenameRoot(t) + if err := os.WriteFile(filepath.Join(base, "source"), []byte("whole\n"), 0o644); err != nil { + t.Fatal(err) + } + + if err := RenameNoReplace(root, "source", "target"); err != nil { + t.Fatalf("RenameNoReplace = %v", err) + } + if got, err := os.ReadFile(filepath.Join(base, "target")); err != nil || string(got) != "whole\n" { + t.Fatalf("target = %q, err=%v", got, err) + } + if _, err := os.Stat(filepath.Join(base, "source")); !os.IsNotExist(err) { + t.Fatalf("source survived a successful rename: %v", err) + } +} + +// The syscall is given a directory handle and bare names. Allowing a +// separator here would let an ancestor component resolve outside the directory +// the caller checked, so it is refused before the syscall. +func TestRenameNoReplaceRejectsPathComponents(t *testing.T) { + root, _ := openRenameRoot(t) + for _, tc := range []struct{ oldname, newname string }{ + {"", "target"}, + {"source", ""}, + {".", "target"}, + {"source", ".."}, + {"dir/source", "target"}, + {"source", "dir/target"}, + {filepath.Join("dir", "source"), "target"}, + } { + if err := RenameNoReplace(root, tc.oldname, tc.newname); !errors.Is(err, ErrEscapes) { + t.Errorf("RenameNoReplace(%q, %q) = %v, want ErrEscapes", tc.oldname, tc.newname, err) + } + } +} diff --git a/internal/observability/rename_noreplace_windows.go b/internal/pathjail/rename_noreplace_windows.go similarity index 99% rename from internal/observability/rename_noreplace_windows.go rename to internal/pathjail/rename_noreplace_windows.go index 7c19df596..01baf71c7 100644 --- a/internal/observability/rename_noreplace_windows.go +++ b/internal/pathjail/rename_noreplace_windows.go @@ -1,6 +1,6 @@ //go:build windows -package observability +package pathjail import ( "errors" diff --git a/internal/tools/file_commit.go b/internal/tools/file_commit.go index e07f2d20a..c019b095f 100644 --- a/internal/tools/file_commit.go +++ b/internal/tools/file_commit.go @@ -6,6 +6,8 @@ import ( "io" "os" "path/filepath" + "strings" + "syscall" "github.com/Twigpine/zero/internal/pathjail" ) @@ -17,6 +19,12 @@ var errFileChangedDuringWrite = errors.New("file changed on disk before the writ // object that will actually be mutated. var fileWriteBeforeCommit func(path string) +// fileWriteBeforePublish is a deterministic test hook that runs after the +// replacement bytes have been staged and before the destination's identity is +// re-checked for the publish. Tests use it to install a competing writer at the +// last point before publication. +var fileWriteBeforePublish func(path string) + // fileWriteStat is a deterministic test seam for proving that the opened-file // identity is captured before the final preimage comparison. Production uses // the file descriptor directly. @@ -33,6 +41,17 @@ var fileWriteTempWrite = func(file *os.File, content string) error { return err } +// fileWriteRename publishes the staged side file over the destination. +// Tests replace it to exercise the in-place fallback deterministically. +var fileWriteRename = func(root *os.Root, oldName, newName string) error { + return root.Rename(oldName, newName) +} + +// fileWriteNoReplaceRename publishes a create without replacing an existing +// name. Tests replace it to exercise the hard-link fallback that filesystems +// without a no-replace rename use. +var fileWriteNoReplaceRename = publishRootedNoReplace + // commitRootedFileContents binds an overwrite to the file identity and bytes // that the caller observed, opening every component through an already-open // *os.Root descriptor rather than re-resolving the absolute path. Because the @@ -42,16 +61,26 @@ var fileWriteTempWrite = func(file *os.File, content string) error { // // Both publishes are atomic against a concurrent reader: the replacement bytes // are written whole to a side file in the target's directory and the target is -// swapped wholesale. A create reserves the name with the exclusive create it -// has always honored and hands it the finished bytes in one rename, so losing -// that reservation refuses instead of replacing the raced file; an overwrite -// publishes through a rename, which hands the path over in one step. A failure -// at any point before the publish removes the side file and leaves the -// observed bytes standing — the caller's file outlives a write that could not -// complete, and no partial state is ever visible at the path itself. +// swapped wholesale. A create publishes through a kernel no-replace rename, so +// losing the create race refuses instead of replacing the raced file. An +// overwrite publishes through a rename after re-checking that the destination +// still names the observed object; that check is the last operation before the +// rename, but no supported platform offers a compare-and-swap rename, so a +// writer that replaces the name between the check and the syscall can still be +// replaced. A failure at any point before the publish removes the side file +// and leaves the observed bytes standing — the caller's file outlives a write +// that could not complete, and no partial state is ever visible at the path +// itself. +// +// A final-component symlink keeps main's behaviour: the link stays a link and +// the resolved file is what is replaced. A hard link is detached by the rename +// instead of rewritten through the shared inode. Where the platform refuses a +// rename (notably Windows with an open reader), the write falls back to the +// previous in-place publish through a freshly validated handle; that is the +// only case in which the preimage is not protected by staging. // // relativePath is the target expressed relative to root; absolutePath is -// retained only for the test hook and error reporting. +// retained only for the test hooks and error reporting. // // expectedInfo nil means the caller observed a missing path. expectedContent // may be nil for an existing but unreadable file; that path may still be @@ -98,135 +127,333 @@ func commitRootedFileContents(root *os.Root, absolutePath, relativePath string, return errFileChangedDuringWrite } } + + // A device, FIFO or socket is not a file this path should replace with a + // regular one, and the previous in-place write is the only publish that + // keeps its semantics. Regular files (including symlink targets) go through + // staging and rename. + if !openedInfo.Mode().IsRegular() { + return writeRootedFileInPlace(root, relativePath, file, openedInfo, expectedContent, content) + } mode := openedInfo.Mode().Perm() + publishPath, err := resolveRootedFinalPath(root, relativePath) + if err != nil { + _ = file.Close() + return err + } + if publishPath != relativePath { + targetInfo, statErr := root.Stat(publishPath) + if statErr != nil || !os.SameFile(openedInfo, targetInfo) { + _ = file.Close() + return fmt.Errorf("%w: link target changed", errFileChangedDuringWrite) + } + } if err := file.Close(); err != nil { return err } - return commitRootedFileOverwrite(root, relativePath, openedInfo, mode, content) + return commitRootedFileOverwrite(root, absolutePath, relativePath, publishPath, openedInfo, mode, expectedContent, content) } // commitRootedFileCreate publishes a brand-new file whose absence the caller -// observed. Nothing the caller's request produces is ever written to the -// target's bytes in pieces: the name is reserved first with the exclusive -// create this path has always honored — empty, refusing to replace a file the -// race between observation and publication created, and receiving from the -// kernel the permission bits the mode and the process umask together grant — -// and the finished bytes are staged on a private side file beside the target -// that the reserved name is then handed in one rename. A concurrent reader -// sees the absent file, the empty reservation, or the whole file. +// observed. The bytes are written whole to a private side file beside the +// target and handed to the name with a kernel no-replace rename: the create is +// either absent or the whole file, a concurrent create is refused rather than +// replaced, and a write that cannot complete leaves no file at all. func commitRootedFileCreate(root *os.Root, relativePath, content string) error { - tempName, err := writeRootedSideFile(root, relativePath, 0o600, content) + tempName, err := writeRootedSideFile(root, relativePath, 0o644, true, content) if err != nil { return err } - reserved, err := root.OpenFile(relativePath, os.O_WRONLY|os.O_CREATE|os.O_EXCL, 0o644) - if err != nil { - _ = removeWriteSideFile(root, tempName) - return fmt.Errorf("creating %s: %w", relativePath, err) + if err := publishRootedCreate(root, tempName, relativePath); err != nil { + return errors.Join(err, removeWriteSideFile(root, tempName)) } - reservedInfo, err := reserved.Stat() - if err != nil { - _ = reserved.Close() - // The reservation's identity could not be captured, so the reserved - // name cannot be proved to be this call's own empty file and is left - // alone; the failure is reported either way and the side file is - // always removed. - _ = removeWriteSideFile(root, tempName) - return fmt.Errorf("creating %s: %w", relativePath, err) - } - discard := func(cause error) error { - _ = reserved.Close() - joined := []error{cause, removeWriteSideFile(root, tempName)} - // The reservation is this call's artifact only while the name still - // resolves to the file it opened; a name carrying something else - // belongs to the hand that swapped it in and is not touched. - pathInfo, statErr := root.Stat(relativePath) - if statErr == nil && os.SameFile(reservedInfo, pathInfo) { - if cleanupErr := removeWriteSideFile(root, relativePath); cleanupErr != nil { - joined = append(joined, fmt.Errorf("removing the reserved empty file: %w", cleanupErr)) - } - } - return fmt.Errorf("creating %s: %w", relativePath, errors.Join(joined...)) + return nil +} + +// publishRootedCreate hands the staged side file to a name that must not +// already exist. RenameNoReplace is the primary primitive; a hard link is the +// no-replace fallback for filesystems whose rename cannot promise it. Neither +// fallback copies into a visible target, so a reader sees only the absent name +// or the complete file. +func publishRootedCreate(root *os.Root, tempName, relativePath string) error { + renameErr := fileWriteNoReplaceRename(root, tempName, relativePath) + if renameErr == nil { + return nil } - if err := reserved.Close(); err != nil { - return discard(err) + if errors.Is(renameErr, os.ErrExist) { + return fmt.Errorf("creating %s: %w", relativePath, renameErr) } - // The reserved create just told us the permissions the kernel granted the - // name for this process; publishing them, instead of assigning the mode - // outright, keeps a process umask as effective as it was for the exclusive - // create this step replaces. - if err := root.Chmod(tempName, reservedInfo.Mode().Perm()); err != nil { - return discard(err) + if !renameNoReplaceUnsupported(renameErr) { + return fmt.Errorf("creating %s: %w", relativePath, renameErr) } - pathInfo, err := root.Stat(relativePath) - if err != nil || !os.SameFile(reservedInfo, pathInfo) { - return discard(errFileChangedDuringWrite) + if linkErr := root.Link(tempName, relativePath); linkErr != nil { + return fmt.Errorf("creating %s: %w", relativePath, errors.Join(renameErr, linkErr)) } - if err := root.Rename(tempName, relativePath); err != nil { - return discard(err) + if cleanupErr := removeWriteSideFile(root, tempName); cleanupErr != nil { + return fmt.Errorf("creating %s: published, but removing the staging name failed: %w", relativePath, cleanupErr) } return nil } +// publishRootedNoReplace performs the no-replace rename through a root handle +// bound to the target's own directory. Binding the directory first is what +// keeps the raw rename inside the workspace: a parent link swapped after the +// caller resolved the path is refused when the directory is opened, and both +// names are single components below that handle. +func publishRootedNoReplace(root *os.Root, tempName, relativePath string) error { + parent := filepath.Dir(relativePath) + if filepath.Dir(tempName) != parent { + return fmt.Errorf("staging file %s is not beside %s", tempName, relativePath) + } + directory, err := root.OpenRoot(parent) + if err != nil { + return err + } + defer directory.Close() + return pathjail.RenameNoReplace(directory, filepath.Base(tempName), filepath.Base(relativePath)) +} + +// renameNoReplaceUnsupported reports whether the platform or filesystem told +// us only that it cannot perform a no-replace rename, so the no-replace hard +// link is the right fallback. A collision is not an unsupported operation and +// is handled by the caller before this is consulted. +func renameNoReplaceUnsupported(err error) bool { + return errors.Is(err, errors.ErrUnsupported) || errors.Is(err, syscall.EINVAL) +} + // commitRootedFileOverwrite publishes a replacement for an object whose // identity and preimage just passed validation: the bytes are written to a // side file beside the target, the observed identity is re-checked against the -// path as late as the rename allows, and a rename that runs through the root -// handle hands the path over in one step. A failure while the side file is -// being written — ENOSPC, a failing device — therefore costs a removed side -// file, not the observed file. The publish carries the observed file's -// permission bits over so the published file looks like the file it replaced, -// and the rename detaches this name from an inode shared by hard-link -// siblings instead of truncating the bytes out from under them. -func commitRootedFileOverwrite(root *os.Root, relativePath string, openedInfo os.FileInfo, mode os.FileMode, content string) error { - tempName, err := writeRootedSideFile(root, relativePath, mode, content) +// path as late as the rename allows, and a rename hands the path over in one +// step. A failure while the side file is being written — ENOSPC, a failing +// device — therefore costs a removed side file, not the observed file. +// +// If the platform refuses the rename (most commonly a Windows reader holding +// the destination open), the previous in-place publish is used after reopening +// and re-validating the same object. That fallback cannot be atomic, but it is +// the only way to write a file another process has open without delete +// sharing, and it is what this tool did before staging existed. +func commitRootedFileOverwrite(root *os.Root, absolutePath, relativePath, publishPath string, openedInfo os.FileInfo, mode os.FileMode, expectedContent *string, content string) error { + tempName, err := writeRootedSideFile(root, publishPath, mode, false, content) if err != nil { return err } - pathInfo, err := root.Stat(relativePath) - if err != nil || !os.SameFile(openedInfo, pathInfo) { - _ = removeWriteSideFile(root, tempName) - return fmt.Errorf("%w: path identity changed", errFileChangedDuringWrite) + if fileWriteBeforePublish != nil { + fileWriteBeforePublish(absolutePath) } - if err := root.Rename(tempName, relativePath); err != nil { + if err := verifyRootedPublishTarget(root, publishPath, openedInfo); err != nil { _ = removeWriteSideFile(root, tempName) - return fmt.Errorf("writing %s: %w", relativePath, err) + return err + } + if err := fileWriteRename(root, tempName, publishPath); err != nil { + renameErr := fmt.Errorf("writing %s: atomic replacement refused: %w", relativePath, err) + fallbackErr := rewriteRootedFileInPlace(root, relativePath, openedInfo, expectedContent, content) + cleanupErr := removeWriteSideFile(root, tempName) + if fallbackErr == nil { + if cleanupErr != nil { + return fmt.Errorf("%w: the file was written in place, but removing the staged replacement failed: %v", renameErr, cleanupErr) + } + return nil + } + return errors.Join(renameErr, fmt.Errorf("writing %s: in-place fallback failed: %w", relativePath, fallbackErr), cleanupErr) + } + return nil +} + +// verifyRootedPublishTarget refuses to replace anything but the exact object +// whose identity and preimage were validated. It is deliberately the last +// operation before the rename; no portable rename offers a compare-and-swap, +// so this check is the strongest condition available before an atomic replace. +func verifyRootedPublishTarget(root *os.Root, publishPath string, openedInfo os.FileInfo) error { + info, err := root.Lstat(publishPath) + if err != nil || info.Mode()&(os.ModeSymlink|os.ModeIrregular) != 0 || !os.SameFile(openedInfo, info) { + return fmt.Errorf("%w: path identity changed", errFileChangedDuringWrite) } return nil } +// resolveRootedFinalPath follows final-component symlinks inside the root and +// returns the path of the file the caller's link resolves to. Publishing over +// that path preserves the link, unlike a rename over the link itself. +func resolveRootedFinalPath(root *os.Root, relativePath string) (string, error) { + current := relativePath + for links := 0; links < 255; links++ { + info, err := root.Lstat(current) + if err != nil { + return "", err + } + if info.Mode()&os.ModeSymlink == 0 { + return current, nil + } + target, err := root.Readlink(current) + if err != nil { + return "", err + } + if filepath.IsAbs(target) || filepath.VolumeName(target) != "" { + return "", fmt.Errorf("%w: %s is an absolute link", errFileChangedDuringWrite, current) + } + next := filepath.Clean(filepath.Join(filepath.Dir(current), target)) + if next == ".." || strings.HasPrefix(next, ".."+string(filepath.Separator)) { + return "", fmt.Errorf("%w: %s points outside the workspace", errFileChangedDuringWrite, current) + } + current = next + } + return "", fmt.Errorf("%w: more than 255 links at %s", errFileChangedDuringWrite, relativePath) +} + // writeRootedSideFile writes content to a fresh side file beside the target, // relative to the root handle, and leaves it closed and ready to publish. The // caller owns removing the side file when the publish that follows does not -// commit. mode carries the permission bits the published file should have, -// because the side file is created with its own restrictive mode so a planted -// link at a predictable name would carry nothing usable. -func writeRootedSideFile(root *os.Root, relativePath string, mode os.FileMode, content string) (string, error) { - temp, tempName, err := pathjail.CreateTemp(root, filepath.Dir(relativePath), "zero-write", ".tmp") +// commit. +// +// umaskDerived selects the permission contract. A create asks the kernel for +// 0o644 and remembers what the process umask actually granted; the bytes are +// made owner-only while they are staged, then the granted bits are restored. +// An overwrite stages 0o600 and finally carries the observed file's permission +// bits over, so the published file looks like the one it replaced. +func writeRootedSideFile(root *os.Root, relativePath string, mode os.FileMode, umaskDerived bool, content string) (string, error) { + createMode := mode.Perm() + if !umaskDerived { + createMode = 0o600 + } + temp, tempName, err := pathjail.CreateTempMode(root, filepath.Dir(relativePath), "zero-write", ".tmp", createMode) if err != nil { return "", fmt.Errorf("writing %s: %w", relativePath, err) } - writeErr := func() error { - if err := fileWriteTempWrite(temp, content); err != nil { + granted := mode.Perm() + if umaskDerived { + info, statErr := temp.Stat() + if statErr != nil { _ = temp.Close() - return err + _ = removeWriteSideFile(root, tempName) + return "", fmt.Errorf("writing %s: %w", relativePath, statErr) } - if err := temp.Chmod(mode.Perm()); err != nil { + granted = info.Mode().Perm() + if chmodErr := temp.Chmod(0o600); chmodErr != nil { _ = temp.Close() + _ = removeWriteSideFile(root, tempName) + return "", fmt.Errorf("writing %s: %w", relativePath, chmodErr) + } + } + writeErr := func() error { + if err := fileWriteTempWrite(temp, content); err != nil { return err } - return temp.Close() + return temp.Chmod(granted.Perm()) }() - if writeErr != nil { + closeErr := temp.Close() + if writeErr != nil || closeErr != nil { _ = removeWriteSideFile(root, tempName) - return "", fmt.Errorf("writing %s: %w", relativePath, writeErr) + return "", fmt.Errorf("writing %s: %w", relativePath, errors.Join(writeErr, closeErr)) } return tempName, nil } -// removeWriteSideFile removes a side file that never reached (or outlived) its -// publish, through the root handle. It shares the implementation with the -// structured patch side files. +// writeRootedFileInPlace publishes through an already-validated handle. It is +// the non-regular and rename-refused fallback, so it keeps the previous +// truncate-and-write behaviour and restores the observed bytes if a write +// fails and the preimage is available. +func writeRootedFileInPlace(root *os.Root, relativePath string, file *os.File, openedInfo os.FileInfo, expectedContent *string, content string) error { + if err := file.Truncate(0); err != nil { + _ = file.Close() + return err + } + if _, err := file.Seek(0, io.SeekStart); err != nil { + _ = file.Close() + return err + } + if _, err := io.WriteString(file, content); err != nil { + restoreErr := restoreRootedFile(file, expectedContent) + closeErr := file.Close() + return errors.Join(err, restoreErr, closeErr) + } + if err := file.Close(); err != nil { + return err + } + pathInfo, err := root.Stat(relativePath) + if err != nil || !os.SameFile(openedInfo, pathInfo) { + return fmt.Errorf("%w: path identity changed", errFileChangedDuringWrite) + } + return nil +} + +// rewriteRootedFileInPlace reopens the observed path, re-validates identity +// and preimage through that handle, and only then falls back to an in-place +// write. A replacement installed during the refused publish is therefore not +// touched. +func rewriteRootedFileInPlace(root *os.Root, relativePath string, expectedInfo os.FileInfo, expectedContent *string, content string) error { + flags := os.O_WRONLY + if expectedContent != nil { + flags = os.O_RDWR + } + file, err := root.OpenFile(relativePath, flags, 0) + if err != nil { + return err + } + openedInfo, err := file.Stat() + if err != nil { + _ = file.Close() + return err + } + if !os.SameFile(expectedInfo, openedInfo) { + _ = file.Close() + return errFileChangedDuringWrite + } + pathInfo, err := root.Stat(relativePath) + if err != nil || !os.SameFile(openedInfo, pathInfo) { + _ = file.Close() + return errFileChangedDuringWrite + } + if expectedContent != nil { + current, readErr := io.ReadAll(file) + if readErr != nil { + _ = file.Close() + return readErr + } + if string(current) != *expectedContent { + _ = file.Close() + return errFileChangedDuringWrite + } + } + return writeRootedFileInPlace(root, relativePath, file, openedInfo, expectedContent, content) +} + +// restoreRootedFile puts the observed bytes back after a failed in-place +// write. The write already truncated the file, so this is best effort: the +// caller receives both the original failure and any restore failure. +func restoreRootedFile(file *os.File, preimage *string) error { + if preimage == nil { + return nil + } + if err := file.Truncate(0); err != nil { + return fmt.Errorf("restoring the preimage: %w", err) + } + if _, err := file.Seek(0, io.SeekStart); err != nil { + return fmt.Errorf("restoring the preimage: %w", err) + } + if _, err := io.WriteString(file, *preimage); err != nil { + return fmt.Errorf("restoring the preimage: %w", err) + } + return nil +} + +// removeWriteSideFile removes a staged replacement that never reached (or did +// not survive) its publish, through the root handle. func removeWriteSideFile(root *os.Root, name string) error { - return removeStructuredPatchTemp(root, name) + err := root.Remove(name) + if err == nil || errors.Is(err, os.ErrNotExist) { + return nil + } + // Windows refuses to delete a read-only file. The staged file only carries + // that bit after its published mode was applied; clear it through the root + // and retry. The name belongs to a random side file this commit created. + if errors.Is(err, os.ErrPermission) { + if chmodErr := root.Chmod(name, 0o600); chmodErr == nil { + err = root.Remove(name) + if err == nil || errors.Is(err, os.ErrNotExist) { + return nil + } + } + } + return err } diff --git a/internal/tools/file_commit_publish_test.go b/internal/tools/file_commit_publish_test.go new file mode 100644 index 000000000..35363cbb6 --- /dev/null +++ b/internal/tools/file_commit_publish_test.go @@ -0,0 +1,283 @@ +package tools + +import ( + "context" + "errors" + "os" + "path/filepath" + "strings" + "testing" +) + +func installFileWriteRename(t *testing.T, rename func(*os.Root, string, string) error) { + t.Helper() + prior := fileWriteRename + fileWriteRename = rename + t.Cleanup(func() { fileWriteRename = prior }) +} + +func installFileWriteBeforePublish(t *testing.T, hook func(string)) { + t.Helper() + prior := fileWriteBeforePublish + fileWriteBeforePublish = hook + t.Cleanup(func() { fileWriteBeforePublish = prior }) +} + +func installFileWriteNoReplaceRename(t *testing.T, rename func(*os.Root, string, string) error) { + t.Helper() + prior := fileWriteNoReplaceRename + fileWriteNoReplaceRename = rename + t.Cleanup(func() { fileWriteNoReplaceRename = prior }) +} + +// A FINAL-COMPONENT SYMLINK STAYS A LINK. Main opened the target through the +// link and wrote through it, so replacing the link with a regular file leaves +// the link's target stuck on its old bytes. Publishing over the resolved path +// preserves both halves: the link is still a link, and the file it names has +// the new content. +// +// The public tools reject a symlink during path validation today, so this is +// asserted at the commit layer, which is where the publish semantics live and +// where a scoped write that resolves through an extra root reaches them. +func TestOverwriteKeepsAnInWorkspaceSymlink(t *testing.T) { + for _, tc := range []struct { + name string + chain bool + }{ + {name: "direct link"}, + {name: "link chain", chain: true}, + } { + t.Run(tc.name, func(t *testing.T) { + root := t.TempDir() + target := filepath.Join(root, "target.txt") + if err := os.WriteFile(target, []byte("observed\n"), 0o644); err != nil { + t.Fatal(err) + } + link := filepath.Join(root, "link.txt") + if err := os.Symlink("target.txt", link); err != nil { + t.Skipf("symlinks unavailable: %v", err) + } + publishPath, links := "link.txt", []string{link} + if tc.chain { + chained := filepath.Join(root, "chained.txt") + if err := os.Symlink("link.txt", chained); err != nil { + t.Fatalf("create second symlink: %v", err) + } + publishPath, links = "chained.txt", append(links, chained) + } + + workspace, err := os.OpenRoot(root) + if err != nil { + t.Fatal(err) + } + defer workspace.Close() + expectedInfo, err := workspace.Stat(publishPath) + if err != nil { + t.Fatal(err) + } + preimage := "observed\n" + if err := commitRootedFileContents(workspace, filepath.Join(root, publishPath), publishPath, expectedInfo, &preimage, "zero\n"); err != nil { + t.Fatalf("symlink overwrite: %v", err) + } + for _, path := range links { + info, err := os.Lstat(path) + if err != nil { + t.Fatalf("inspect %s: %v", path, err) + } + if info.Mode()&os.ModeSymlink == 0 { + t.Fatalf("%s is no longer a symlink: mode=%v", path, info.Mode()) + } + } + if got, err := os.ReadFile(target); err != nil || string(got) != "zero\n" { + t.Fatalf("link target = %q, err=%v; the link must keep pointing at the rewritten file", got, err) + } + entries, err := os.ReadDir(root) + if err != nil { + t.Fatal(err) + } + for _, entry := range entries { + if strings.HasSuffix(entry.Name(), ".tmp") { + t.Fatalf("staging file %s survived a successful publish", entry.Name()) + } + } + }) + } +} + +// A READER CAN HOLD THE FILE OPEN. Windows refuses a rename over a name while +// another handle has it open without delete sharing, so the atomic publish is +// not available there; the commit must fall back to writing through the +// validated handle rather than failing an ordinary overwrite. The same test +// also runs on the rename-capable platforms, where it proves the common path. +func TestOverwriteSucceedsWithAnOpenReader(t *testing.T) { + root := t.TempDir() + target := filepath.Join(root, "existing.txt") + if err := os.WriteFile(target, []byte("observed\n"), 0o644); err != nil { + t.Fatal(err) + } + reader, err := os.Open(target) + if err != nil { + t.Fatal(err) + } + defer reader.Close() + + result := NewScopedWriteFileTool(root, nil).Run(context.Background(), map[string]any{ + "path": "existing.txt", "content": "zero\n", "overwrite": true, + }) + if result.Status != StatusOK { + t.Fatalf("overwrite with an open reader = %s: %s", result.Status, result.Output) + } + if got, err := os.ReadFile(target); err != nil || string(got) != "zero\n" { + t.Fatalf("target = %q, err=%v", got, err) + } +} + +// WHEN THE PLATFORM REFUSES THE REPLACEMENT, the previous in-place publish is +// still available. The fallback reopens and re-validates the object first, so +// this is not a licence to write into whatever currently carries the name. +func TestOverwriteFallsBackToInPlaceWhenReplacementIsRefused(t *testing.T) { + root := t.TempDir() + target := filepath.Join(root, "existing.txt") + if err := os.WriteFile(target, []byte("observed\n"), 0o644); err != nil { + t.Fatal(err) + } + installFileWriteRename(t, func(*os.Root, string, string) error { + return errors.New("replacement refused") + }) + + result := NewScopedWriteFileTool(root, nil).Run(context.Background(), map[string]any{ + "path": "existing.txt", "content": "zero\n", "overwrite": true, + }) + if result.Status != StatusOK { + t.Fatalf("fallback overwrite = %s: %s", result.Status, result.Output) + } + if got, err := os.ReadFile(target); err != nil || string(got) != "zero\n" { + t.Fatalf("fallback target = %q, err=%v", got, err) + } + entries, err := os.ReadDir(root) + if err != nil { + t.Fatal(err) + } + if len(entries) != 1 || entries[0].Name() != "existing.txt" { + t.Fatalf("fallback left %d entries; the staged replacement must be removed", len(entries)) + } +} + +// THE FALLBACK IS NOT A REPLACEMENT LICENCE. A competing writer that installs +// a new file during the refused publish must be detected by identity before +// the fallback truncates anything, even when the bytes are identical. +func TestOverwriteFallbackRefusesAReplacedTarget(t *testing.T) { + root := t.TempDir() + target := filepath.Join(root, "existing.txt") + if err := os.WriteFile(target, []byte("observed\n"), 0o644); err != nil { + t.Fatal(err) + } + installFileWriteRename(t, func(*os.Root, string, string) error { + preserved := target + ".preserved" + if err := os.Rename(target, preserved); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(target, []byte("observed\n"), 0o644); err != nil { + t.Fatal(err) + } + return errors.New("replacement refused") + }) + + result := NewScopedWriteFileTool(root, nil).Run(context.Background(), map[string]any{ + "path": "existing.txt", "content": "zero\n", "overwrite": true, + }) + if result.Status != StatusError || !strings.Contains(result.Output, errFileChangedDuringWrite.Error()) { + t.Fatalf("replaced fallback target = %s: %s", result.Status, result.Output) + } + if got, err := os.ReadFile(target); err != nil || string(got) != "observed\n" { + t.Fatalf("replacement = %q, err=%v; the fallback must not truncate a file it did not validate", got, err) + } +} + +// A FILESYSTEM MAY NOT OFFER A NO-REPLACE RENAME. The hard-link fallback is +// still atomic against readers — a reader sees the absent name or the complete +// inode — and refuses a name another writer created. +func TestCreateFallsBackToAHardLinkWhenNoReplaceRenameIsUnsupported(t *testing.T) { + root := t.TempDir() + target := filepath.Join(root, "created.txt") + installFileWriteNoReplaceRename(t, func(*os.Root, string, string) error { + return errors.ErrUnsupported + }) + + result := NewScopedWriteFileTool(root, nil).Run(context.Background(), map[string]any{ + "path": "created.txt", "content": "zero\n", + }) + if result.Status != StatusOK { + t.Fatalf("hard-link create = %s: %s", result.Status, result.Output) + } + if got, err := os.ReadFile(target); err != nil || string(got) != "zero\n" { + t.Fatalf("created file = %q, err=%v", got, err) + } + entries, err := os.ReadDir(root) + if err != nil { + t.Fatal(err) + } + if len(entries) != 1 || entries[0].Name() != "created.txt" { + t.Fatalf("root holds %d entries; the staging name must not survive the publish", len(entries)) + } +} + +func TestCreateHardLinkFallbackStillRefusesAnExistingName(t *testing.T) { + root := t.TempDir() + target := filepath.Join(root, "existing.txt") + if err := os.WriteFile(target, []byte("other writer\n"), 0o644); err != nil { + t.Fatal(err) + } + installFileWriteNoReplaceRename(t, func(*os.Root, string, string) error { + return errors.ErrUnsupported + }) + + workspace, err := os.OpenRoot(root) + if err != nil { + t.Fatal(err) + } + defer workspace.Close() + if err := commitRootedFileContents(workspace, target, "existing.txt", nil, nil, "zero\n"); !errors.Is(err, os.ErrExist) { + t.Fatalf("hard-link create over an existing name = %v, want os.ErrExist", err) + } + if got, err := os.ReadFile(target); err != nil || string(got) != "other writer\n" { + t.Fatalf("existing file = %q, err=%v; the fallback must not replace it", got, err) + } + entries, err := os.ReadDir(root) + if err != nil { + t.Fatal(err) + } + if len(entries) != 1 || entries[0].Name() != "existing.txt" { + t.Fatalf("root holds %d entries; a refused create must remove its staging file", len(entries)) + } +} + +// A REPLACEMENT INSTALLED AFTER THE BYTES WERE STAGED must not be silently +// overwritten by the publish. The identity check immediately before the rename +// is what keeps the conflict guarantee for the window that remains. +func TestOverwriteRefusesAReplacementInstalledBeforePublish(t *testing.T) { + root := t.TempDir() + target := filepath.Join(root, "existing.txt") + if err := os.WriteFile(target, []byte("observed\n"), 0o644); err != nil { + t.Fatal(err) + } + installFileWriteBeforePublish(t, func(path string) { + preserved := path + ".preserved" + if err := os.Rename(path, preserved); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(path, []byte("competing\n"), 0o644); err != nil { + t.Fatal(err) + } + }) + + result := NewScopedWriteFileTool(root, nil).Run(context.Background(), map[string]any{ + "path": "existing.txt", "content": "zero\n", "overwrite": true, + }) + if result.Status != StatusError || !strings.Contains(result.Output, errFileChangedDuringWrite.Error()) { + t.Fatalf("pre-publish replacement = %s: %s", result.Status, result.Output) + } + if got, err := os.ReadFile(target); err != nil || string(got) != "competing\n" { + t.Fatalf("competing file = %q, err=%v; the publish must refuse a replacement it did not observe", got, err) + } +} diff --git a/internal/tools/file_commit_test.go b/internal/tools/file_commit_test.go index 5f3527677..b21f19b9d 100644 --- a/internal/tools/file_commit_test.go +++ b/internal/tools/file_commit_test.go @@ -385,9 +385,11 @@ func TestOverwriteDoesNotTruncateAHardlinkSibling(t *testing.T) { // carried over from the file that was observed, not from the side file the // bytes were staged in. func TestOverwriteKeepsTheObservedPermissionBits(t *testing.T) { - want := os.FileMode(0o600) + // 0o640 differs from the 0o600 the side file is staged with, so a publish + // that forgot to carry the observed bits cannot pass by coincidence. + want := os.FileMode(0o640) if runtime.GOOS == "windows" { - // A Windows file carries only the read-only bit, so a 0o600 file is + // A Windows file carries only the read-only bit, so a 0o640 file is // reported as 0o666; the write bit, not the exact value, is the fact // being pinned. want = os.FileMode(0o666) diff --git a/internal/tools/file_commit_umask_test.go b/internal/tools/file_commit_umask_test.go index 4039f6e48..a3b82705c 100644 --- a/internal/tools/file_commit_umask_test.go +++ b/internal/tools/file_commit_umask_test.go @@ -13,15 +13,15 @@ import ( // THE PUBLISHED FILE CARRIES THE PERMISSIONS THE PROCESS UMASK PRODUCES, NOT // THE RAW MODE AN ASSIGNMENT WOULD SET. // -// The create path reserves the target name with the same exclusive create this -// branch of the write path always honored, and the kernel applies the process -// umask to the reserved file it grants. The publish must carry those granted -// bits — taken from the reserved file itself — because a chmod never consults -// the umask: a process that requests tight defaults on new files would find -// every write it publishes shared with the world. Under the restrictive umask -// a plain mode assignment would publish 0o644 where the reservation was -// granted 0o600, so the test fails before the granted bits are carried. -func TestCreatePublishesTheUmaskedPermissionsOfTheReservedCreate(t *testing.T) { +// The create path opens its staging file with the same exclusive 0o644 create +// this branch of the write path always honored, and the kernel applies the +// process umask to that open. The publish must carry the granted bits — taken +// from the staging file itself — because a chmod never consults the umask: a +// process that requests tight defaults on new files would find every write it +// publishes shared with the world. Under the restrictive umask a plain mode +// assignment would publish 0o644 where the kernel granted 0o600, so the test +// fails before the granted bits are carried. +func TestCreatePublishesTheUmaskedPermissionsOfTheStagedCreate(t *testing.T) { for name, testCase := range map[string]struct { umask int want os.FileMode @@ -46,7 +46,7 @@ func TestCreatePublishesTheUmaskedPermissionsOfTheReservedCreate(t *testing.T) { t.Fatal(err) } if got := info.Mode().Perm(); got != testCase.want { - t.Fatalf("published mode = %#o, want the %#o the kernel granted the reservation under this umask; a raw mode assignment would publish %#o and share the file with hands the umask excludes", got, testCase.want, os.FileMode(0o644)) + t.Fatalf("published mode = %#o, want the %#o the kernel granted the staging create under this umask; a raw mode assignment would publish %#o and share the file with hands the umask excludes", got, testCase.want, os.FileMode(0o644)) } if got, readErr := os.ReadFile(target); readErr != nil || string(got) != "zero\n" { t.Fatalf("published content = %q, err=%v", got, readErr)