amadan
stopevidence tests: a holder is scanned only once it has stopped opening files (#274)
TestFDIdentityFindsAHardLinkHeldByAChild failed once in CI under load. Its holder was `sleep 60`, scanned as soon as exec.Cmd.Start returned. A process that has just been exec'd is still loading: sleep opens and closes /etc/ld.so.cache, libc.so.6, the locale archive and locale.alias in turn, all on descriptor 4. A scan that reads one of them between its close and the next open gets ENOENT from the stat through the descriptor and then finds the entry present, which FDHolders answers with ErrIncomplete by design (TestFDHoldersStatGoneWhileTheEntryRemains). The window is two consecutive syscalls wide, so it needs the scan to be descheduled there. The holders are now a shell that prints "ready" when it has reached its script and then blocks in its own read builtin, and the tests wait for that line. After it the holder has descriptors 0 to 3 and nothing else. The mount-namespace test had the same race in a different place (it waited for descriptor 3 and then scanned while sh was still exec'ing sleep), so its shell now opens descriptor 3 itself and ends the same way. Test-only. FDHolders is unchanged: a process that closes and reopens a descriptor number during a scan still makes it report ErrIncomplete.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
1 file changed,
+56
−32
internal/stopevidence/fdholders_alias_linux_test.go+56 −32
diff --git a/internal/stopevidence/fdholders_alias_linux_test.go b/internal/stopevidence/fdholders_alias_linux_test.go| index b0ee488ce..002a827d1 100644 |
| --- a/internal/stopevidence/fdholders_alias_linux_test.go |
| +++ b/internal/stopevidence/fdholders_alias_linux_test.go |
| @@ -3,8 +3,10 @@ |
| package stopevidence |
| import ( |
| + "bufio" |
| "context" |
| "errors" |
| + "fmt" |
| "io/fs" |
| "os" |
| "os/exec" |
| @@ -17,21 +19,62 @@ import ( |
| "time" |
| ) |
| -// holdOpen starts a child that holds path open as descriptor 3 and sleeps. |
| +// heldAndQuiet ends every holder's script: the shell says it has finished |
| +// starting, then blocks in its own read on a pipe nothing is written to. |
| +// |
| +// A scan must not meet a holder that is still starting. A process that has |
| +// just been exec'd opens and closes the linker cache, libc and the locale |
| +// files in turn, each on the first free descriptor. A scan that reads one of |
| +// them between its close and the next open sees a descriptor whose stat says |
| +// gone and whose entry is there, which is ErrIncomplete by design |
| +// (TestFDHoldersStatGoneWhileTheEntryRemains). The holders here used to be |
| +// `sleep 60`, scanned as soon as it was started, and the hard-link test |
| +// failed that way once under load (#274). |
| +const heldAndQuiet = "echo ready && read wait" |
| + |
| +// startHolder starts cmd, a shell whose script ends in heldAndQuiet, and |
| +// returns its pid once it has said ready. An error means it never did. |
| +func startHolder(t *testing.T, cmd *exec.Cmd) (int, error) { |
| + t.Helper() |
| + // Wait closes this end, and until then the child's read has nothing. |
| + if _, err := cmd.StdinPipe(); err != nil { |
| + t.Fatal(err) |
| + } |
| + r, w, err := os.Pipe() |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + defer r.Close() |
| + cmd.Stdout = w |
| + err = cmd.Start() |
| + w.Close() |
| + if err != nil { |
| + return 0, err |
| + } |
| + t.Cleanup(func() { cmd.Process.Kill(); cmd.Wait() }) |
| + r.SetReadDeadline(time.Now().Add(10 * time.Second)) |
| + if line, err := bufio.NewReader(r).ReadString('\n'); line != "ready\n" { |
| + return 0, fmt.Errorf("the holder said %q before it stopped: %v", line, err) |
| + } |
| + return cmd.Process.Pid, nil |
| +} |
| + |
| +// holdOpen starts a child that holds path open as descriptor 3 and opens |
| +// nothing more. |
| func holdOpen(t *testing.T, path string) int { |
| t.Helper() |
| f, err := os.Open(path) |
| if err != nil { |
| t.Fatal(err) |
| } |
| - cmd := exec.Command("sleep", "60") |
| + defer f.Close() |
| + cmd := exec.Command("sh", "-c", heldAndQuiet) |
| cmd.ExtraFiles = []*os.File{f} |
| - if err := cmd.Start(); err != nil { |
| + pid, err := startHolder(t, cmd) |
| + if err != nil { |
| t.Fatal(err) |
| } |
| - f.Close() |
| - t.Cleanup(func() { cmd.Process.Kill(); cmd.Wait() }) |
| - return cmd.Process.Pid |
| + return pid |
| } |
| // projectPIDs is a scan root holding only these processes' real /proc |
| @@ -124,35 +167,16 @@ func TestFDIdentityFindsAHolderInAnotherMountNamespace(t *testing.T) { |
| t.Fatal(err) |
| } |
| aliased := filepath.Join(alias, filepath.Base(db)) |
| + // The shell opens the database on descriptor 3 itself, after the mount, |
| + // and says ready only then: until that point descriptor 3 can be some |
| + // other file (the dynamic loader's libc, while unshare or sh starts). |
| cmd := exec.Command("unshare", "--user", "--map-root-user", "--mount", "sh", "-c", |
| - `mount --bind "$1" "$2" && exec sleep 60 3<"$3"`, "sh", filepath.Join(dir, "real"), alias, aliased) |
| - if err := cmd.Start(); err != nil { |
| - t.Skip("cannot start unshare:", err) |
| - } |
| - t.Cleanup(func() { cmd.Process.Kill(); cmd.Wait() }) |
| - pid := cmd.Process.Pid |
| - fd3 := filepath.Join("/proc", strconv.Itoa(pid), "fd", "3") |
| - // Until sh has run the mount and exec'd sleep, fd 3 can be some other |
| - // file (the dynamic loader's libc, while unshare or sh starts), so wait |
| - // for fd 3 to be the database itself, by identity: stat through the |
| - // magic link reaches the file the descriptor holds in any namespace. |
| - want, err := os.Stat(db) |
| + `mount --bind "$1" "$2" && exec 3<"$3" && `+heldAndQuiet, "sh", filepath.Join(dir, "real"), alias, aliased) |
| + pid, err := startHolder(t, cmd) |
| if err != nil { |
| - t.Fatal(err) |
| - } |
| - var target string |
| - held := false |
| - for deadline := time.Now().Add(5 * time.Second); time.Now().Before(deadline); time.Sleep(20 * time.Millisecond) { |
| - if fi, err := os.Stat(fd3); err == nil && os.SameFile(fi, want) { |
| - target, _ = os.Readlink(fd3) |
| - held = true |
| - break |
| - } |
| - } |
| - if !held { |
| - t.Skip("the namespace holder never came up holding the database (unprivileged user namespaces refused?)") |
| + t.Skip("the namespace holder never came up holding the database (unprivileged user namespaces refused?):", err) |
| } |
| - if target != aliased { |
| + if target, _ := os.Readlink(filepath.Join("/proc", strconv.Itoa(pid), "fd", "3")); target != aliased { |
| t.Logf("descriptor text %q (expected the bind path %q)", target, aliased) |
| } |
| root := projectPIDs(t, pid) |