diff --git a/MobileGL/MG_Remote/Server/ServerMain.cpp b/MobileGL/MG_Remote/Server/ServerMain.cpp index 62eeb55b..02fa7d81 100644 --- a/MobileGL/MG_Remote/Server/ServerMain.cpp +++ b/MobileGL/MG_Remote/Server/ServerMain.cpp @@ -23,6 +23,7 @@ #include #include #include +#include #include #include #include @@ -191,7 +192,8 @@ enum class DataSource { None, Handoff, Listener }; // cut for reading FIRST (after shutdown(SHUT_RD) every further sendmsg to it fails with EPIPE, which // FdPassing reports as "peer gone" and the supervisor answers by name), and then whatever was // queued before the cut is received here and refused with the supervisor's own words. No -// DataBind can be in neither place. +// DataBind can be in neither place - on the way out after Accept here, and on every way out +// before it through ExitBeforeAccept below; only a crashed child still drops them. void CloseHandoffRefusingQueued(int handoff) { (void)::shutdown(handoff, SHUT_RD); for (;;) { @@ -207,12 +209,42 @@ void CloseHandoffRefusingQueued(int handoff) { ::close(handoff); } +// ID-P7-44, f2-auth fix round. EVERY WAY OUT OF A SESSION CHILD BEFORE ACCEPT LETS GO OF THE +// HAND-OFF THE WAY THE WAY OUT AFTER IT DOES. The supervisor forwards DataBinds to a child from the +// moment it forks it, so a child that refused its Hello at ValidatePeerHandshake, refused its +// backend, or could not bring one up used to `_exit` with DataBinds queued on the hand-off - and an +// exit drops queued descriptors exactly as a close does: those peers saw their connections end +// with no frame. On any other source (the single-session listener, the unix endpoint's none) there +// is nothing queued to refuse, and this is the `fflush` + `_exit` it replaces. +[[noreturn]] void ExitBeforeAccept(int sourceFd, DataSource sourceKind, int code) { + if (sourceFd >= 0 && sourceKind == DataSource::Handoff) CloseHandoffRefusingQueued(sourceFd); + std::fflush(nullptr); + ::_exit(code); +} + +// A TEST-ONLY lever (f2-auth fix round, ID-P7-44): MOBILEGL_TEST_DELAY_SESSION_HANDSHAKE_MS holds a +// session child for that many milliseconds before it looks at its Hello, so a control can queue +// DataBinds on the hand-off of a child that is about to refuse that Hello - a window of +// microseconds otherwise. Like MOBILEGL_TEST_DELAY_FIRST_CAPS_MS (ServerSession.cpp) it exists only +// in the disaggregated server, does nothing unless set, and says so when it is. +void DelaySessionHandshakeForTest() { + const char* text = std::getenv("MOBILEGL_TEST_DELAY_SESSION_HANDSHAKE_MS"); + if (text == nullptr || *text == '\0') return; + const long ms = std::strtol(text, nullptr, 10); + if (ms <= 0) return; + WireLogError("MG_Remote server: pid=%d MOBILEGL_TEST_DELAY_SESSION_HANDSHAKE_MS=%ld - this session child " + "waits that long before its handshake. A test-only lever; never set it in a measured run", + static_cast(::getpid()), ms); + std::this_thread::sleep_for(std::chrono::milliseconds(std::min(ms, 60000))); +} + // Called only after fork (or, in the single-session shape, instead of it): the supervisor never // creates a backend or EGL context. `dataSource` is set on TCP (PH-7 (4)) and empty on a unix // endpoint, whose second connection is the SCM_RIGHTS socket AcceptPair already paired. // `sourceFd` is the descriptor that source reads from - the child's end of the hand-off // socketpair under --serve, the listener itself in the single-session shape - and -1 when there -// is none; it is let go of the moment Accept returns. +// is none; it is let go of the moment Accept returns, and on every exit before that +// (ExitBeforeAccept, ID-P7-44). // // `helloFrame` is the first frame when somebody has already read and authenticated it - the TCP // supervisor always has (PH-7 (5)) - and empty when this function reads it itself, within @@ -230,15 +262,13 @@ void CloseHandoffRefusingQueued(int handoff) { const auto received = control->ReceiveFrame({nullptr, 0}, &bytes, helloWaitMs); if (received == MOBILEGL_ERR_TIMEOUT) { Refuse(*control, Protocol::RefuseCode::Authentication, Server::kPreAuthDeadline); - std::fflush(nullptr); - ::_exit(0); + ExitBeforeAccept(sourceFd, sourceKind, 0); } if (received == MOBILEGL_ERR_PROTOCOL_MISMATCH || (received == MOBILEGL_ERR_BUFFER_TOO_SMALL && bytes > Server::kPreAuthFirstFrameMaxBytes)) { DrainUnread(control->StreamFd()); Refuse(*control, Protocol::RefuseCode::MalformedHello, Server::kFirstFrameNotAFrame); - std::fflush(nullptr); - ::_exit(0); + ExitBeforeAccept(sourceFd, sourceKind, 0); } if (received == MOBILEGL_ERR_TRANSPORT_CLOSED) { // Fix round: this used to be a bare `_exit(0)` whatever had arrived. A close INSIDE @@ -249,8 +279,7 @@ void CloseHandoffRefusingQueued(int handoff) { Refuse(*control, Protocol::RefuseCode::MalformedHello, Server::kFirstFrameTruncated); else WireLogError("MG_Remote server: pid=%d control peer closed before sending a first frame", selfPid); - std::fflush(nullptr); - ::_exit(0); + ExitBeforeAccept(sourceFd, sourceKind, 0); } if (received == MOBILEGL_ERR_BUFFER_TOO_SMALL) { // The reassembler holds the whole frame already; this second call copies it out. @@ -258,19 +287,18 @@ void CloseHandoffRefusingQueued(int handoff) { if (control->ReceiveFrame({helloFrame.data(), helloFrame.size()}, &bytes, 0) != MOBILEGL_OK) { WireLogError("MG_Remote server: pid=%d first frame of %llu bytes could not be copied out", selfPid, static_cast(helloFrame.size())); - std::fflush(nullptr); - ::_exit(0); + ExitBeforeAccept(sourceFd, sourceKind, 0); } } else if (received != MOBILEGL_OK) { WireLogError("MG_Remote server: pid=%d first frame could not be read (rc=%d)", selfPid, static_cast(received)); - std::fflush(nullptr); - ::_exit(0); + ExitBeforeAccept(sourceFd, sourceKind, 0); } // MOBILEGL_OK with nothing copied is a ZERO-LENGTH frame. It was the one complete frame // this read exited on without a word (fix round); `helloFrame` stays empty and the // classification below names it, as the TCP supervisor's does ("no identifier"). } + DelaySessionHandshakeForTest(); // THE FIRST FRAME, CLASSIFIED BY THE SAME FUNCTION THE TCP SUPERVISOR USES (plan ยง1.1, fuzz // arm 1 row: "ServerMain.cpp:193 CtrlEnvelopeBufferHasIdentifier"). The identifier is asked // first, and every malformed shape is refused BY NAME - this read used to `_exit(67)` on all @@ -283,13 +311,11 @@ void CloseHandoffRefusingQueued(int handoff) { // arm is the single-session shape's.) if (shape == Server::FirstFrameShape::DataBind) { Refuse(*control, Protocol::RefuseCode::Authentication, "data connection names no live session"); - std::fflush(nullptr); - ::_exit(0); + ExitBeforeAccept(sourceFd, sourceKind, 0); } if (shape != Server::FirstFrameShape::Hello) { Refuse(*control, Protocol::RefuseCode::MalformedHello, Server::FirstFrameRefusalDetail(shape)); - std::fflush(nullptr); - ::_exit(0); + ExitBeforeAccept(sourceFd, sourceKind, 0); } const auto* hello = Protocol::GetCtrlEnvelope(helloFrame.data())->msg_as_Hello(); // PH-7 (1). The SAME policy ServerSession::Accept applies, from Handshake.h, so the @@ -306,11 +332,11 @@ void CloseHandoffRefusingQueued(int handoff) { // this build's stamp. An unauthenticated peer now learns only Refuse{Authentication}. On TCP // --serve the supervisor has already asked (the answer is the same; it is asked again so this // function holds the rule on its own for the unix and single-session shapes). - if (AuthenticatePeerToken(*control, hello->token()) != MOBILEGL_OK) ::_exit(0); + if (AuthenticatePeerToken(*control, hello->token()) != MOBILEGL_OK) ExitBeforeAccept(sourceFd, sourceKind, 0); // Reject incompatible wire before backend bring-up can obscure the cause. if (ValidatePeerHandshake(*control, hello->abiMajor(), hello->abiMinor(), hello->wireFingerprint(), hello->buildFingerprint() ? hello->buildFingerprint()->c_str() : nullptr, - hello->dialMode()) != MOBILEGL_OK) ::_exit(0); + hello->dialMode()) != MOBILEGL_OK) ExitBeforeAccept(sourceFd, sourceKind, 0); MobileGL::MG_ConfigLoader::Init(); MobileGL::MG_Config::Transport = MobileGL::MG_Config::TransportMode::Spawn; MobileGL::MG_Pipe::MGPipeSetServerProcessRole(true); @@ -324,21 +350,21 @@ void CloseHandoffRefusingQueued(int handoff) { const auto backend = static_cast(hello->backendType()); if (backend != MobileGL::BackendType::DirectGLES && backend != MobileGL::BackendType::DirectVulkan) { Refuse(*control, Protocol::RefuseCode::Backend, "unsupported Hello.backendType"); - ::_exit(0); + ExitBeforeAccept(sourceFd, sourceKind, 0); } const char* pinned = std::getenv("MOBILEGL_BACKEND_TYPE"); if (pinned && *pinned && backend != MobileGL::MG_Config::ActiveBackendType) { Refuse(*control, Protocol::RefuseCode::Backend, "Hello.backendType disagrees with pinned backend"); - ::_exit(0); + ExitBeforeAccept(sourceFd, sourceKind, 0); } MobileGL::MG_Config::ActiveBackendType = backend; - if (!MobileGL::MG_Backend::InitServerRoleForSpawn()) ::_exit(66); + if (!MobileGL::MG_Backend::InitServerRoleForSpawn()) ExitBeforeAccept(sourceFd, sourceKind, 66); auto& session = Server::ServerSessionInstance(); int serverBell[2] = {-1, -1}, clientBell[2] = {-1, -1}; std::unique_ptr selfBell, peerBell; if (!tcp) { if (::socketpair(AF_UNIX, SOCK_STREAM, 0, serverBell) != 0 || - ::socketpair(AF_UNIX, SOCK_STREAM, 0, clientBell) != 0) ::_exit(74); + ::socketpair(AF_UNIX, SOCK_STREAM, 0, clientBell) != 0) ExitBeforeAccept(sourceFd, sourceKind, 74); selfBell = std::make_unique(serverBell[0], serverBell[1], 1, true); peerBell = std::make_unique(-1, clientBell[1], 1, true); session.SetExternalDoorbells(selfBell.get(), peerBell.get()); @@ -652,7 +678,8 @@ void LogSupervisorSummary(unsigned long sessionsFaulted) { // // - every new connection is PENDING until its first frame is complete. The supervisor reads it // itself, a few bytes per wakeup, from all pending connections at once (one poll loop - no -// peer can make it wait for another), and never a byte past the frame; +// unauthenticated peer can make it wait for another; the one wait left is up to 50 ms for an +// exiting session before an AUTHENTICATED Hello is told Busy), and never a byte past the frame; // - a pending connection has MOBILEGL_IPC_PREAUTH_MS (default 2000) to complete it, and at most // MOBILEGL_IPC_PREAUTH_MAX (default 8) are pending at once. The queue is SHARED between // addresses: when it is full a newcomer displaces the oldest connection of the address holding @@ -771,8 +798,10 @@ private: // exit_group closes files just before waitpid can observe the exit. Allow that small // scheduling window before deciding whether a session is live - the old loop gave it to every - // connection it accepted while a child existed; this gives it to the two decisions that - // depend on the answer (a DataBind's routing, an authenticated Hello's Busy). + // connection it accepted while a child existed; this gives it to the one decision that + // depends on the answer: an AUTHENTICATED Hello's Busy (a client reconnecting the moment its + // last session ended must not be told Busy by a zombie). Only a peer holding the token can + // make the loop wait here; a DataBind's routing takes one look (Judge, fix round). void WaitOutAnExitingSession() { const auto reapDeadline = Clock::now() + std::chrono::milliseconds(50); while (m_active > 0 && Clock::now() < reapDeadline) { @@ -851,7 +880,12 @@ private: // from a session that just ended is a legitimate client's. std::uint8_t nonce[kDataNonceBytes] = {}; (void)DecodeDataBind(payload, nonce); - WaitOutAnExitingSession(); + // One look, not WaitOutAnExitingSession's 50 ms (fix round): DataBinds are + // unauthenticated and never counted, so waiting for each would let any peer stall this + // loop 50 ms per connection. Nothing needs the wait here - a child that is exiting + // either has cut its hand-off already (the send below fails and is refused by name) or + // refuses what is queued on its way out (ExitBeforeAccept / CloseHandoffRefusingQueued). + Reap(); if (m_active <= 0) { // StreamLink's frames may already follow the DataBind; read them away so the close // does not become an RST that discards the refusal. diff --git a/scripts/ci/tcp_supervisor_smoke.py b/scripts/ci/tcp_supervisor_smoke.py index 14a32655..6b272013 100644 --- a/scripts/ci/tcp_supervisor_smoke.py +++ b/scripts/ci/tcp_supervisor_smoke.py @@ -20,8 +20,10 @@ deadline (`silent_peer`), that Busy is the answer to an AUTHENTICATED second cli pending cap and the backoff (`pending_cap`, `auth_backoff`), and ID-P7-44's queued-DataBind refusal (`data_bind_queued_before_bound`). The f2-auth fix round added that the pending queue is shared between addresses (`pending_queue_is_shared`) and that an address holding slots with silent -connections is backed off (`silent_slot_holder_backed_off`). The five malformed-frame shapes are -fuzz arm 1's own entry, scripts/ci/ph_fuzz_control_frames.py. +connections is backed off (`silent_slot_holder_backed_off`), that routing DataBinds does not stall +the supervisor (`data_bind_routing_does_not_stall`), and that a child which refuses its Hello +refuses the DataBinds queued to it (`data_bind_queued_before_refused_hello`, ID-P7-44). The five +malformed-frame shapes are fuzz arm 1's own entry, scripts/ci/ph_fuzz_control_frames.py. It needs flatc, which is deliberately absent from the default build graph (gen_protocol.py's header block says why). Resolution order is gen_protocol.py's, minus the build-it-for-you arm: @@ -731,6 +733,42 @@ def main(): 'fresh_control': still_busy} assert process.poll() is None, 'the refused DataBinds killed the supervisor' + # ---- f2-auth fix round: ROUTING A DataBind DOES NOT STALL THE SUPERVISOR ------------- + # + # While a session was live the supervisor waited up to 50 ms for it to exit before + # routing EACH DataBind - and a DataBind is unauthenticated and never counted, so any + # peer could hold the one poll loop 50 ms per connection it opened. Now it takes one + # look. Twenty DataBinds into a bound session, opened four at a time (under the pending + # cap of 8 however the host schedules the supervisor), are all refused by name, and + # the last answer arrives well inside the 20 x 50 ms = 1 s the old wait alone cost. + # RED before the fix: the stalled loop let the connections pile up past the pending + # cap (Busy "too many connections are awaiting authentication"), or elapsed >= 1.0 s. + with held_session(port, schema, flatbuffers, + hello(schema, flatbuffers, fingerprint=fingerprint)) as (control, welcome): + assert welcome.get('welcome', 0), welcome + ready = wait_for_log(serverLog, f'pid={welcome["welcome"]} transport=spawn role=server ready') + assert ready is not None, f'the held session never came up; see {serverLog}' + stalled = [] + began = time.monotonic() + for _ in range(5): + burst = [open_data(port, schema, flatbuffers, os.urandom(16)) for _ in range(4)] + try: + for extra in burst: + extra.settimeout(10) + try: + stalled.append(receive(extra, schema)) + except (RuntimeError, OSError) as error: + stalled.append({'error': repr(error)}) + finally: + for extra in burst: + extra.close() + elapsed = time.monotonic() - began + for answer in stalled: + assert answer.get('code') == 4 and answer.get('detail') == kDetailHandoff, answer + assert elapsed < .5, f'twenty DataBinds into a live session took {elapsed:.2f} s to be answered' + evidence['data_bind_routing_does_not_stall'] = {'refused': len(stalled), 'seconds': round(elapsed, 3)} + assert process.poll() is None, 'the DataBind burst killed the supervisor' + # ---- ID-P7-44: A DataBind QUEUED TO THE CHILD BEFORE IT LET GO IS REFUSED, NOT DROPPED # # The F fix round closed the child's end of the hand-off the moment Accept returned; @@ -934,6 +972,55 @@ def main(): l.strip() for l in named.splitlines() if reason in l)} assert process.poll() is None, 'the pre-auth limits killed the supervisor' + # ---- ID-P7-44, f2-auth fix round: A CHILD THAT REFUSES ITS HELLO REFUSES WHAT WAS QUEUED -- + # + # The supervisor forwards DataBinds to a child from the moment it forks it. A child that + # then refused its Hello (here: an authenticated Hello with fingerprint 0, refused at + # ValidatePeerHandshake) used to `_exit` with them still queued on the hand-off, and the + # exit dropped their descriptors - those peers' connections just ended. The window is + # microseconds, so it is held open with the server's test-only lever + # MOBILEGL_TEST_DELAY_SESSION_HANDSHAKE_MS: the child waits 1.5 s before its handshake, three + # DataBinds are routed into its queue meanwhile, and each must be refused by name when the + # child refuses its Hello - by the CHILD, after its wait (not at once by the supervisor). + # RED before the fix: "control channel closed before a complete frame" on all three. + delay = {'MOBILEGL_TEST_DELAY_SESSION_HANDSHAKE_MS': '1500'} + with supervisor(args.server, args.out / 'handshake-delay.log', extra_env=delay) as (port, process, serverLog): + control = connect_control(port) + extras = [] + try: + control.settimeout(10) + control.sendall(hello(schema, flatbuffers)) + deadline = time.monotonic() + 5 + while not child_pids(process.pid): + assert time.monotonic() < deadline, 'the supervisor forked no child for an authenticated Hello' + time.sleep(.01) + began = time.monotonic() + extras = [open_data(port, schema, flatbuffers, os.urandom(16)) for _ in range(3)] + queued = [] + for extra in extras: + extra.settimeout(10) + try: + answer = receive(extra, schema) + except (RuntimeError, OSError) as error: + answer = {'error': repr(error)} + answer['seconds'] = round(time.monotonic() - began, 3) + queued.append(answer) + refused_hello = receive(control, schema) + finally: + for extra in extras: + extra.close() + control.close() + assert refused_hello.get('code') == 2, refused_hello + for answer in queued: + assert answer.get('code') == 4 and answer.get('detail') == kDetailHandoff, ( + f'a DataBind queued to a session child that then refused its Hello was answered {answer}, ' + f'not refused by name (ID-P7-44); all: {queued}') + assert answer['seconds'] >= 1.0, ( + f'the DataBind was answered after {answer["seconds"]} s - by the supervisor, not by the ' + f'child it was queued to; all: {queued}') + evidence['data_bind_queued_before_refused_hello'] = {'hello': refused_hello, 'refused': queued} + assert process.poll() is None, 'the refused Hello killed the supervisor' + # ---- F fix round: THE SINGLE-SESSION SERVER CLOSES ITS LISTENER ONCE THE SESSION IS BOUND # # Without --serve the process is the session; it keeps the listener open only so the