fix(ingestor): refuse a hard-linked stats tmp, and harden the #160/#161 tests (#228)

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

cmd/ingestor/stats_file_errtext_160_unix_test.go

@@ -15,9 +15,11 @@ import (
 // a "[stats-file] write <path>: …" 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,34 +28,59 @@ 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")
 			}
 			if err := os.WriteFile(path+".tmp", nil, 0o400); err != nil {
 				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)

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

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

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

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 }