swung0x48/MobileGL
1
0
Fork 0
mirror of https://github.com/MobileGL-Dev/MobileGL synced 2026-09-09 04:38:30 +09:00
Code Issues Packages Projects Releases Wiki Activity

[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

Browse Source 
- 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.
This commit is contained in:
swung0x48
2026-09-05 20:16:50 -04:00
parent bdd4bed431
commit c1a7ffac94
9 changed files with 147 additions and 32 deletions
Download Patch File Download Diff File Expand all files Collapse all files
CMakeLists.txt
+3
View File
@@ -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()
MobileGL/MG_Remote/Transport/Framing.h
+11 -9
View File
@@ -31,7 +31,9 @@
#include "../Protocol/mg_protocol_base.h"
#include <MG_Util/Debug/Log.h>
// NOT <MG_Util/Debug/Log.h>: that header pulls the GL frontend's umbrella into
// every translation unit that reassembles a frame. See WireLog.h.
#include "WireLog.h"
#include <cstddef>
#include <cstdint>
@@ -54,8 +56,8 @@ namespace MobileGL::MG_Remote::Transport {
inline MobileGLResult AppendFrame(std::vector<std::uint8_t>& 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<unsigned long long>(size),
static_cast<unsigned long long>(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<unsigned long long>(kMaxFramePayloadSize));
WireLogError("MG_Remote framing: frame length %u exceeds the %llu byte cap; "
"refusing to allocate on a peer-supplied length",
length, static_cast<unsigned long long>(kMaxFramePayloadSize));
return MOBILEGL_ERR_PROTOCOL_MISMATCH;
}
m_pendingSize = length;
MobileGL/MG_Remote/Transport/ITransport.h
+10 -4
View File
@@ -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;
MobileGL/MG_Remote/Transport/InProcessTransport.cpp
+17 -4
View File
@@ -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<std::vector<std::uint8_t>> messages;
std::deque<FdOffer> 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); }
MobileGL/MG_Remote/Transport/InProcessTransport.h
+12 -6
View File
@@ -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
MobileGL/MG_Remote/Transport/Ring.cpp
+12 -8
View File
@@ -87,10 +87,12 @@ namespace MobileGL::MG_Remote::Transport {
: m_control(control), m_base(static_cast<std::uint8_t*>(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<unsigned long long>(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<unsigned long long>(capacityBytes), sizeof(RingRecordHeader),
static_cast<unsigned long long>(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<const std::uint8_t*>(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<unsigned long long>(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<unsigned long long>(capacityBytes), sizeof(RingRecordHeader),
static_cast<unsigned long long>(kMaxRingCapacity));
m_control = nullptr;
m_base = nullptr;
m_capacity = 0;
MobileGL/MG_Remote/Transport/Ring.h
+11 -1
View File
@@ -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);
MobileGL/MG_Remote/Transport/WireLog.cpp
+33
View File
@@ -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 <MG_Util/Debug/Log.h>
#include <cstdarg>
#include <cstdio>
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
MobileGL/MG_Remote/Transport/WireLog.h
+38
View File
@@ -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 <Includes.h>, 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