diff --git a/ingestion/internal/brainfs/brainfstest/brainfstest.go b/ingestion/internal/brainfs/brainfstest/brainfstest.go index fdfe968..c352194 100644 --- a/ingestion/internal/brainfs/brainfstest/brainfstest.go +++ b/ingestion/internal/brainfs/brainfstest/brainfstest.go @@ -27,7 +27,7 @@ const outsideContent = "---\ntitle: " + Sentinel + "\naliases:\n - " + Sentinel "\n\n" + Sentinel + " [[" + Sentinel + "]]\n" // Outside writes a file outside any brain dir containing Sentinel. -func Outside(t *testing.T) string { +func Outside(t testing.TB) string { t.Helper() p := filepath.Join(t.TempDir(), "secret.md") if err := os.WriteFile(p, []byte(outsideContent), 0o644); err != nil { @@ -42,9 +42,12 @@ var swapSeq atomic.Int64 // a symlink to outside or a FIFO into path, until the test ends. FIFOs get a // writer held open until the swapper exits, so a reader that skips the // handle check parks forever instead of reading EOF. The swapper is stopped -// and awaited before t.TempDir cleanup. -func StartSwap(t *testing.T, path, inside, outside string, fifo bool) { +// and awaited before t.TempDir cleanup. It returns the number of symlinks +// or FIFOs actually renamed into path so far, so a caller can tell a race +// that ran from one whose every swap failed (hyperguild#177). +func StartSwap(t testing.TB, path, inside, outside string, fifo bool) *atomic.Int64 { t.Helper() + var swaps atomic.Int64 var stop atomic.Bool var wg sync.WaitGroup wg.Add(1) @@ -67,22 +70,26 @@ func StartSwap(t *testing.T, path, inside, outside string, fifo bool) { }() for i := 0; !stop.Load(); i++ { _ = os.Remove(tmp) + var made error switch { case i%2 == 0: _ = os.WriteFile(tmp, []byte(inside), 0o644) case fifo: - if err := syscall.Mkfifo(tmp, 0o644); err == nil { + if made = syscall.Mkfifo(tmp, 0o644); made == nil { if w, err := os.OpenFile(tmp, os.O_RDWR, 0); err == nil { writers = append(writers, w) } } default: - _ = os.Symlink(link, tmp) + made = os.Symlink(link, tmp) + } + if err := os.Rename(tmp, path); err == nil && made == nil && i%2 == 1 { + swaps.Add(1) } - _ = os.Rename(tmp, path) } }() t.Cleanup(func() { stop.Store(true); wg.Wait() }) + return &swaps } // WriteSentinelAt writes a file carrying Sentinel at p (creating its @@ -107,18 +114,41 @@ func AssertNoLeak(t *testing.T, path, inside string, d time.Duration, read func( AssertNoLeakFrom(t, path, inside, Outside(t), d, read) } +// InsideMarker must appear in the inside note's content and in read's +// output whenever read saw that note: the floor below counts such reads. +const InsideMarker = "inside" + // AssertNoLeakFrom is AssertNoLeak with the symlink pointing at target, e.g. // a file elsewhere in the brain written with WriteSentinelAt. -func AssertNoLeakFrom(t *testing.T, path, inside, target string, d time.Duration, read func() string) { +// +// Beyond "no Sentinel in any output" it sets two floors, because without +// them the assertion passes when nothing was tested (hyperguild#177): at +// least one symlink must have been swapped into path, and at least one read +// must have returned InsideMarker (a reader that always errors returns +// nothing, and so never leaks). +func AssertNoLeakFrom(t testing.TB, path, inside, target string, d time.Duration, read func() string) { t.Helper() - StartSwap(t, path, inside, target, false) - leaks, iters := 0, 0 + if !strings.Contains(inside, InsideMarker) { + t.Fatalf("inside content must contain %q", InsideMarker) + } + swaps := StartSwap(t, path, inside, target, false) + leaks, iters, insideReads := 0, 0, 0 for deadline := time.Now().Add(d); time.Now().Before(deadline); iters++ { - leaks += strings.Count(read(), Sentinel) + out := read() + leaks += strings.Count(out, Sentinel) + if strings.Contains(out, InsideMarker) { + insideReads++ + } } if leaks > 0 { t.Fatalf("outside file leaked %d times in %d iterations", leaks, iters) } + if n := swaps.Load(); n == 0 { + t.Fatalf("no symlink was swapped into %s in %d iterations: the race did not run", path, iters) + } + if insideReads == 0 { + t.Fatalf("no read in %d iterations returned the inside note (%q): the reader read nothing", iters, InsideMarker) + } } // AssertNoHang loops read for d while a FIFO swapper (held writers) runs on diff --git a/ingestion/internal/brainfs/brainfstest/brainfstest_test.go b/ingestion/internal/brainfs/brainfstest/brainfstest_test.go new file mode 100644 index 0000000..7dfc857 --- /dev/null +++ b/ingestion/internal/brainfs/brainfstest/brainfstest_test.go @@ -0,0 +1,79 @@ +package brainfstest + +import ( + "fmt" + "os" + "path/filepath" + "runtime" + "syscall" + "testing" + "time" +) + +// recordingTB stands in for the test under AssertNoLeakFrom so a fatal +// there can be observed instead of failing this test. Fatalf ends the +// calling goroutine the way testing.T does. +type recordingTB struct { + testing.TB + failed bool + msg string +} + +func (r *recordingTB) Fatalf(format string, args ...any) { + r.failed, r.msg = true, fmt.Sprintf(format, args...) + runtime.Goexit() +} + +func runAssert(t *testing.T, path, target string, read func() string) *recordingTB { + t.Helper() + r := &recordingTB{TB: t} + done := make(chan struct{}) + go func() { + defer close(done) + AssertNoLeakFrom(r, path, "inside", target, 200*time.Millisecond, read) + }() + <-done + return r +} + +// hyperguild#177: with every swap failing, no leak can occur, so the leak +// assertion passed without testing anything. +func TestAssertNoLeakFromFailsWhenNoSymlinkSwapSucceeds(t *testing.T) { + path := filepath.Join(t.TempDir(), "missing-dir", "r.md") + r := runAssert(t, path, Outside(t), func() string { return "inside" }) + if !r.failed { + t.Fatal("AssertNoLeakFrom passed with zero successful symlink swaps") + } +} + +// hyperguild#177: a reader that always errors returns nothing, so it never +// leaks either. +func TestAssertNoLeakFromFailsWhenNoReadReturnsTheInsideNote(t *testing.T) { + path := filepath.Join(t.TempDir(), "r.md") + r := runAssert(t, path, Outside(t), func() string { return "" }) + if !r.failed { + t.Fatal("AssertNoLeakFrom passed with zero reads returning the inside note") + } +} + +// Positive control: a reader that refuses symlinks and reads regular files +// passes, so the floors do not fail a working reader. +func TestAssertNoLeakFromPassesAReaderThatRefusesSymlinks(t *testing.T) { + path := filepath.Join(t.TempDir(), "r.md") + read := func() string { + f, err := os.OpenFile(path, os.O_RDONLY|noFollow, 0) + if err != nil { + return "" + } + defer func() { _ = f.Close() }() + b := make([]byte, 64) + n, _ := f.Read(b) + return string(b[:n]) + } + r := runAssert(t, path, Outside(t), read) + if r.failed { + t.Fatalf("a symlink-refusing reader failed: %s", r.msg) + } +} + +const noFollow = syscall.O_NOFOLLOW diff --git a/ingestion/internal/search/brainroot_test.go b/ingestion/internal/search/brainroot_test.go index 6acb636..7c70ad0 100644 --- a/ingestion/internal/search/brainroot_test.go +++ b/ingestion/internal/search/brainroot_test.go @@ -93,16 +93,20 @@ func TestQueryDoesNotFollowASymlinkedWingUnderAWingAndHallRoot(t *testing.T) { // bypass of readOpened hung Query after 5 iterations). // startSwap runs swapInto until the test ends and waits for it before // t.TempDir's cleanup (hyperguild#162). -func startSwap(t *testing.T, path, outside string, fifo bool) { +// It returns the count of symlinks or FIFOs actually renamed into path +// (hyperguild#177: a swapper whose every swap fails tests nothing). +func startSwap(t *testing.T, path, outside string, fifo bool) *atomic.Int64 { t.Helper() var stop atomic.Bool + var swaps atomic.Int64 var wg sync.WaitGroup wg.Add(1) - go func() { defer wg.Done(); swapInto(path, outside, fifo, &stop) }() + go func() { defer wg.Done(); swapInto(path, outside, fifo, &stop, &swaps) }() t.Cleanup(func() { stop.Store(true); wg.Wait() }) + return &swaps } -func swapInto(path, outside string, fifo bool, stop *atomic.Bool) { +func swapInto(path, outside string, fifo bool, stop *atomic.Bool, swaps *atomic.Int64) { tmp := path + ".swap" // Writers stay open until the swapper exits, so a reader parked on a // FIFO stays parked (closing one would hand the reader EOF and hide the @@ -115,33 +119,46 @@ func swapInto(path, outside string, fifo bool, stop *atomic.Bool) { }() for i := 0; !stop.Load(); i++ { _ = os.Remove(tmp) + var made error switch { case i%2 == 0: _ = os.WriteFile(tmp, []byte("# r\n\nzqinside "+"zqracenote"), 0o644) case fifo: - if err := syscall.Mkfifo(tmp, 0o644); err == nil { + if made = syscall.Mkfifo(tmp, 0o644); made == nil { if w, err := os.OpenFile(tmp, os.O_RDWR, 0); err == nil { writers = append(writers, w) } } default: - _ = os.Symlink(outside, tmp) + made = os.Symlink(outside, tmp) + } + if err := os.Rename(tmp, path); err == nil && made == nil && i%2 == 1 { + swaps.Add(1) } - _ = os.Rename(tmp, path) } } func TestQueryLeaksNothingUnderASymlinkSwapRace(t *testing.T) { brain, facts, outside := searchBrain(t) - startSwap(t, filepath.Join(facts, "r.md"), outside, false) - leaks, iters := 0, 0 + swaps := startSwap(t, filepath.Join(facts, "r.md"), outside, false) + leaks, iters, insideReads := 0, 0, 0 for deadline := time.Now().Add(1500 * time.Millisecond); time.Now().Before(deadline); iters++ { - rs, _ := Query(brain, QueryOptions{Query: sentinel157, Limit: 10}) + rs, err := Query(brain, QueryOptions{Query: sentinel157, Limit: 10}) + require.NoError(t, err) leaks += leaked(rs) + // Positive control (hyperguild#177): a Query that always errors + // returns nothing and so never leaks. + rs, err = Query(brain, QueryOptions{Query: "zqracenote", Limit: 10}) + require.NoError(t, err) + if len(rs) > 0 { + insideReads++ + } } if leaks > 0 { t.Fatalf("outside file leaked %d times in %d iterations", leaks, iters) } + require.Positive(t, swaps.Load(), "no symlink was swapped in: the race did not run") + require.Positive(t, insideReads, "no Query returned the inside note") } func TestQueryDoesNotHangUnderAFIFOSwapRace(t *testing.T) {