From 06f2a62d159b5b22f7d75436300fcdd2ecdbd7fc Mon Sep 17 00:00:00 2001 From: dborup Date: Mon, 5 Oct 2026 07:18:34 +0000 Subject: [PATCH 1/2] test(ingestor): reproduce the hard-linked stats tmp; guard the #160 FIFO tests (#228) - TestWriteStatsAtomicRefusesHardLinkedTmp_228 and a "hard link" case in TestStatsWriteErrorNamesThePathOnce_160 plant a hard link at the tmp path. On master the writer accepts it, truncates the target and publishes it. - The FIFO subtest of TestStatsWriteErrorNamesThePathOnce_160 now runs through writeStatsAtomicOrRelease, and TestStatsFileWriterFailureLineNamesThePathOnce_160 stops the writer through stopStatsWriterOrRelease (body and cleanup). If #161 regresses, they now fail in seconds instead of hanging until the package timeout. stopStatsWriterOrRelease is the guard that TestStatsFileWriterStopsWithFIFOAtTmp_161 had inline. - A "rename failure" case (a non-empty directory at the stats path) covers the stripping of the *os.LinkError. Co-Authored-By: Claude Opus 5.5 --- .../stats_file_errtext_160_unix_test.go | 49 ++++++++++++++---- cmd/ingestor/stats_file_fifo_161_unix_test.go | 34 ++++++++----- .../stats_file_hardlink_228_unix_test.go | 50 +++++++++++++++++++ 3 files changed, 110 insertions(+), 23 deletions(-) create mode 100644 cmd/ingestor/stats_file_hardlink_228_unix_test.go diff --git a/cmd/ingestor/stats_file_errtext_160_unix_test.go b/cmd/ingestor/stats_file_errtext_160_unix_test.go index 59f97a5dc..6582478dd 100644 --- a/cmd/ingestor/stats_file_errtext_160_unix_test.go +++ b/cmd/ingestor/stats_file_errtext_160_unix_test.go @@ -15,9 +15,11 @@ import ( // a "[stats-file] write : …" line. Each error now names the tmp once, // at the start, and the writer's failure line adds no second copy. func TestStatsWriteErrorNamesThePathOnce_160(t *testing.T) { + // setup returns the FIFO it planted at the tmp path, if any, so a write + // that blocks on it can be released (#161). type tc struct { name string - setup func(t *testing.T, path string) + setup func(t *testing.T, path string) (fifo string) want string } foreign := func(t *testing.T) { @@ -26,19 +28,21 @@ func TestStatsWriteErrorNamesThePathOnce_160(t *testing.T) { statsFileEUID = func() int { return os.Geteuid() + 1 } } cases := []tc{ - {"FIFO without a reader", func(t *testing.T, path string) { mkStatsTmpFIFO(t, path) }, "not a regular file"}, - {"directory", func(t *testing.T, path string) { + {"FIFO without a reader", func(t *testing.T, path string) string { return mkStatsTmpFIFO(t, path) }, "not a regular file"}, + {"directory", func(t *testing.T, path string) string { if err := os.Mkdir(path+".tmp", 0o700); err != nil { t.Fatal(err) } + return "" }, "not a regular file"}, - {"foreign owner", func(t *testing.T, path string) { + {"foreign owner", func(t *testing.T, path string) string { if err := os.WriteFile(path+".tmp", nil, 0o600); err != nil { t.Fatal(err) } foreign(t) + return "" }, "remove it or fix its owner"}, - {"foreign owner, unopenable", func(t *testing.T, path string) { + {"foreign owner, unopenable", func(t *testing.T, path string) string { if os.Geteuid() == 0 { t.Skip("root opens a 0400 file for writing") } @@ -46,14 +50,37 @@ func TestStatsWriteErrorNamesThePathOnce_160(t *testing.T) { t.Fatal(err) } foreign(t) + return "" }, "remove it or fix its owner"}, + // The rename fails on a non-empty directory at the stats path; its + // *os.LinkError names both paths (#228). + {"rename failure", func(t *testing.T, path string) string { + if err := os.MkdirAll(filepath.Join(path, "keep"), 0o700); err != nil { + t.Fatal(err) + } + return "" + }, "rename to stats.json: "}, + {"hard link", func(t *testing.T, path string) string { + target := filepath.Join(filepath.Dir(path), "other") + if err := os.WriteFile(target, nil, 0o600); err != nil { + t.Fatal(err) + } + if err := os.Link(target, path+".tmp"); err != nil { + t.Skipf("link: %v", err) + } + return "" + }, "hard-linked (nlink 2); remove it"}, } for _, c := range cases { t.Run(c.name, func(t *testing.T) { dir := t.TempDir() path := filepath.Join(dir, "stats.json") - c.setup(t, path) - err := writeStatsAtomic(path, []byte(`{}`)) + var err error + if fifo := c.setup(t, path); fifo != "" { + err = writeStatsAtomicOrRelease(t, path, fifo) + } else { + err = writeStatsAtomic(path, []byte(`{}`)) + } if err == nil { t.Fatal("accepted") } @@ -76,11 +103,13 @@ func TestStatsFileWriterFailureLineNamesThePathOnce_160(t *testing.T) { dir := t.TempDir() path := filepath.Join(dir, "stats.json") t.Setenv("CORESCOPE_INGESTOR_STATS", path) - mkStatsTmpFIFO(t, path) + fifo := mkStatsTmpFIFO(t, path) logs := captureStdLog(t) store := openStatsTestStore(t, dir) stop := StartStatsFileWriter(store, 5*time.Millisecond) - t.Cleanup(stop) + // Guarded: a writer blocked on the FIFO (#161) fails the test, not + // hangs it, also when the test ends early. + t.Cleanup(func() { stopStatsWriterOrRelease(t, stop, fifo) }) deadline := time.Now().Add(3 * time.Second) for len(logs.linesWith("[stats-file] write")) == 0 { @@ -89,7 +118,7 @@ func TestStatsFileWriterFailureLineNamesThePathOnce_160(t *testing.T) { } time.Sleep(5 * time.Millisecond) } - stop() + stopStatsWriterOrRelease(t, stop, fifo) line := logs.linesWith("[stats-file] write")[0] if n := strings.Count(line, dir); n != 1 { t.Errorf("path named %d times in %q, want once", n, line) diff --git a/cmd/ingestor/stats_file_fifo_161_unix_test.go b/cmd/ingestor/stats_file_fifo_161_unix_test.go index 1fb7f8154..14ede38e7 100644 --- a/cmd/ingestor/stats_file_fifo_161_unix_test.go +++ b/cmd/ingestor/stats_file_fifo_161_unix_test.go @@ -54,6 +54,26 @@ func writeStatsAtomicOrRelease(t *testing.T, path, fifo string) error { } } +// stopStatsWriterOrRelease calls a stats writer's stop. If it blocks for +// 3s, the FIFO gets a reader so the writer can end, and the test fails. It +// reports with t.Error, so it may also run as a cleanup. +func stopStatsWriterOrRelease(t *testing.T, stop func(), fifo string) { + t.Helper() + stopped := make(chan struct{}) + go func() { stop(); close(stopped) }() + select { + case <-stopped: + case <-time.After(3 * time.Second): + r := openFIFOReader(t, fifo) + defer r.Close() + select { + case <-stopped: + case <-time.After(3 * time.Second): + } + t.Error("stop hung: the writer is blocked on the FIFO") + } +} + // assertNotRegularRefused checks the error and that nothing was published. func assertNotRegularRefused(t *testing.T, err error, path string) { t.Helper() @@ -98,17 +118,5 @@ func TestStatsFileWriterStopsWithFIFOAtTmp_161(t *testing.T) { stop := StartStatsFileWriter(store, 5*time.Millisecond) time.Sleep(100 * time.Millisecond) // several ticks against the FIFO - stopped := make(chan struct{}) - go func() { stop(); close(stopped) }() - select { - case <-stopped: - case <-time.After(3 * time.Second): - r := openFIFOReader(t, fifo) - defer r.Close() - select { - case <-stopped: - case <-time.After(3 * time.Second): - } - t.Fatal("stop hung: the writer is blocked on the FIFO") - } + stopStatsWriterOrRelease(t, stop, fifo) } diff --git a/cmd/ingestor/stats_file_hardlink_228_unix_test.go b/cmd/ingestor/stats_file_hardlink_228_unix_test.go new file mode 100644 index 000000000..761827099 --- /dev/null +++ b/cmd/ingestor/stats_file_hardlink_228_unix_test.go @@ -0,0 +1,50 @@ +//go:build !windows + +package main + +import ( + "os" + "path/filepath" + "testing" +) + +// A hard link at the tmp path is a regular file of the ingestor's user, so +// it passed the type and owner checks: the writer truncated and overwrote +// the link target and published it by the rename. It is now refused and +// left in place, and the target is untouched (#228). +func TestWriteStatsAtomicRefusesHardLinkedTmp_228(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "stats.json") + target := filepath.Join(dir, "precious") + const content = "precious content" + if err := os.WriteFile(target, []byte(content), 0o640); err != nil { + t.Fatal(err) + } + if err := os.Chmod(target, 0o640); err != nil { // past the umask + t.Fatal(err) + } + if err := os.Link(target, path+".tmp"); err != nil { + t.Skipf("link: %v", err) + } + + err := writeStatsAtomic(path, []byte(`{}`)) + if err == nil { + t.Fatal("hard-linked tmp accepted") + } + want := path + ".tmp: hard-linked (nlink 2); remove it" + if err.Error() != want { + t.Errorf("error %q, want %q", err, want) + } + if got, err := os.ReadFile(target); err != nil || string(got) != content { + t.Errorf("link target changed: %q, %v", got, err) + } + if fi, err := os.Stat(target); err != nil || fi.Mode().Perm() != 0o640 { + t.Errorf("link target mode changed: %v, %v", fi.Mode(), err) + } + if _, err := os.Lstat(path); !os.IsNotExist(err) { + t.Errorf("stats file published from a hard link: lstat err %v", err) + } + if _, err := os.Lstat(path + ".tmp"); err != nil { + t.Errorf("hard-linked tmp not left in place: %v", err) + } +} From 2acd9249ab95686cf7dc5776b2ca2532b3a71244 Mon Sep 17 00:00:00 2001 From: dborup Date: Mon, 5 Oct 2026 07:18:34 +0000 Subject: [PATCH 2/2] fix(ingestor): refuse a hard-linked stats tmp (#228) A hard link at .tmp to another file of the ingestor's user passed the type and owner checks, so the writer chmod'ed, truncated and overwrote that file and published it by the rename. The writer now checks the link count from the open descriptor's Fstat before changing anything. When nlink > 1 it leaves the tmp in place and reports ": hard-linked (nlink N); remove it". On Windows, FileInfo has no link count, so the check is skipped there, like the owner check. The owner detail and hint are now built in one helper, setForeignOwner, which setPermissionHint and checkStatsTmpOwner share (round-2 review of #216). Co-Authored-By: Claude Opus 5.5 --- cmd/ingestor/stats_file.go | 32 ++++++++++++++++++------ cmd/ingestor/stats_file_owner_unix.go | 9 +++++++ cmd/ingestor/stats_file_owner_windows.go | 4 +++ 3 files changed, 38 insertions(+), 7 deletions(-) diff --git a/cmd/ingestor/stats_file.go b/cmd/ingestor/stats_file.go index 86cdbfec9..268f12f20 100644 --- a/cmd/ingestor/stats_file.go +++ b/cmd/ingestor/stats_file.go @@ -186,6 +186,16 @@ func writeStatsAtomic(path string, b []byte) error { f.Close() return err } + // A hard link is a second name for another file, which the chmod, + // truncate and rename below would change and publish. It is left in + // place for the operator (#228). + if n, ok := fileLinkCount(fi); ok && n > 1 { + f.Close() + e := newStatsWriteError(tmp, "", nil) + e.detail = fmt.Sprintf("hard-linked (nlink %d)", n) + e.hint = "remove it" + return e + } if err := f.Chmod(0o600); err != nil { f.Close() return fail("chmod", err) @@ -265,9 +275,7 @@ func (e *statsWriteError) setNotRegular(mode os.FileMode) { // (fi from Lstat) the ingestor may not open: fix the owner when it is // someone else's, else its permissions (#160). func (e *statsWriteError) setPermissionHint(fi os.FileInfo) { - if uid, ok := fileOwnerUID(fi); ok && uid != statsFileEUID() { - e.detail = fmt.Sprintf("owned by uid %d, ingestor uid %d", uid, statsFileEUID()) - e.hint = "remove it or fix its owner" + if e.setForeignOwner(fi) { return } e.detail = fmt.Sprintf("mode %v", fi.Mode()) @@ -283,15 +291,25 @@ var statsFileEUID = os.Geteuid // no Unix owner (Windows) it passes. The ingestor cannot fix this itself; // the error says what the operator must do (#160). func checkStatsTmpOwner(name string, fi os.FileInfo) error { - if uid, ok := fileOwnerUID(fi); ok && uid != statsFileEUID() { - e := newStatsWriteError(name, "", nil) - e.detail = fmt.Sprintf("owned by uid %d, ingestor uid %d", uid, statsFileEUID()) - e.hint = "remove it or fix its owner" + if e := newStatsWriteError(name, "", nil); e.setForeignOwner(fi) { return e } return nil } +// setForeignOwner marks a tmp (fi) that belongs to a user other than +// statsFileEUID, and reports whether it does. Where files have no Unix +// owner (Windows) it does not (#160). +func (e *statsWriteError) setForeignOwner(fi os.FileInfo) bool { + uid, ok := fileOwnerUID(fi) + if !ok || uid == statsFileEUID() { + return false + } + e.detail = fmt.Sprintf("owned by uid %d, ingestor uid %d", uid, statsFileEUID()) + e.hint = "remove it or fix its owner" + return true +} + // statsWriteErrLogEvery is how often a failure that persists is logged // again. The writer ticks every second (#160). const statsWriteErrLogEvery = time.Minute diff --git a/cmd/ingestor/stats_file_owner_unix.go b/cmd/ingestor/stats_file_owner_unix.go index 7d42787c2..dd54060e3 100644 --- a/cmd/ingestor/stats_file_owner_unix.go +++ b/cmd/ingestor/stats_file_owner_unix.go @@ -15,3 +15,12 @@ func fileOwnerUID(fi os.FileInfo) (int, bool) { } return int(st.Uid), true } + +// fileLinkCount returns the number of hard links to fi. +func fileLinkCount(fi os.FileInfo) (uint64, bool) { + st, ok := fi.Sys().(*syscall.Stat_t) + if !ok { + return 0, false + } + return uint64(st.Nlink), true +} diff --git a/cmd/ingestor/stats_file_owner_windows.go b/cmd/ingestor/stats_file_owner_windows.go index b0739a357..5ddf3883e 100644 --- a/cmd/ingestor/stats_file_owner_windows.go +++ b/cmd/ingestor/stats_file_owner_windows.go @@ -7,3 +7,7 @@ import "os" // fileOwnerUID reports no owner on Windows, which has no Unix user IDs; the // ingestor is only deployed on Linux. func fileOwnerUID(os.FileInfo) (int, bool) { return 0, false } + +// fileLinkCount reports no link count on Windows, where FileInfo does not +// carry one. +func fileLinkCount(os.FileInfo) (uint64, bool) { return 0, false }