diff --git a/internal/engine/epoll/driver.go b/internal/engine/epoll/driver.go index 23b8696e8..1800e7992 100644 --- a/internal/engine/epoll/driver.go +++ b/internal/engine/epoll/driver.go @@ -125,11 +125,33 @@ func (dc *driverConn) epollEvent(events uint32) unix.EpollEvent { // testHookCloseDriverClaimed, when non-nil, runs in closeDriver once the conn // is claimed closed and removed from the interest set, with no lock held, and // before the conn leaves driverConns: where UnregisterConn can return, the -// caller close the descriptor and register another conn on its number. Tests +// caller can close the descriptor and register another conn on its number. Tests // only (celeris#771): a test sets it before it creates the engine and clears // it after the engine is stopped. var testHookCloseDriverClaimed func(dc *driverConn) +// testHookDriverReadChecked, when non-nil, runs in driverRead after the conn's +// closed check and before its read(2), with dc.mu held: the one place where an +// UnregisterConn that is not made to wait for the read could return, so that +// the caller closes the number and another file takes it before the read goes +// out (celeris#710, #885 item 3). A reader that checks and reads in two +// critical sections, or reads and then checks, lets a test do exactly that +// from inside the hook, and the test sees the bytes of the file that took the +// number. Tests only: a test sets it before it starts the loop's reads and +// clears it after they have stopped. Costs one load and one nil test per read +// when unset. +var testHookDriverReadChecked func(dc *driverConn) + +// testHookCloseDriverDeleted, when non-nil, runs in closeDriver in the +// critical section that claims the conn closed, right after the +// EPOLL_CTL_DEL, with dc.mu held: the descriptor must already be out of the +// interest set there, or an UnregisterConn(A) that returns between the unlock +// and a later DEL lets the caller close the number and register another conn +// on it, which that DEL then removes (celeris#771, #885 item 23). Tests only: +// a test sets it before it creates the engine and clears it after the engine +// is stopped. +var testHookCloseDriverDeleted func(dc *driverConn) + // testHookConnClosed, when non-nil, runs in closeConn on the loop's goroutine // right after the descriptor is closed, with no lock held: the number is free // and a driver may take it, so the slot l.conns[fd] must already be clear @@ -346,7 +368,10 @@ func (l *Loop) RegisterConn(fd int, onRecv func([]byte), onClose func(error)) er // // Once UnregisterConn has returned, the worker reads fd no more, even when // it is inside the conn's read loop at the time (celeris#710): every read -// checks, under dc.mu, the closed flag this sets under dc.mu. +// checks, under dc.mu, the closed flag this sets under dc.mu. A read that +// completed before UnregisterConn took dc.mu is still delivered, though: the +// worker calls onRecv outside dc.mu, so onRecv can run once more, concurrently +// with or after onClose (celeris#885 item 4). func (l *Loop) UnregisterConn(fd int) error { l.driverMu.Lock() dc, ok := l.driverConns[fd] @@ -616,6 +641,9 @@ func (l *Loop) driverRead(dc *driverConn) (more bool) { dc.mu.Unlock() return false } + if h := testHookDriverReadChecked; h != nil { + h(dc) + } n, err := unix.Read(dc.fd, l.driverReadBuf) cb := dc.onRecv dc.mu.Unlock() @@ -662,6 +690,9 @@ func (l *Loop) closeDriver(dc *driverConn, cause error) { dc.closed = true cb := dc.onClose _ = l.driverEpollCtl(unix.EPOLL_CTL_DEL, dc.fd, nil) + if h := testHookCloseDriverDeleted; h != nil { + h(dc) + } dc.mu.Unlock() if h := testHookCloseDriverClaimed; h != nil { diff --git a/internal/engine/epoll/driver_critical_sections_885_linux_test.go b/internal/engine/epoll/driver_critical_sections_885_linux_test.go new file mode 100644 index 000000000..304af0e00 --- /dev/null +++ b/internal/engine/epoll/driver_critical_sections_885_linux_test.go @@ -0,0 +1,243 @@ +//go:build linux + +package epoll + +// celeris#885 items 3 and 23: the two critical sections of the driver paths +// that celeris#710 and celeris#771 rely on, each pinned by a nil-by-default +// seam inside it (testHookDriverReadChecked, testHookCloseDriverDeleted) +// instead of by where a test hook happens to sit relative to the code. + +import ( + "bytes" + "errors" + "testing" + "time" + + "golang.org/x/sys/unix" +) + +// bareDriverLoop885 is a Loop with an epoll instance of its own and nothing +// else: no engine, no worker goroutine, no driver conn registered. A test +// drives driverRead and closeDriver on its own goroutine, so the interleaving +// is the test's, and RegisterConn's ADD lands in an instance nobody waits on. +// The epoll descriptor is real: a zero epollFD would be stdin. +func bareDriverLoop885(t *testing.T) *Loop { + t.Helper() + epfd, err := unix.EpollCreate1(unix.EPOLL_CLOEXEC) + if err != nil { + t.Fatalf("epoll_create1: %v", err) + } + l := &Loop{epollFD: epfd, listenFD: -1, timerFD: -1} + t.Cleanup(l.closeEpollFD) + return l +} + +// TestDriverReadCheckAndReadAreOneCriticalSection885 is the epoll port of the +// standalone loop's TestReadCheckAndReadAreOneCriticalSection784 (#843 did the +// standalone loop only): driverRead's closed check and its read(2) must be one +// critical section with the flag UnregisterConn sets before it returns. A +// reader that checks and then reads in a separate step passes the #710 tests +// (driver_unregister_readloop_test.go), which unregister while the reader is +// parked in onRecv, before its next check; #772's surviving mutant m1-toctou is +// such a reader. It fails here: testHookDriverReadChecked runs between the +// check and the read, and from inside it the test starts UnregisterConn(A) on +// another goroutine and gives it 300 ms. +// +// If UnregisterConn returns in that time, the reader has been overtaken +// between its check and its read: the test closes A, moves X onto A's number +// with bytes waiting, and lets the read go, which then reads X. If it does not +// return, UnregisterConn is waiting for the read, as it must; the test does the +// close and the reuse once it has returned, and then offers the conn a read +// turn for the old number, which must read nothing. +func TestDriverReadCheckAndReadAreOneCriticalSection885(t *testing.T) { + l := bareDriverLoop885(t) + a, aPeer := socketpairNonblocking(t) + t.Cleanup(func() { _ = unix.Close(aPeer) }) + if _, err := unix.Write(aPeer, []byte("AAAA")); err != nil { + t.Fatalf("queue A's bytes: %v", err) + } + + const stolen = "XXXXXXXXXX" + var ( + armed = true + hookRan bool + overtaken bool + aClosed bool + x0, x1 = -1, -1 + unregDone = make(chan error, 1) + chunks [][]byte + closes int + ) + // reuse is what the caller does once UnregisterConn(A) has returned: close + // A's descriptor, and put a new socket X, with bytes waiting, on its number. + reuse := func() { + if err := unix.Close(a); err != nil { + t.Fatalf("close A: %v", err) + } + aClosed = true + x0, x1 = takeNumber710(t, a, true) + if _, err := unix.Write(x1, []byte(stolen)); err != nil { + t.Fatalf("queue X's bytes: %v", err) + } + } + t.Cleanup(func() { + if x0 >= 0 { + closeX710(a, x0, x1) + } else if !aClosed { + _ = unix.Close(a) + } + }) + // This test starts no worker, so no other goroutine reads the var. + t.Cleanup(func() { testHookDriverReadChecked = nil }) + testHookDriverReadChecked = func(dc *driverConn) { + if dc.fd != a || !armed { + return + } + armed = false + hookRan = true + go func() { unregDone <- l.UnregisterConn(a) }() + select { + case err := <-unregDone: + if err != nil { + t.Errorf("UnregisterConn(A): %v", err) + } + overtaken = true + reuse() + case <-time.After(300 * time.Millisecond): + } + } + + if err := l.RegisterConn(a, func(b []byte) { + chunks = append(chunks, append([]byte(nil), b...)) + }, func(error) { closes++ }); err != nil { + t.Fatalf("RegisterConn(A): %v", err) + } + dc := l.lookupDriver(a) + if dc == nil { + t.Fatal("A is not registered") + } + + l.driverRead(dc) + if !hookRan { + t.Fatal("testHookDriverReadChecked never ran for A: driverRead did not go through the hooked read") + } + if !overtaken { + select { + case err := <-unregDone: + if err != nil { + t.Fatalf("UnregisterConn(A): %v", err) + } + case <-time.After(5 * time.Second): + t.Fatal("UnregisterConn(A) did not return within 5 s of the read") + } + reuse() + // A turn the loop still owed A when it was unregistered (the queue holds + // conns, not numbers): the conn is gone, so it must read nothing. + if more := l.driverRead(dc); more { + t.Error("driverRead of an unregistered conn reports bytes left") + } + } + + got, rerr := readAvailable(a) + stole := false + for _, c := range chunks { + if bytes.Contains(c, []byte(stolen)) { + stole = true + } + } + t.Logf("885 critical-section: UnregisterConn returned while the reader was between its check and its read: %v; "+ + "onRecv got %d chunk(s), X's bytes among them: %v; onClose ran %d time(s); X (fd %d) reads %q (err %v)", + overtaken, len(chunks), stole, closes, a, got, rerr) + if overtaken { + t.Errorf("UnregisterConn(A) returned while the reader was between its closed check and its read: the two are not one critical section") + } + if stole { + t.Errorf("A's onRecv got X's bytes: the reader read A's number after UnregisterConn(A) returned and X took it") + } + if got != stolen { + t.Errorf("X, which took A's number, reads %q (err %v), want %q: the reader consumed X's bytes", got, rerr, stolen) + } + if len(chunks) != 1 || string(chunks[0]) != "AAAA" { + t.Errorf("A's onRecv got %q, want exactly A's own \"AAAA\" (the read UnregisterConn waited for is still delivered)", chunks) + } + if closes != 1 { + t.Errorf("A's onClose ran %d times, want 1", closes) + } +} + +// TestCloseDriverDeletesInsideItsClaimSection885: closeDriver removes the +// descriptor from the interest set in the critical section that claims the +// conn closed, not after it. celeris#771: UnregisterConn takes dc.mu before it +// returns, so while closeDriver holds dc.mu from the claim to the DEL, the +// caller can neither return nor close the number and register another conn on +// it. TestCloseDriverSparesConnOnReusedNumber771 pins where the DEL sits +// relative to its own hook (testHookCloseDriverClaimed, which runs with no +// lock held), which a DEL moved to just after dc.mu.Unlock() still satisfies; +// that DEL has #771's second window, and it passes every 771 and 775 test. This +// one runs inside the section (testHookCloseDriverDeleted) and asserts three +// things there: dc.mu is held, the conn is claimed closed, and the descriptor +// is out of the interest set, which EPOLL_CTL_MOD answers with ENOENT. +// +// The same MOD, made before closeDriver, must succeed: the ENOENT afterwards is +// the DEL's, not a wrong epoll descriptor's or a wrong event's. +func TestCloseDriverDeletesInsideItsClaimSection885(t *testing.T) { + l := bareDriverLoop885(t) + a, aPeer := socketpairNonblocking(t) + t.Cleanup(func() { _ = unix.Close(a); _ = unix.Close(aPeer) }) + closes := 0 + if err := l.RegisterConn(a, func([]byte) {}, func(error) { closes++ }); err != nil { + t.Fatalf("RegisterConn(A): %v", err) + } + dc := l.lookupDriver(a) + if dc == nil { + t.Fatal("A is not registered") + } + + mod := func() error { + ev := dc.epollEvent(unix.EPOLLIN | unix.EPOLLET | unix.EPOLLRDHUP) + return l.driverEpollCtl(unix.EPOLL_CTL_MOD, dc.fd, &ev) + } + if err := mod(); err != nil { + t.Fatalf("PREMISE: EPOLL_CTL_MOD of the registered descriptor: %v (the ENOENT below would prove nothing)", err) + } + + var ( + ran, held, claimed bool + modErr error + ) + testHookCloseDriverDeleted = func(got *driverConn) { + ran = true + if got != dc { + t.Errorf("the hook was given another conn") + } + if held = !dc.mu.TryLock(); !held { + dc.mu.Unlock() + } + claimed = dc.closed + modErr = mod() + } + t.Cleanup(func() { testHookCloseDriverDeleted = nil }) + + l.closeDriver(dc, nil) + + if !ran { + t.Fatal("testHookCloseDriverDeleted never ran: closeDriver did not reach its DEL") + } + if !held { + t.Error("dc.mu was not held at the DEL: the DEL is outside the critical section that claims the conn closed") + } + if !claimed { + t.Error("the conn was not claimed closed at the DEL") + } + if !errors.Is(modErr, unix.ENOENT) { + t.Errorf("EPOLL_CTL_MOD in the claim section after the DEL: %v, want ENOENT: the descriptor was still in the "+ + "interest set, so an UnregisterConn(A) that returns before the DEL lets the caller close the number "+ + "and register another conn the DEL then removes (celeris#771)", modErr) + } + if closes != 1 { + t.Errorf("onClose ran %d times, want 1", closes) + } + if l.lookupDriver(a) != nil { + t.Error("the conn is still in the map after closeDriver") + } +} diff --git a/internal/engine/epoll/driver_test.go b/internal/engine/epoll/driver_test.go index 891d1b50f..647970cd8 100644 --- a/internal/engine/epoll/driver_test.go +++ b/internal/engine/epoll/driver_test.go @@ -8,6 +8,7 @@ import ( "errors" "fmt" "net" + "os" "runtime" "runtime/debug" "strconv" @@ -414,6 +415,89 @@ func TestDriverUnknownFDReturnsErr(t *testing.T) { } } +// TestRegisterConnRollsBackAFailedEpollAdd885 (celeris#885 item 8, from the +// review of #787): RegisterConn publishes the conn (map entry, hasDriverConns) +// before it arms the descriptor, so a refused EPOLL_CTL_ADD must take both +// back. A regular file's descriptor is refused with EPERM, which is +// deterministic. After the failure the number is unknown to the loop (Write and +// UnregisterConn say ErrUnknownFD), the HTTP path is not made to look every +// event up (hasDriverConns false, the zero-overhead gate), and the failed +// registration did not take a registered neighbour's place: the second part +// registers a good conn first, and the flag must stay set for it. +func TestRegisterConnRollsBackAFailedEpollAdd885(t *testing.T) { + regularFile := func(t *testing.T) int { + t.Helper() + f, err := os.CreateTemp(t.TempDir(), "regfile") + if err != nil { + t.Fatalf("create temp file: %v", err) + } + t.Cleanup(func() { _ = f.Close() }) + return int(f.Fd()) + } + driverConnCount := func(l *Loop) int { + l.driverMu.RLock() + defer l.driverMu.RUnlock() + return len(l.driverConns) + } + closed := 0 + onClose := func(error) { closed++ } + + t.Run("no other conn", func(t *testing.T) { + l := bareDriverLoop885(t) + fd := regularFile(t) + err := l.RegisterConn(fd, func([]byte) {}, onClose) + if !errors.Is(err, unix.EPERM) { + t.Fatalf("RegisterConn of a regular file's descriptor: %v, want EPERM from EPOLL_CTL_ADD", err) + } + if l.hasDriverConns.Load() { + t.Error("hasDriverConns is still set after the failed registration of the loop's only conn") + } + if dc := l.lookupDriver(fd); dc != nil { + t.Errorf("lookupDriver(%d) = %v after the failed registration, want nil", fd, dc) + } + if n := driverConnCount(l); n != 0 { + t.Errorf("%d conn(s) in driverConns after the failed registration, want 0", n) + } + if err := l.Write(fd, []byte("x")); !errors.Is(err, celerisengine.ErrUnknownFD) { + t.Errorf("Write after the failed registration: %v, want ErrUnknownFD", err) + } + if err := l.UnregisterConn(fd); !errors.Is(err, celerisengine.ErrUnknownFD) { + t.Errorf("UnregisterConn after the failed registration: %v, want ErrUnknownFD", err) + } + if closed != 0 { + t.Errorf("onClose ran %d time(s) for a conn that was never registered", closed) + } + }) + + t.Run("another conn registered", func(t *testing.T) { + l := bareDriverLoop885(t) + b, bPeer := socketpairNonblocking(t) + t.Cleanup(func() { _ = unix.Close(b); _ = unix.Close(bPeer) }) + if err := l.RegisterConn(b, func([]byte) {}, func(error) {}); err != nil { + t.Fatalf("RegisterConn(B): %v", err) + } + fd := regularFile(t) + if err := l.RegisterConn(fd, func([]byte) {}, onClose); !errors.Is(err, unix.EPERM) { + t.Fatalf("RegisterConn of a regular file's descriptor: %v, want EPERM", err) + } + if !l.hasDriverConns.Load() { + t.Error("hasDriverConns was cleared by a failed registration while another driver conn is registered") + } + if l.lookupDriver(b) == nil { + t.Error("the failed registration removed the other conn from the map") + } + if l.lookupDriver(fd) != nil { + t.Errorf("the failed registration's conn is still in the map") + } + if err := l.UnregisterConn(b); err != nil { + t.Fatalf("UnregisterConn(B): %v", err) + } + if l.hasDriverConns.Load() { + t.Error("hasDriverConns is still set after the last conn was unregistered") + } + }) +} + // currentGoroutineID parses "goroutine N [running]:" from the first line // of runtime.Stack. Test-only; not used on the hot path. func currentGoroutineID() int64 {