From 2bcec27ac2b296c6d97f90c2961063fc882f6965 Mon Sep 17 00:00:00 2001 From: Swung0x48 Date: Wed, 23 Sep 2026 04:23:13 -0400 Subject: [PATCH] [MG_Remote, MG_Test] (P7): bound readback scratch by server reply limit --- MobileGL/MG_Remote/Server/PipeApplier.cpp | 76 +++++++++++------------ MobileGL/MG_Remote/Server/PipeApplier.h | 3 + MobileGL/MG_Test/Wire/ServerSpawnTest.cpp | 68 ++++++++++++++++++-- 3 files changed, 100 insertions(+), 47 deletions(-) diff --git a/MobileGL/MG_Remote/Server/PipeApplier.cpp b/MobileGL/MG_Remote/Server/PipeApplier.cpp index 9469f399..242197dd 100644 --- a/MobileGL/MG_Remote/Server/PipeApplier.cpp +++ b/MobileGL/MG_Remote/Server/PipeApplier.cpp @@ -30,6 +30,7 @@ #include #include +#include namespace MobileGL::MG_Remote::Server { @@ -396,50 +397,42 @@ namespace MobileGL::MG_Remote::Server { "and a client blocked on seq %llu would never be answered", static_cast(seq)); } - const MG_Backend::GlobalBackendFunctionsTable* table = Table("read_pixels"); - if (table == nullptr || table->GL.ReadPixels == nullptr) { - // DECLINED IS A REAL ANSWER (table 0's slot-header row) and it is the RIGHT one - // here: the client is parked on this seq inside the verb barrier, so returning - // false without posting would convert "not implemented" into "never returns". - replies->PostReply(seq, Wire::ReplySink::kStatusDeclined, nullptr, 0); - return false; - } - if (info.DstSize == 0) { - replies->PostReply(seq, Wire::ReplySink::kStatusError, nullptr, 0); - return false; - } - // ID-49: THE REPLY CROSSES TIGHT AND PACK STATE NEVER CROSSES FOR A READ. The server reads - // with NEUTRAL pack state - ROW_LENGTH 0, SKIP_ROWS/PIXELS/IMAGES 0, ALIGNMENT 1 - into a - // w*h*bytesPerPixel extent that IS the reply payload, and restores the pack state - // afterwards; the CLIENT scatters those tight rows into the application's pointer per its - // own GL_PACK_* state (c1's half). Reading with the client's pack state HERE was the - // codex-1 blocker: the backend's ReadPixels honours ROW_LENGTH/SKIP_* and writes PAST a - // DstSize the client sized without the initial skip (a 4x3 RGBA8 read with ROW_LENGTH=8, - // SKIP_ROWS=1, SKIP_PIXELS=2 allocates 80 and lands its last write at 120), which is the - // two DepthReadbackHonoursThePackPixelStoreParameters SEGFAULTs the census omitted. THE - // DstSize FORMULA BOTH SIDES AGREE ON: w * h * bytesPerPixel. A non-default server-visible - // pack state can no longer change either the reply's size or its bytes. const SizeT bytesPerPixel = MG_Util::GetInputBytesPerPixel( MG_Util::ConvertGLEnumToTextureInputFormat(static_cast(info.Format)), MG_Util::ConvertGLEnumToTexturePixelDataType(static_cast(info.Type))); - // Tight size = w*h*bpp, the whole of ID-49's formula. If this build cannot size the - // (format, type) pair (bpp == 0) it trusts the client's DstSize - a neutral read still - // cannot overflow it via row length or skips, and an unsizeable pair is c1's - // Fatal{UnsizedReadback} at emission, not this side's. - const Uint64 tight = bytesPerPixel != 0 - ? static_cast(info.Box.W) * static_cast(info.Box.H) * - static_cast(bytesPerPixel) - : info.DstSize; - if (bytesPerPixel != 0 && tight != info.DstSize) { - // Both halves compute w*h*bpp under ID-49, so a disagreement is the two sides - // disagreeing about the frame. Read (and post) the tight extent this side owns rather - // than the client's number, so a wrong DstSize can never make this a short read into - // uninitialised scratch. - MGLOG_E_ONCE("MG_Remote server: read_pixels DstSize %llu != tight w*h*bpp %llu " - "(%ux%u, bpp %zu); reading the tight extent (ID-49)", - static_cast(info.DstSize), - static_cast(tight), info.Box.W, info.Box.H, - bytesPerPixel); + // The server's LinkTerms.maxReplyBytes is copied from its attached ILink capabilities. + // Reject malformed shapes and oversized replies before growing m_readbackScratch. The + // client-side guard is not a trust boundary: a peer can write this record directly. + const auto rejectReadback = [&] { + replies->PostReply(seq, Wire::ReplySink::kStatusError, nullptr, 0); + return false; + }; + if (bytesPerPixel == 0 || info.Box.W == 0 || info.Box.H == 0 || + info.Box.W > static_cast(std::numeric_limits::max()) || + info.Box.H > static_cast(std::numeric_limits::max()) || + info.DstSize == 0 || m_maxReplyBytes == 0) { + return rejectReadback(); + } + + const Uint64 width = info.Box.W; + const Uint64 height = info.Box.H; + const Uint64 bpp = static_cast(bytesPerPixel); + constexpr Uint64 kUint64Max = std::numeric_limits::max(); + if (width > kUint64Max / height) return rejectReadback(); + const Uint64 pixels = width * height; + if (pixels > kUint64Max / bpp) return rejectReadback(); + const Uint64 tight = pixels * bpp; + if (tight != info.DstSize || tight > m_maxReplyBytes || + tight > static_cast(std::numeric_limits::max())) { + return rejectReadback(); + } + + const MG_Backend::GlobalBackendFunctionsTable* table = Table("read_pixels"); + if (table == nullptr || table->GL.ReadPixels == nullptr) { + // A well-formed call can still be unsupported by this server; return the + // established decline status after the peer-controlled size has been checked. + replies->PostReply(seq, Wire::ReplySink::kStatusDeclined, nullptr, 0); + return false; } if (tight > m_readbackScratch.size()) { m_readbackScratch.resize(static_cast(tight)); @@ -1200,6 +1193,7 @@ namespace MobileGL::MG_Remote::Server { if (!link || !link->Attached() || !m_segments) SessionFail(MGFatalFamily::ProtocolCorruption, "MGPipe: Fatal{ProtocolCorruption, PipeApplier::Attach missing link}"); m_verbs.SetBackend(backend); + m_verbs.SetMaxReplyBytes(link->Capabilities().MaxReplyBytes); m_decoder = Wire::PipeWireDecoder(link, m_segments, m_replies); m_decoder.SetVerbSink(&m_verbs); MG_Pipe::MGPipeServerBlockNoteIdentity(); m_attached = true; diff --git a/MobileGL/MG_Remote/Server/PipeApplier.h b/MobileGL/MG_Remote/Server/PipeApplier.h index 09fb56c5..228400ce 100644 --- a/MobileGL/MG_Remote/Server/PipeApplier.h +++ b/MobileGL/MG_Remote/Server/PipeApplier.h @@ -136,6 +136,7 @@ namespace MobileGL::MG_Remote::Server { // The server's private backend. Null until ServerLoop::CreateBackend has run, and a // verb that arrives before then declines by name rather than dereferencing. void SetBackend(MG_Backend::BackendObject* backend); + void SetMaxReplyBytes(Uint64 bytes) { m_maxReplyBytes = bytes; } Bool OnFenceCreate(const MG_Pipe::MGPHandleOnly&) override; Bool OnFenceDestroy(const MG_Pipe::MGPHandleOnly&) override; @@ -328,6 +329,8 @@ namespace MobileGL::MG_Remote::Server { // ReadPixels writes into a caller buffer, so one staging vector per session sits // between them. Grown, never shrunk, and never handed out past the call. Vector m_readbackScratch; + // Server-declared LinkTerms.maxReplyBytes, copied from the attached server link. + Uint64 m_maxReplyBytes = 0; // P5b d1: the multi-draw arrays the glMultiDraw* slots take, rebuilt from the ranges // per record (rule C: bounded by NumDraws, owned here, never handed out past the call), // and the last-record witness above. diff --git a/MobileGL/MG_Test/Wire/ServerSpawnTest.cpp b/MobileGL/MG_Test/Wire/ServerSpawnTest.cpp index 38771e04..0ada60f0 100644 --- a/MobileGL/MG_Test/Wire/ServerSpawnTest.cpp +++ b/MobileGL/MG_Test/Wire/ServerSpawnTest.cpp @@ -95,13 +95,14 @@ namespace { } }; - // A peer can mutate the command-ring bytes AFTER the production encoder has - // accepted and written them. The sequence, framing size, and submitted - // watermark stay valid; only the opcode becomes unknown to the server. - // This is the seam the later D11/PH-2 controls will use for payload fields. + // A peer can mutate command-ring record bytes AFTER the production encoder + // accepts and writes them. Sequence, framing size, and submitted watermark stay + // valid; selected payload fields bypass the encoder's client-side checks. template bool EncodeThenMutateHeaderAndPublish(Client::ClientSession& session, MobileGL::MG_Pipe::MGPWireOp op, - const Payload& payload, Mutator&& mutate) { + const Payload& payload, Mutator&& mutate, + std::uint64_t* outSeq = nullptr) { + if (outSeq != nullptr) *outSeq = Wire::kInvalidSeq; auto* link = session.DataLink(); if (link == nullptr || !link->Attached()) return false; Wire::WireRecordLayout layout{}; @@ -125,7 +126,9 @@ namespace { // and the producer-owned submittedSeq stay monotone and teardown knows // the record's sequence. Only the record bytes bypass encoder checks. session.Producer().PublishAndNotify(seq); - return link->Flush() == MOBILEGL_OK; + if (link->Flush() != MOBILEGL_OK) return false; + if (outSeq != nullptr) *outSeq = seq; + return true; } std::string ServerImage() { @@ -287,6 +290,59 @@ TEST(ServerSpawnTest, RawPeerOutOfRangeRenderStateCsoSlotGetsNamedProtocolCorrup client.Stop(); } +TEST(ServerSpawnTest, RawPeerOversizedReadbackGetsErrorAndSessionContinues) { + const std::string endpoint = Endpoint("bad-readback-size"); + const std::string logBase = endpoint + ".log"; + ScopedEnvironment logPath("MOBILEGL_LOG_FILE_PATH", logBase); + Session session; + ScopedSessionCleanup cleanup{session}; + ASSERT_TRUE(Bring("bad-readback-size", &session)); + ASSERT_EQ(Handshake(session), MOBILEGL_OK); + + auto& client = Client::ClientSessionInstance(); + MobileGL::MG_Pipe::MGPReadbackInfo info{}; + info.Res = MobileGL::MG_Pipe::kMGPipeNullHandle; + info.Box = MobileGL::MG_Pipe::MGPBox{0, 0, 0, 1, 1, 1}; + info.Format = GL_RGBA; + info.Type = GL_UNSIGNED_BYTE; + info.DstSize = 4; + + const std::uint64_t maxReplyBytes = client.MaxReplyBytes(); + ASSERT_GT(maxReplyBytes, 0u); + std::uint64_t readSeq = Wire::kInvalidSeq; + ASSERT_TRUE(EncodeThenMutateHeaderAndPublish( + client, MobileGL::MG_Pipe::MGPWireOp::ReadPixels, info, + [maxReplyBytes](Transport::RingRecordHeader&, std::uint8_t* payloadBytes) { + auto* wireInfo = reinterpret_cast(payloadBytes); + const auto width = static_cast(maxReplyBytes / 4 + 1); + wireInfo->Box.W = width; + wireInfo->Box.H = 1; + wireInfo->DstSize = static_cast(width) * 4; + }, &readSeq)) << "the peer failed to mutate and publish the encoder-accepted readback"; + ASSERT_NE(readSeq, Wire::kInvalidSeq); + ASSERT_EQ(client.WaitForApplied(readSeq, 5000), Transport::SessionWait::Reached); + + MobileGL::Int32 status = Wire::ReplySink::kStatusDeclined; + std::uint64_t replySize = 99; + ASSERT_TRUE(client.ReadReply(readSeq, nullptr, 0, &status, &replySize)); + EXPECT_EQ(status, Wire::ReplySink::kStatusError); + EXPECT_EQ(replySize, 0u); + + // A valid follow-up record proves this refusal answered the slot without ending the + // server session or stranding the command consumer. + MobileGL::MG_Pipe::MGPMemoryBarrier barrier{}; + barrier.Bits = 0x2000u; + const auto followupSeq = client.EmitAndWait( + MobileGL::MG_Pipe::MGPWireOp::MemoryBarrier, &barrier, sizeof(barrier), + nullptr, 0, nullptr, 0, nullptr); + ASSERT_NE(followupSeq, Wire::kInvalidSeq); + EXPECT_EQ(client.WaitForApplied(followupSeq, 5000), Transport::SessionWait::Reached); + Client::ClientSessionInstance().Stop(); + int exitCode = -1; + ASSERT_EQ(Server::ReapServer(session.server, 5000, &exitCode), MOBILEGL_OK); + EXPECT_EQ(exitCode, 0); +} + TEST(ServerSpawnTest, StartsAServerProcessAndHandshakesAcrossIt) { const int before = Server::CountOwnChildren(); ASSERT_GE(before, 0) << "/proc unreadable; the process-tree gate cannot run here";