From f5beffe485885801a3c2b1ddeb3d40686c7b5336 Mon Sep 17 00:00:00 2001 From: Segfault <5221072+Segfaultd@users.noreply.github.com> Date: Thu, 8 Oct 2026 13:56:14 +0200 Subject: [PATCH] test(sessionconfig): stop waits from destroying evidence a later assertion needs InteractiveAcceptGatesBothConnectionPackets is commented as "the load-bearing assertion" -- neither peer may report a connection until the server answers the session request -- but it could not detect that violation at all. PumpUntil() deallocates every packet that is not the id it waits for, on the waited-on peer as well as on alsoPump. The test's first action waits for ID_SESSION_CONFIG_REQUEST on the server, so a connection packet produced early is drained and freed there, before the negative assertion that looks for it ever runs. Proved with a library mutant that reports the connection instead of withholding it: the test passed 10/10, while the probe showed PumpUntil(want=128) discarding id=19 from the waited-on peer. The test also built its two negative assertions out of successive SawWithin() probes, which swallow each other's evidence -- the hazard CollectIds()'s own comment forbids. Fixes both: - PumpUntil() takes optional ledgers recording the ids it drains and discards, from the waited-on peer and from alsoPump. Default-null, so no existing call site changes. - CollectIdsBoth() drains two peers in one loop, appending, so a paired negative assertion cannot pass because the other probe consumed its packet. - The gating test carries both ledgers across the wait and asserts over them. An audit of the suite found exactly 2 at-risk negative assertions, both in this test; every other one already uses the collect-once pattern. Verified on macOS arm64. Against the withholding mutant the test now fails 5/5 deterministically in ~630 ms naming the side ("server reported a connection before answering the session request"), where it previously passed 10/10. The payload-storing mutant is still caught. Clean library: SessionConfigLive.* x10 green, full ctest 276/276. Refs #64 --- Tests/Integration/SessionConfigLiveTests.cpp | 54 +++++++++++++++++--- 1 file changed, 48 insertions(+), 6 deletions(-) diff --git a/Tests/Integration/SessionConfigLiveTests.cpp b/Tests/Integration/SessionConfigLiveTests.cpp index cb3123203..61c8e251f 100644 --- a/Tests/Integration/SessionConfigLiveTests.cpp +++ b/Tests/Integration/SessionConfigLiveTests.cpp @@ -51,7 +51,15 @@ namespace // Drain a peer, returning the first packet with the given id, or 0 if the deadline passes. // Packets that are not the wanted id are discarded; both peers are pumped so a handshake that // needs traffic from either side can make progress. - Packet *PumpUntil(RakPeerInterface *wanted, int wantedId, RakPeerInterface *alsoPump, int timeoutMs) + // + // The optional ledgers record the id of every packet this helper drains and throws away, from the + // waited-on peer and from alsoPump respectively. A negative assertion placed AFTER a wait cannot + // otherwise see those packets -- they were deallocated in here -- so a violation that surfaces + // early is swallowed by the very wait that precedes the assertion looking for it. Pass a ledger + // whenever a later assertion needs to know what already went by; see + // InteractiveAcceptGatesBothConnectionPackets. + Packet *PumpUntil(RakPeerInterface *wanted, int wantedId, RakPeerInterface *alsoPump, int timeoutMs, + std::vector *seenOnWanted = 0, std::vector *seenOnAlsoPump = 0) { TimeMS entry = GetTimeMS(); while (GetTimeMS() - entry < (TimeMS)timeoutMs) @@ -61,11 +69,16 @@ namespace { if (p->data[0] == (unsigned char)wantedId) return p; // caller deallocates + if (seenOnWanted) + seenOnWanted->push_back((int)p->data[0]); } if (alsoPump) { for (p = alsoPump->Receive(); p; alsoPump->DeallocatePacket(p), p = alsoPump->Receive()) - ; + { + if (seenOnAlsoPump) + seenOnAlsoPump->push_back((int)p->data[0]); + } } RakSleep(15); } @@ -149,6 +162,27 @@ namespace return ids; } + // Drain BOTH peers over a window, appending every id each one yields to its own ledger. + // + // The single-peer CollectIds() above discards the other peer's packets, and two successive + // SawWithin()/CollectIds() probes swallow each other's evidence for the same reason. A negative + // assertion about a PAIR of events -- neither peer may report a connection -- therefore has to + // drain both in one loop. Appends rather than assigns so a ledger already carrying what an earlier + // PumpUntil() drained keeps it. + void CollectIdsBoth(RakPeerInterface *a, std::vector *aIds, RakPeerInterface *b, std::vector *bIds, int windowMs) + { + TimeMS entry = GetTimeMS(); + while (GetTimeMS() - entry < (TimeMS)windowMs) + { + Packet *p; + for (p = a->Receive(); p; a->DeallocatePacket(p), p = a->Receive()) + aIds->push_back((int)p->data[0]); + for (p = b->Receive(); p; b->DeallocatePacket(p), p = b->Receive()) + bIds->push_back((int)p->data[0]); + RakSleep(15); + } + } + bool Contains(const std::vector &ids, int id) { for (size_t i = 0; i < ids.size(); ++i) @@ -326,7 +360,12 @@ TEST_F(SessionConfigLive, InteractiveAcceptGatesBothConnectionPackets) const unsigned short port = StartPeers(); ASSERT_EQ(client->Connect("127.0.0.1", port, 0, 0), CONNECTION_ATTEMPT_STARTED); - Packet *request = PumpUntil(server, ID_SESSION_CONFIG_REQUEST, client, kConnectTimeoutMs); + // Ledgers, because the wait below drains and frees everything that is not the session request -- + // including an early connection packet, which is precisely the violation this test exists to + // catch. Asserting only over a window that starts after the wait cannot see it. + std::vector serverSaw; + std::vector clientSaw; + Packet *request = PumpUntil(server, ID_SESSION_CONFIG_REQUEST, client, kConnectTimeoutMs, &serverSaw, &clientSaw); ASSERT_NE(request, nullptr) << "server never saw the session request"; const RakNetGUID clientGuid = request->guid; @@ -335,10 +374,13 @@ TEST_F(SessionConfigLive, InteractiveAcceptGatesBothConnectionPackets) EXPECT_EQ(memcmp(request->data + 1, kClientPayload, strlen(kClientPayload)), 0); server->DeallocatePacket(request); - // Nothing may be reported while the decision is outstanding. - EXPECT_FALSE(SawWithin(server, ID_NEW_INCOMING_CONNECTION, client, 400)) + // Nothing may be reported while the decision is outstanding. Both peers are drained in one loop + // and both ledgers carry forward what the wait above already consumed, so neither assertion can + // pass because the other probe -- or the wait -- swallowed its evidence. + CollectIdsBoth(server, &serverSaw, client, &clientSaw, 400); + EXPECT_FALSE(Contains(serverSaw, ID_NEW_INCOMING_CONNECTION)) << "server reported a connection before answering the session request"; - EXPECT_FALSE(SawWithin(client, ID_CONNECTION_REQUEST_ACCEPTED, server, 400)) + EXPECT_FALSE(Contains(clientSaw, ID_CONNECTION_REQUEST_ACCEPTED)) << "client reported a connection before the server answered"; server->AcceptSession(clientGuid, kServerPayload, (unsigned int)strlen(kServerPayload));