From c1a7ffac94755146cf66438c53785ebcc8b2443c Mon Sep 17 00:00:00 2001 From: Swung0x48 Date: Sat, 5 Sep 2026 14:42:44 -0400 Subject: [PATCH] [Fix] (MG_Remote, Transport): give descriptor offers their own condition variable, cap the ring at what a 32-bit size field can describe, and keep the frontend umbrella out of Framing.h - InProcessChannel::Direction served two different predicates from one condition_variable signalled with notify_one, so a SendFrame wakeup could be delivered to a thread blocked in ReceiveFd, which re-tests its own predicate and goes back to sleep - leaving a queued message undelivered until some unrelated later event. ITransport narrows the contract to one dedicated reader thread, but that is a comment, not a mechanism, and the first caller that splits its reader should not have to discover this. Offers now have their own fdCv, and Close() notifies both. - RingProducer/RingConsumer accepted any power-of-two capacity while RingRecordHeader::size is 32-bit by wire contract (plan section 8.1 -> PLAN.md section 6.3: 8-byte RecHeader). At 4 GiB or more a record's size - or a wrap filler's, which is sized by the distance to the boundary - would be truncated on the way in, and the consumer would then bounds-check the truncated value against the real one. kMaxRingCapacity rejects that at construction, the same class of guard as the power-of-two and smaller-than-a-header checks beside it. Unreachable today (SEG_CMD 8 MiB, SEG_STAGE 32 MiB), which is the point of catching it now. - Framing.h included MG_Util/Debug/Log.h, which includes Includes.h, the GL frontend's umbrella header: 661 headers by `clang++ -H`. It is the one header under Transport/ that broke the rule ITransport.h states for this layer ("nothing about a byte pipe needs the GL frontend's umbrella header"), which matters when the server-side binary links this and when the include-graph purity gate of plan section 10.3 (gate A, asserted on -H output rather than on symbols) lands. Its three error paths now call WireLogError, declared in a new dependency-free WireLog.h whose .cpp owns the umbrella. Framing.h is down to 134 headers, none of them MG_State, Includes.h or Log.h. - Two documentation corrections. ITransport::Shutdown documented a one-sided "releases the endpoint" while InProcessTransport::Shutdown closes both directions - which is what closing a socket does, so the spawn transport will behave the same way; the interface now says whole-connection teardown, and keeps the promise that queued messages stay readable until drained. InProcessTransport.h cited a CMake option MOBILEGL_BUILD_DISAGGREGATED_INPROC that grep finds nowhere: plan appendix B reserves it for the role-isolation shim, this skeleton does not add it, and the delivery mode is a runtime choice (MOBILEGL_TRANSPORT), not a build one. - Evidence: both configurations reconfigured and rebuilt; nm --defined-only on the OFF build still reports 0 MG_Remote symbols and the ldd dependency set is byte-identical to the OFF link (plan section 10.3, the two surviving byte-level equalities). Negative controls: removing the capacity ceiling makes RingTest.RejectsACapacityTheRecordHeaderCannotDescribe fail on both roles; collapsing fdCv back into cv makes InProcessTransportTest.AFrameWakeupIsNotEatenByAWaiterOnDescriptors fail at 3950ms against its 2000ms bound. --- CMakeLists.txt | 3 ++ MobileGL/MG_Remote/Transport/Framing.h | 20 +++++----- MobileGL/MG_Remote/Transport/ITransport.h | 14 +++++-- .../Transport/InProcessTransport.cpp | 21 ++++++++-- .../MG_Remote/Transport/InProcessTransport.h | 18 ++++++--- MobileGL/MG_Remote/Transport/Ring.cpp | 20 ++++++---- MobileGL/MG_Remote/Transport/Ring.h | 12 +++++- MobileGL/MG_Remote/Transport/WireLog.cpp | 33 ++++++++++++++++ MobileGL/MG_Remote/Transport/WireLog.h | 38 +++++++++++++++++++ 9 files changed, 147 insertions(+), 32 deletions(-) create mode 100644 MobileGL/MG_Remote/Transport/WireLog.cpp create mode 100644 MobileGL/MG_Remote/Transport/WireLog.h diff --git a/CMakeLists.txt b/CMakeLists.txt index f0f5f610..6e2dd414 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -458,6 +458,9 @@ if (MOBILEGL_BUILD_DISAGGREGATED) MobileGL/MG_Remote/Transport/ShmSegmentWin32.cpp MobileGL/MG_Remote/Transport/FdPassing.cpp MobileGL/MG_Remote/Transport/InProcessTransport.cpp + # Keeps MG_Util/Debug/Log.h - and through it the GL frontend's + # umbrella header - out of the header-only wire code (WireLog.h). + MobileGL/MG_Remote/Transport/WireLog.cpp ) endif() diff --git a/MobileGL/MG_Remote/Transport/Framing.h b/MobileGL/MG_Remote/Transport/Framing.h index 9123f20c..f05239c4 100644 --- a/MobileGL/MG_Remote/Transport/Framing.h +++ b/MobileGL/MG_Remote/Transport/Framing.h @@ -31,7 +31,9 @@ #include "../Protocol/mg_protocol_base.h" -#include +// NOT : that header pulls the GL frontend's umbrella into +// every translation unit that reassembles a frame. See WireLog.h. +#include "WireLog.h" #include #include @@ -54,8 +56,8 @@ namespace MobileGL::MG_Remote::Transport { inline MobileGLResult AppendFrame(std::vector& out, const void* payload, std::uint64_t size) { if (size > kMaxFramePayloadSize) { - MGLOG_E("MG_Remote framing: refusing to send a %llu byte payload (cap %llu); bulk " - "bytes belong in shm", + WireLogError("MG_Remote framing: refusing to send a %llu byte payload (cap %llu); " + "bulk bytes belong in shm", static_cast(size), static_cast(kMaxFramePayloadSize)); return MOBILEGL_ERR_INVALID_ARGUMENT; @@ -163,16 +165,16 @@ namespace MobileGL::MG_Remote::Transport { std::memcpy(&length, m_buffer.data() + m_readPos + 4, sizeof(length)); if (magic != kFrameMagic) { m_failed = true; - MGLOG_E("MG_Remote framing: bad frame magic 0x%08X (expected 0x%08X); the control " - "stream is desynchronized and this transport is now dead", - magic, kFrameMagic); + WireLogError("MG_Remote framing: bad frame magic 0x%08X (expected 0x%08X); the " + "control stream is desynchronized and this transport is now dead", + magic, kFrameMagic); return MOBILEGL_ERR_PROTOCOL_MISMATCH; } if (length > kMaxFramePayloadSize) { m_failed = true; - MGLOG_E("MG_Remote framing: frame length %u exceeds the %llu byte cap; refusing to " - "allocate on a peer-supplied length", - length, static_cast(kMaxFramePayloadSize)); + WireLogError("MG_Remote framing: frame length %u exceeds the %llu byte cap; " + "refusing to allocate on a peer-supplied length", + length, static_cast(kMaxFramePayloadSize)); return MOBILEGL_ERR_PROTOCOL_MISMATCH; } m_pendingSize = length; diff --git a/MobileGL/MG_Remote/Transport/ITransport.h b/MobileGL/MG_Remote/Transport/ITransport.h index 1a68bd24..723ab538 100644 --- a/MobileGL/MG_Remote/Transport/ITransport.h +++ b/MobileGL/MG_Remote/Transport/ITransport.h @@ -109,10 +109,16 @@ namespace MobileGL::MG_Remote::Transport { // ---- lifecycle ------------------------------------------------------ - // Idempotent. Unblocks every waiter with MOBILEGL_ERR_TRANSPORT_CLOSED - // and releases the endpoint. Messages already queued for this endpoint - // stay readable until drained, so a peer that shuts down after sending - // does not lose its last message. + // Idempotent. Tears down the WHOLE connection, not just this end: + // both directions are half-closed, so after either endpoint calls it + // neither side can send any more (SendFrame returns + // MOBILEGL_ERR_TRANSPORT_CLOSED) and every waiter on either side is + // unblocked. That is what closing a socket does, and the spawn + // transport behaves the same way, so a one-sided contract here would + // be a promise only the in-process implementation could keep. + // + // Messages already queued stay readable until drained: a peer that + // shuts down right after sending does not lose its last message. virtual void Shutdown() = 0; virtual TransportRole Role() const = 0; diff --git a/MobileGL/MG_Remote/Transport/InProcessTransport.cpp b/MobileGL/MG_Remote/Transport/InProcessTransport.cpp index 289ec3d8..da5989ab 100644 --- a/MobileGL/MG_Remote/Transport/InProcessTransport.cpp +++ b/MobileGL/MG_Remote/Transport/InProcessTransport.cpp @@ -39,7 +39,16 @@ namespace MobileGL::MG_Remote::Transport { public: struct Direction { std::mutex mutex; - std::condition_variable cv; + // One variable per predicate. A single cv signalled with + // notify_one would let a SendFrame's wakeup land on a thread + // blocked in ReceiveFd, which re-tests its own predicate and goes + // straight back to sleep - leaving a queued message undelivered + // until some unrelated later event. ITransport narrows the + // contract to one dedicated reader thread, but a comment is not a + // reason to ship a primitive that breaks the moment someone + // splits the reader. + std::condition_variable cv; // messages + std::condition_variable fdCv; // fdOffers std::deque> messages; std::deque fdOffers; bool closed = false; @@ -69,6 +78,7 @@ namespace MobileGL::MG_Remote::Transport { dir.closed = true; } dir.cv.notify_all(); + dir.fdCv.notify_all(); } // Anything parked on a ring doorbell has to come back too, or a // shutdown mid-frame hangs the peer forever. @@ -207,7 +217,7 @@ namespace MobileGL::MG_Remote::Transport { } dir.fdOffers.push_back(std::move(offer)); } - dir.cv.notify_one(); + dir.fdCv.notify_one(); return MOBILEGL_OK; #endif } @@ -244,9 +254,9 @@ namespace MobileGL::MG_Remote::Transport { if (dir.fdOffers.empty() && !dir.closed && timeoutMs != 0) { const auto ready = [&dir] { return !dir.fdOffers.empty() || dir.closed; }; if (timeoutMs == kWaitForever) { - dir.cv.wait(lock, ready); + dir.fdCv.wait(lock, ready); } else { - dir.cv.wait_for(lock, std::chrono::milliseconds(timeoutMs), ready); + dir.fdCv.wait_for(lock, std::chrono::milliseconds(timeoutMs), ready); } } if (dir.fdOffers.empty()) { @@ -266,6 +276,9 @@ namespace MobileGL::MG_Remote::Transport { #endif } + // Whole-connection teardown, as ITransport::Shutdown documents: both + // directions are half-closed and both ring doorbells are rung, because a + // peer parked on a ring doorbell mid-frame would otherwise never come back. void InProcessTransport::Shutdown() { m_channel->Close(); } Doorbell& InProcessTransport::PeerDoorbell() { return m_channel->Bell(1 - m_endpoint); } diff --git a/MobileGL/MG_Remote/Transport/InProcessTransport.h b/MobileGL/MG_Remote/Transport/InProcessTransport.h index d3b1fe34..efa8c005 100644 --- a/MobileGL/MG_Remote/Transport/InProcessTransport.h +++ b/MobileGL/MG_Remote/Transport/InProcessTransport.h @@ -9,12 +9,18 @@ // The `inproc` transport: two in-memory message queues and a pair of condvar // doorbells, one connected endpoint at each end. // -// It is not a test double. `inproc` is a delivery mode of its own (CMake -// option MOBILEGL_BUILD_DISAGGREGATED_INPROC): the server side is the -// monolith's own render thread, which is the single largest CPU lever this -// project has, and it is also the CI form of the split build. What it does NOT -// exercise is serialization of the byte stream, so the framing codec is -// covered separately by FramingTest. +// It is not a test double. `inproc` is a delivery mode of its own - the server +// side is the monolith's own render thread, which is the single largest CPU +// lever this project has, and it is also the CI form of the split build. What +// it does NOT exercise is serialization of the byte stream, so the framing +// codec is covered separately by FramingTest. +// +// It is built by MOBILEGL_BUILD_DISAGGREGATED, the one option this skeleton +// adds, and selected at RUNTIME (plan appendix B: MOBILEGL_TRANSPORT = +// monolith / inproc / spawn / ...). The plan also reserves a separate +// MOBILEGL_BUILD_DISAGGREGATED_INPROC option for the role-isolation shim that +// a single-process CI build will need; that option does not exist yet, and +// nothing here depends on it. // // Messages are queued whole, so no framing bytes are involved; the size cap is // still enforced so that a payload which would be illegal on a socket is diff --git a/MobileGL/MG_Remote/Transport/Ring.cpp b/MobileGL/MG_Remote/Transport/Ring.cpp index ee092645..90fb95c6 100644 --- a/MobileGL/MG_Remote/Transport/Ring.cpp +++ b/MobileGL/MG_Remote/Transport/Ring.cpp @@ -87,10 +87,12 @@ namespace MobileGL::MG_Remote::Transport { : m_control(control), m_base(static_cast(base)), m_capacity(capacityBytes), m_mask(capacityBytes - 1), m_cursors(cursors) { if (control == nullptr || base == nullptr || !IsPowerOfTwo(capacityBytes) || - capacityBytes < sizeof(RingRecordHeader)) { - MGLOG_E("MG_Remote ring: producer rejected, capacity %llu must be a power of two of at " - "least %zu bytes over a non-null mapping", - static_cast(capacityBytes), sizeof(RingRecordHeader)); + capacityBytes < sizeof(RingRecordHeader) || capacityBytes > kMaxRingCapacity) { + MGLOG_E("MG_Remote ring: producer rejected, capacity %llu must be a power of two " + "between %zu and %llu bytes over a non-null mapping (the record header's size " + "field is 32-bit, so a bigger ring would truncate it)", + static_cast(capacityBytes), sizeof(RingRecordHeader), + static_cast(kMaxRingCapacity)); m_control = nullptr; m_base = nullptr; m_capacity = 0; @@ -182,10 +184,12 @@ namespace MobileGL::MG_Remote::Transport { : m_control(control), m_base(static_cast(base)), m_capacity(capacityBytes), m_mask(capacityBytes - 1), m_cursors(cursors) { if (control == nullptr || base == nullptr || !IsPowerOfTwo(capacityBytes) || - capacityBytes < sizeof(RingRecordHeader)) { - MGLOG_E("MG_Remote ring: consumer rejected, capacity %llu must be a power of two of at " - "least %zu bytes over a non-null mapping", - static_cast(capacityBytes), sizeof(RingRecordHeader)); + capacityBytes < sizeof(RingRecordHeader) || capacityBytes > kMaxRingCapacity) { + MGLOG_E("MG_Remote ring: consumer rejected, capacity %llu must be a power of two " + "between %zu and %llu bytes over a non-null mapping (the record header's size " + "field is 32-bit, so a bigger ring would truncate it)", + static_cast(capacityBytes), sizeof(RingRecordHeader), + static_cast(kMaxRingCapacity)); m_control = nullptr; m_base = nullptr; m_capacity = 0; diff --git a/MobileGL/MG_Remote/Transport/Ring.h b/MobileGL/MG_Remote/Transport/Ring.h index 7fc04205..115a9931 100644 --- a/MobileGL/MG_Remote/Transport/Ring.h +++ b/MobileGL/MG_Remote/Transport/Ring.h @@ -111,6 +111,15 @@ namespace MobileGL::MG_Remote::Transport { inline constexpr std::uint64_t kRingRecordAlignment = 8; + // Largest ring the 8-byte header can describe. Both a record's size and a + // wrap filler's size are bounded only by the capacity and are stored in + // RingRecordHeader::size, which is 32 bits by wire contract: a ring of + // 4 GiB or more would silently truncate them, and the consumer would then + // bounds-check the truncated value against the real one. SEG_CMD is 8 MiB + // and SEG_STAGE 32 MiB today, so this is unreachable - it is the same + // class of construction-time guard as the power-of-two check beside it. + inline constexpr std::uint64_t kMaxRingCapacity = 0xFFFFFFFFull; + // Which cursor triple a producer/consumer pair drives. enum class RingCursorSet : std::uint32_t { Cmd = 0, @@ -147,7 +156,8 @@ namespace MobileGL::MG_Remote::Transport { public: RingProducer() = default; // `base` is the ring's byte area (NOT the control page) and - // `capacityBytes` must be a power of two. + // `capacityBytes` must be a power of two of at least one record header + // and at most kMaxRingCapacity. Anything else leaves Valid() false. RingProducer(RingControl* control, void* base, std::uint64_t capacityBytes, RingCursorSet cursors); diff --git a/MobileGL/MG_Remote/Transport/WireLog.cpp b/MobileGL/MG_Remote/Transport/WireLog.cpp new file mode 100644 index 00000000..34d18a53 --- /dev/null +++ b/MobileGL/MG_Remote/Transport/WireLog.cpp @@ -0,0 +1,33 @@ +// MobileGL - MobileGL/MG_Remote/Transport/WireLog.cpp +// Copyright (c) 2025-2026 MobileGL-Dev +// Licensed under the GNU Lesser General Public License v3.0: +// https://www.gnu.org/licenses/gpl-3.0.txt +// https://www.gnu.org/licenses/lgpl-3.0.txt +// SPDX-License-Identifier: LGPL-3.0-only +// End of Source File Header + +#include "WireLog.h" + +#include + +#include +#include + +namespace MobileGL::MG_Remote::Transport { + + void WireLogError(const char* format, ...) { + // One stack line, no allocation: this runs on paths that have just + // decided the connection is unusable. + char line[512]; + va_list args; + va_start(args, format); + const int written = std::vsnprintf(line, sizeof(line), format, args); + va_end(args); + if (written < 0) { + MGLOG_E("MG_Remote wire: unformattable diagnostic (format=%s)", format); + return; + } + MGLOG_E("%s", line); + } + +} // namespace MobileGL::MG_Remote::Transport diff --git a/MobileGL/MG_Remote/Transport/WireLog.h b/MobileGL/MG_Remote/Transport/WireLog.h new file mode 100644 index 00000000..1be056d3 --- /dev/null +++ b/MobileGL/MG_Remote/Transport/WireLog.h @@ -0,0 +1,38 @@ +// MobileGL - MobileGL/MG_Remote/Transport/WireLog.h +// Copyright (c) 2025-2026 MobileGL-Dev +// Licensed under the GNU Lesser General Public License v3.0: +// https://www.gnu.org/licenses/gpl-3.0.txt +// https://www.gnu.org/licenses/lgpl-3.0.txt +// SPDX-License-Identifier: LGPL-3.0-only +// End of Source File Header + +// A one-function logging shim for the wire layer's header-only code. +// +// MG_Util/Debug/Log.h includes , the GL frontend's umbrella +// header - 661 headers, measured with `clang++ -H`. That is fine inside a +// .cpp, and Ring.cpp / Doorbell.cpp / the transports all do it. It is not fine +// in a header of this layer: ITransport.h states the rule ("nothing about a +// byte pipe needs the GL frontend's umbrella header") because these headers +// are included by both roles and by the eventual server-side binary, and +// because the disaggregated build's include-graph purity gate (plan section +// 10.3, gate A) asserts on `-H` output rather than on symbols. Framing.h was +// the one header under Transport/ that broke the rule; it now calls this +// instead, and the umbrella stays inside WireLog.cpp. +// +// ERROR only, deliberately. Everything routed here is a latched protocol +// violation, never per-frame noise; non-critical wire lines use MGLOG_D from a +// .cpp, where the INFO build compiles them out entirely. + +#pragma once + +namespace MobileGL::MG_Remote::Transport { + + // Formats one line and emits it at ERROR level (MGLOG_E). printf-style, + // with the format checked against the arguments at compile time. +#if defined(__GNUC__) || defined(__clang__) + __attribute__((format(printf, 1, 2))) +#endif + void + WireLogError(const char* format, ...); + +} // namespace MobileGL::MG_Remote::Transport