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_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) + } +} 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 }