diff --git a/internal/cli/credentials.go b/internal/cli/credentials.go index c6a9685dd..ab98b7484 100644 --- a/internal/cli/credentials.go +++ b/internal/cli/credentials.go @@ -154,6 +154,11 @@ func loadStoreLocked(path string) (*CredentialStore, error) { return newEmptyStore(), nil } if i >= credentialsEmptyRereads { + // A stat that shows a body means a save wrote between the last read + // and the stat: read once more rather than answer empty (BUG-3838). + if serr == nil && fi.Size() != 0 && i == credentialsEmptyRereads { + continue + } if serr == nil { emptyConfirmed.set(path, fi) } @@ -164,10 +169,15 @@ func loadStoreLocked(path string) (*CredentialStore, error) { } // emptyConfirmed is the empty credentials file a locked load already waited -// out (BUG-3829). A save moves the mtime at its truncate, even of a file that -// is already empty (measured on ext4, Linux 6.17: ftruncate(0) of an empty -// file sets mtime to now), so a later empty read of a file a save is -// rewriting does not match and is re-read again. +// out (BUG-3829). A save sets the file's mtime to now before it truncates +// (writeFileInPlace0600), even when the file is already empty, so a later +// empty read of a file a save is rewriting does not match and is re-read +// again. The save sets it explicitly because the truncate alone is not +// enough everywhere: ftruncate(0) of an empty file moved mtime on ext4 +// (Linux 6.17), but POSIX requires that only when the size changes, and +// macOS and Windows were never measured (BUG-3838). Residuals: a save by a +// pad built before BUG-3838, or one whose touch fails (a foreign-owned +// file), relies on the truncate as before. // // Only a file whose mtime is emptyConfirmedMinAge older than the confirmation // is remembered. A later save is stamped no earlier than the confirmation, so @@ -411,7 +421,13 @@ func writeFileInPlace0600(path string, data []byte) error { } if fi.Mode().IsRegular() { _ = f.Chmod(0600) - if err := f.Truncate(0); err != nil { + // The mtime is set before the truncate, so a reader's emptyConfirmed + // memo stops matching even when the file is already empty: POSIX moves + // mtime on ftruncate only when the size changes, and Windows' + // SetEndOfFile need not either (BUG-3838). Best effort, like the chmod: + // a foreign-owned file we may write but not touch keeps that residual. + _ = touchCredentialsFile(f, path) + if err := truncateCredentialsFile(f, 0); err != nil { _ = f.Close() return err } @@ -427,6 +443,12 @@ func writeFileInPlace0600(path string, data []byte) error { // statOpenFile is f.Stat, a variable so a test can fail it (BUG-3810). var statOpenFile = (*os.File).Stat +// truncateCredentialsFile is f.Truncate, a variable so a test can stall a +// save between its truncate and its write, and give the truncate the +// semantics of a platform where it leaves an empty file's mtime alone +// (BUG-3838). +var truncateCredentialsFile = (*os.File).Truncate + // credentialsLockWarning is where a write that could not take the lock says // so; a variable so a test can read it. var credentialsLockWarning io.Writer = os.Stderr diff --git a/internal/cli/credentials_bug3838_test.go b/internal/cli/credentials_bug3838_test.go new file mode 100644 index 000000000..76ccb2557 --- /dev/null +++ b/internal/cli/credentials_bug3838_test.go @@ -0,0 +1,195 @@ +package cli + +import ( + "os" + "sync" + "testing" + "time" +) + +// BUG-3838: the emptyConfirmed memo relied on a save's truncate moving the +// file's mtime even when the file was already empty. That was measured on +// Linux only; POSIX moves it only when the size changes. The save now sets +// the mtime itself before it truncates. + +// posixTruncate gives the save's truncate the semantics of a platform where +// ftruncate leaves an empty file's mtime alone, and stalls the save after it +// until release is called. truncated is closed once the truncate has run. +// Cleanup releases a save the test left stalled, so a failing test leaves no +// goroutine blocked in the seam (codex r1 on BUG-3838). +func posixTruncate(t *testing.T, path string) (truncated chan struct{}, release func()) { + truncated = make(chan struct{}) + stall := make(chan struct{}) + var once sync.Once + release = func() { once.Do(func() { close(stall) }) } + orig := truncateCredentialsFile + truncateCredentialsFile = func(f *os.File, size int64) error { + before, err := os.Stat(path) + if err != nil { + return err + } + if err := orig(f, size); err != nil { + return err + } + if before.Size() == size { + if err := os.Chtimes(path, before.ModTime(), before.ModTime()); err != nil { + return err + } + } + close(truncated) + <-stall + return nil + } + t.Cleanup(func() { truncateCredentialsFile = orig }) + t.Cleanup(release) + return truncated, release +} + +// A process has remembered a genuinely empty file. Another process saves over +// it without the lock, on a platform whose truncate leaves the mtime alone. A +// locked load between that save's truncate and its write must not take the +// memo: it re-reads and loads what the save wrote. +func TestBUG3838_MemoDoesNotSkipASaveWhoseTruncateKeepsTheMtime(t *testing.T) { + home := withTempHome(t) + path := writeCredsFile(t, home, "") + resetEmptyConfirmed(t) + shortRereads(t, 3, time.Millisecond) + aged := time.Now().Add(-time.Hour) + if err := os.Chtimes(path, aged, aged); err != nil { + t.Fatal(err) + } + if _, err := LoadStore(); err != nil { + t.Fatal(err) + } + emptyConfirmed.mu.Lock() + remembered := emptyConfirmed.fi != nil + emptyConfirmed.mu.Unlock() + if !remembered { + t.Fatal("the aged empty file was not remembered; the test needs the memo set") + } + + truncated, release := posixTruncate(t, path) + saved := make(chan error, 1) + go func() { saved <- writeFileInPlace0600(path, []byte(twoServers)) }() + // Registered after posixTruncate's, so it runs first: release, then wait. + t.Cleanup(func() { + release() + if err := <-saved; err != nil { + t.Error(err) + } + }) + select { + case <-truncated: + case err := <-saved: + saved <- err // for the cleanup's receive + t.Fatalf("the save returned before its truncate stalled: %v", err) + case <-time.After(5 * time.Second): + t.Fatal("the save never reached its truncate") + } + reads := countReads(t, func(n int) { + if n == 2 { + release() + // Wait for the write without consuming the save's result, bounded + // so a save that failed after the release fails the test instead + // of hanging it (hunter r2 on BUG-3838); cleanup reports its error. + for deadline := time.Now().Add(5 * time.Second); time.Now().Before(deadline); { + if fi, err := os.Stat(path); err == nil && fi.Size() != 0 { + return + } + time.Sleep(time.Millisecond) + } + t.Error("the released save never wrote the file") + } + }) + + store, err := loadStoreLocked(path) + if err != nil { + t.Fatal(err) + } + if n := *reads; n < 2 { + t.Fatalf("reads = %d: the load answered from the memo during the save", n) + } + if store.Get("http://a:7777") == nil || store.Get("http://b:7777") == nil { + t.Errorf("store = %+v, want the save's two entries", store.Credentials) + } +} + +// The last re-read is empty, and a save writes before the stat that follows +// it: the stat's non-zero size earns one more read instead of an empty store. +func TestBUG3838_FinalStatShowingABodyRereadsOnce(t *testing.T) { + home := withTempHome(t) + path := writeCredsFile(t, home, "") + resetEmptyConfirmed(t) + shortRereads(t, 3, time.Millisecond) + orig := readCredentialsFile + n := 0 + readCredentialsFile = func(p string) ([]byte, error) { + n++ + data, err := orig(p) + if n == 4 { + if werr := os.WriteFile(path, []byte(twoServers), 0600); werr != nil { + t.Error(werr) + } + } + return data, err + } + t.Cleanup(func() { readCredentialsFile = orig }) + + store, err := loadStoreLocked(path) + if err != nil { + t.Fatal(err) + } + if n != 5 { + t.Errorf("reads = %d, want 5 (1+3 re-reads, then the one the stat earned)", n) + } + if store.Get("http://a:7777") == nil || store.Get("http://b:7777") == nil { + t.Errorf("store = %+v, want the save's two entries", store.Credentials) + } +} + +// The touch lands on the file the save opened, not on whatever the path names +// by the time it runs: a symlink retargeted after the open must not take the +// stamp while the opened file keeps its old mtime (codex r3 on BUG-3838). +func TestBUG3838_TouchStampsTheOpenedFile(t *testing.T) { + dir := t.TempDir() + opened, other, link := dir+"/opened", dir+"/other", dir+"/credentials.json" + aged := time.Now().Add(-time.Hour).Truncate(time.Second) + for _, p := range []string{opened, other} { + if err := os.WriteFile(p, nil, 0600); err != nil { + t.Fatal(err) + } + if err := os.Chtimes(p, aged, aged); err != nil { + t.Fatal(err) + } + } + if err := os.Symlink(opened, link); err != nil { + t.Skipf("no symlinks here: %v", err) + } + f, err := os.OpenFile(link, os.O_WRONLY, 0600) + if err != nil { + t.Fatal(err) + } + defer f.Close() + if err := os.Remove(link); err != nil { + t.Fatal(err) + } + if err := os.Symlink(other, link); err != nil { + t.Fatal(err) + } + + if err := touchCredentialsFile(f, link); err != nil { + t.Fatalf("touch: %v", err) + } + for _, c := range []struct { + path string + moved bool + }{{opened, true}, {other, false}} { + fi, err := os.Stat(c.path) + if err != nil { + t.Fatal(err) + } + if moved := !fi.ModTime().Equal(aged); moved != c.moved { + t.Errorf("%s mtime moved = %v, want %v", c.path, moved, c.moved) + } + } +} diff --git a/internal/cli/credentials_touch_other.go b/internal/cli/credentials_touch_other.go new file mode 100644 index 000000000..327b77b53 --- /dev/null +++ b/internal/cli/credentials_touch_other.go @@ -0,0 +1,15 @@ +//go:build !windows && !(linux || darwin || freebsd || netbsd || openbsd || dragonfly) + +package cli + +import ( + "os" + "time" +) + +// touchCredentialsFile sets the save's file's mtime to now (BUG-3838), by +// path: this platform has no descriptor form wired here. +func touchCredentialsFile(_ *os.File, path string) error { + now := time.Now() + return os.Chtimes(path, now, now) +} diff --git a/internal/cli/credentials_touch_unix.go b/internal/cli/credentials_touch_unix.go new file mode 100644 index 000000000..66aa9288b --- /dev/null +++ b/internal/cli/credentials_touch_unix.go @@ -0,0 +1,30 @@ +//go:build linux || darwin || freebsd || netbsd || openbsd || dragonfly + +package cli + +import ( + "os" + "time" + + "golang.org/x/sys/unix" +) + +// touchCredentialsFile sets the save's open file's mtime to now (BUG-3838), +// through its descriptor only, so a path replaced or a symlink retargeted +// since the open cannot take the stamp instead (codex r3). There is no path +// fallback (codex r4): when the descriptor form fails (Linux's futimes +// resolves /proc/self/fd, so no /proc; EPERM on a foreign-owned file) the +// save relies on its truncate, which moves an empty file's mtime on Linux. +func touchCredentialsFile(f *os.File, _ string) error { + now := time.Now() + tv := []unix.Timeval{unix.NsecToTimeval(now.UnixNano()), unix.NsecToTimeval(now.UnixNano())} + rc, err := f.SyscallConn() + if err != nil { + return err + } + var ferr error + if err := rc.Control(func(fd uintptr) { ferr = unix.Futimes(int(fd), tv) }); err != nil { + return err + } + return ferr +} diff --git a/internal/cli/credentials_touch_windows.go b/internal/cli/credentials_touch_windows.go new file mode 100644 index 000000000..dbce23ec8 --- /dev/null +++ b/internal/cli/credentials_touch_windows.go @@ -0,0 +1,19 @@ +//go:build windows + +package cli + +import ( + "os" + "time" + + "golang.org/x/sys/windows" +) + +// touchCredentialsFile sets the save's open file's last-write time to now, +// through the handle the save already holds (BUG-3838). os.Chtimes would open +// the path again sharing only FILE_SHARE_WRITE, which fails against a reader +// holding the file open and, while it is open, makes a reader's open fail. +func touchCredentialsFile(f *os.File, _ string) error { + now := windows.NsecToFiletime(time.Now().UnixNano()) + return windows.SetFileTime(windows.Handle(f.Fd()), nil, &now, &now) +}