From 1052b13a1a3055721fc6afa96cd5ee5d00e7ec75 Mon Sep 17 00:00:00 2001 From: Swung0x48 Date: Wed, 23 Sep 2026 16:36:45 -0400 Subject: [PATCH] [MG_State, MG_Test] (P7): PH-5 bounds a vector count by its element's minimum encoded size, so compact archives the codec writes decode again --- .../ProgramState/ProgramArtifactsCodec.cpp | 70 ++++++++-- .../Program/ProgramArtifactsCodecTest.cpp | 128 ++++++++++++++++-- MobileGL/MG_Test/Wire/PeerLatchTest.cpp | 8 +- 3 files changed, 185 insertions(+), 21 deletions(-) diff --git a/MobileGL/MG_State/GLState/ProgramState/ProgramArtifactsCodec.cpp b/MobileGL/MG_State/GLState/ProgramState/ProgramArtifactsCodec.cpp index f1c16911..f1bef80d 100644 --- a/MobileGL/MG_State/GLState/ProgramState/ProgramArtifactsCodec.cpp +++ b/MobileGL/MG_State/GLState/ProgramState/ProgramArtifactsCodec.cpp @@ -247,16 +247,68 @@ namespace MobileGL::MG_State::GLState { return true; } - // A COUNT IS CHECKED AGAINST THE BYTES THAT REMAIN BEFORE ANYTHING IS ALLOCATED. - // For resizable vectors, also charge sizeof(value_type) per element: even when a - // serialized record is compact, its immediate vector reservation must not exceed the - // remaining untrusted archive bytes. The division form avoids multiplying attacker - // input and is safe on both 32-bit and 64-bit SizeT. - Bool TakeCount(ReadCursor& in, SizeT& count, SizeT allocationBytesPerElement = 1) { + // ---- the fewest bytes the writer can emit for one value of a type ---- + // + // What PH-5's vector bound charges per element (TakeCount below). It is the WRITER'S + // floor, derived arm for arm from WriteValue above: a scalar is its own width, a string + // or a container is its u64 count and nothing else when empty, a fixed array is its + // width times its element's floor, and a record - VisitFields, or the hand-written + // TUniformInitializer table - is the sum of its fields' floors. It depends on the TYPE + // only; the default-constructed instance is there because VisitFields needs something to + // walk, and no member value is read. + template + SizeT MinEncodedBytesOf() { + using T = std::remove_cv_t; + if constexpr (ArchiveScalar) { + return sizeof(T); + } else if constexpr (ArchiveString || ArchiveMap || ArchiveSet || ArchiveVector) { + return sizeof(Uint64); + } else if constexpr (ArchiveFixedArray) { + return std::tuple_size::value * MinEncodedBytesOf(); + } else { + T value{}; + SizeT bytes = 0; + const auto field = [&bytes](const char*, const auto& member) { + bytes += MinEncodedBytesOf>(); + }; + if constexpr (ArchiveUniformInitializer) VisitUniformInitializer(value, field); + else VisitFields(value, field); + return bytes; + } + } + + // At least one byte, so a count is always bounded by the bytes that remain (a type whose + // floor were zero - an empty record, a zero-width array - would otherwise admit any count). + template + SizeT MinEncodedBytes() { + static const SizeT bytes = [] { + const SizeT least = MinEncodedBytesOf(); + return least == 0 ? SizeT{1} : least; + }(); + return bytes; + } + + // A COUNT IS CHECKED AGAINST THE BYTES THAT REMAIN BEFORE ANYTHING IS ALLOCATED: every + // one of `count` elements still has to be read out of Remaining(), so a count larger than + // Remaining() / (the element's smallest encoding) is one no stream this writer produced + // can back, and it is refused before the resize. + // + // PH-5 (codex closeout finding 3): the charge per element is the MINIMUM ENCODED size, + // NOT sizeof(value_type). The first PH-5 charged sizeof, and that refused archives this + // very encoder writes: a Vector element is eight bytes of count plus its + // characters on the wire but a 32-byte object in memory, so ten one-character + // xfbInterfaceNames with empty vectors behind them decoded to false + // (ProgramArtifactsCodecTest's ACompactArchiveOf* round trips). The allocation stays + // bounded all the same - resize(count) costs at most + // (Remaining() / MinEncodedBytes()) * sizeof(T), a + // constant factor of the archive fixed per type (on libstdc++: 4 for a String, 3 for a + // nested vector, ~1.3 for a ResourceReflection), never the peer's count. The division + // form avoids multiplying attacker input and is safe on both 32-bit and 64-bit SizeT. + Bool TakeCount(ReadCursor& in, SizeT& count, SizeT minEncodedBytesPerElement = 1) { Uint64 raw = 0; if (!TakeRaw(in, raw)) return false; - if (allocationBytesPerElement == 0 || - raw > static_cast(in.Remaining() / allocationBytesPerElement)) { + if (minEncodedBytesPerElement == 0 || + raw > static_cast(in.Remaining() / minEncodedBytesPerElement)) { in.Ok = false; return false; } @@ -306,7 +358,7 @@ namespace MobileGL::MG_State::GLState { } else if constexpr (ArchiveVector) { SizeT count = 0; using Element = typename T::value_type; - if (!TakeCount(in, count, sizeof(Element))) return; + if (!TakeCount(in, count, MinEncodedBytes())) return; value.clear(); value.resize(count); for (auto& element : value) { diff --git a/MobileGL/MG_Test/Program/ProgramArtifactsCodecTest.cpp b/MobileGL/MG_Test/Program/ProgramArtifactsCodecTest.cpp index c777650d..5c225cbf 100644 --- a/MobileGL/MG_Test/Program/ProgramArtifactsCodecTest.cpp +++ b/MobileGL/MG_Test/Program/ProgramArtifactsCodecTest.cpp @@ -193,6 +193,36 @@ namespace { return link; } + // How many archive bytes follow a u64 `count` that is itself followed by `next` - the LAST such + // spot, so a fixture can find the count of a field near the archive's end. This is + // Remaining() at the moment TakeCount reads that count. SIZE_MAX when there is no such spot. + SizeT BytesAfterTheCount(const Vector& bytes, Uint64 count, const Vector& next) { + Vector needle(sizeof(count)); + std::memcpy(needle.data(), &count, sizeof(count)); + needle.insert(needle.end(), next.begin(), next.end()); + if (bytes.size() < needle.size()) return static_cast(-1); + for (SizeT at = bytes.size() - needle.size() + 1; at-- > 0;) { + if (std::memcmp(bytes.data() + at, needle.data(), needle.size()) == 0) { + return bytes.size() - at - sizeof(count); + } + } + return static_cast(-1); + } + + // The encoded width of one DEFAULT element of LinkArtifacts::uniformReflection, measured + // through the encoder itself: an archive with one such element minus an archive with none. + // A default ResourceReflection has an empty name, so this is the smallest encoding one can + // have - the charge PH-5's TakeCount makes per element. + SizeT EncodedBytesOfOneDefaultResourceReflection() { + Vector none; + EncodeProgramArtifacts(LinkArtifacts{}, SpirvArtifacts{}, none); + LinkArtifacts one{}; + one.uniformReflection.resize(1); + Vector withOne; + EncodeProgramArtifacts(one, SpirvArtifacts{}, withOne); + return withOne.size() - none.size(); + } + SpirvArtifacts MakeSpirvArtifacts() { SpirvArtifacts spirv{}; spirv.generatedSpirv = {{0x07230203u, 0x00010300u, 0u}, {0x07230203u, 0x00010300u, 1u}}; @@ -299,6 +329,75 @@ TEST(ProgramArtifactsCodec, RoundTripsAFullyPopulatedArchive) { #endif } +// A COMPACT archive - one whose wire form is SMALLER than its in-memory form - is still an archive +// this encoder wrote, and it has to come back (codex closeout finding 3). Ten one-character +// xfbInterfaceNames are 90 bytes on the wire and ten 32-byte std::strings in memory; with every +// vector behind them empty, the bytes that remain at their count are fewer than 10 x sizeof(String), +// and PH-5's first bound - which charged sizeof(value_type) per element - refused the archive. The +// same holds one level down for SpirvArtifacts::generatedSpirv: ten EMPTY modules are ten u64 +// counts on the wire and ten 24-byte vectors in memory (the next case). Red with TakeCount's vector +// charge put back to sizeof(Element): the decode answers false. Each case's ASSERT_LT is the +// fixture's own proof that it can go red that way. +TEST(ProgramArtifactsCodec, ACompactArchiveOfTenShortXfbNamesRoundTrips) { +#if MOBILEGL_PIPE_PUSH + { + LinkArtifacts link{}; + link.xfbInterfaceNames = {"a", "b", "c", "d", "e", "f", "g", "h", "i", "j"}; + Vector bytes; + EncodeProgramArtifacts(link, SpirvArtifacts{}, bytes); + const Uint64 one = 1; + Vector firstName(sizeof(one)); + std::memcpy(firstName.data(), &one, sizeof(one)); + firstName.push_back(static_cast('a')); + const SizeT remaining = BytesAfterTheCount(bytes, link.xfbInterfaceNames.size(), firstName); + ASSERT_LT(remaining, link.xfbInterfaceNames.size() * sizeof(String)) + << "fixture: a sizeof(value_type) charge would admit this count, so the case could not go red"; + + LinkArtifacts decodedLink; + SpirvArtifacts decodedSpirv; + ASSERT_TRUE(DecodeProgramArtifacts(bytes.data(), bytes.size(), decodedLink, decodedSpirv)) + << "a LinkArtifacts with ten short XFB interface names, which this encoder wrote, was refused"; + EXPECT_EQ(decodedLink.xfbInterfaceNames, link.xfbInterfaceNames); + EXPECT_TRUE(decodedLink.xfbStrides.empty()); + EXPECT_TRUE(decodedLink.gsStripTriangles.empty()); + Vector reencoded; + EncodeProgramArtifacts(decodedLink, decodedSpirv, reencoded); + EXPECT_EQ(reencoded, bytes); + } +#else + GTEST_SKIP() << "MOBILEGL_PIPE_PUSH is off: the archive codec is push-only"; +#endif +} + +// The same compact shape one level down: a Vector> element (one SPIR-V module) is +// its u64 count on the wire when empty and a 24-byte object in memory. +TEST(ProgramArtifactsCodec, ACompactArchiveOfTenEmptySpirvModulesRoundTrips) { +#if MOBILEGL_PIPE_PUSH + { + SpirvArtifacts spirv{}; + spirv.generatedSpirv.resize(10); + Vector bytes; + EncodeProgramArtifacts(LinkArtifacts{}, spirv, bytes); + const SizeT remaining = BytesAfterTheCount(bytes, spirv.generatedSpirv.size(), + Vector(10 * sizeof(Uint64), 0)); + ASSERT_LT(remaining, spirv.generatedSpirv.size() * sizeof(Vector)) + << "fixture: a sizeof(value_type) charge would admit this count, so the case could not go red"; + + LinkArtifacts decodedLink; + SpirvArtifacts decodedSpirv; + ASSERT_TRUE(DecodeProgramArtifacts(bytes.data(), bytes.size(), decodedLink, decodedSpirv)) + << "a SpirvArtifacts with ten empty modules, which this encoder wrote, was refused"; + ASSERT_EQ(decodedSpirv.generatedSpirv.size(), 10u); + for (const auto& module : decodedSpirv.generatedSpirv) EXPECT_TRUE(module.empty()); + Vector reencoded; + EncodeProgramArtifacts(decodedLink, decodedSpirv, reencoded); + EXPECT_EQ(reencoded, bytes); + } +#else + GTEST_SKIP() << "MOBILEGL_PIPE_PUSH is off: the archive codec is push-only"; +#endif +} + // Negative control 1. Every prefix length is checked against the bytes that REMAIN, so a // stream cut anywhere has to come back false with both outputs defaulted - never a partially // filled archive, and never a resize driven by a count the stream cannot back. @@ -335,8 +434,12 @@ TEST(ProgramArtifactsCodec, ATruncatedStreamIsRefusedNotGuessed) { } // A count is not permission to reserve a container larger than the bytes left in the -// untrusted archive. This mutates the first LinkArtifacts vector count to the boundary value -// that the former raw <= Remaining check accepted but sizeof(value_type) <= Remaining rejects. +// untrusted archive can back. This mutates the first LinkArtifacts vector count to the boundary +// value one element past PH-5's bound - the smallest count the remaining bytes cannot hold at +// the element's minimum encoded size - which the former raw <= Remaining check accepted. (Not +// discriminating on its own: under the byte bound the resize is small and the element reads run +// out of bytes, so the decode is false either way. ACountOnlyTheByteBoundAdmitsIsRefusedBeforeItsResize +// is the case that goes red.) TEST(ProgramArtifactsCodec, AVectorCountCannotReservePastTheRemainingArchiveBytes) { #if MOBILEGL_PIPE_PUSH Vector bytes; @@ -344,7 +447,12 @@ TEST(ProgramArtifactsCodec, AVectorCountCannotReservePastTheRemainingArchiveByte constexpr SizeT countOffset = sizeof(Uint32) + sizeof(Uint64); ASSERT_GT(bytes.size(), countOffset + sizeof(Uint64)); const SizeT available = bytes.size() - countOffset - sizeof(Uint64); - const Uint64 forgedCount = static_cast(available / sizeof(UniformReflection)) + 1; + const SizeT perElement = EncodedBytesOfOneDefaultResourceReflection(); + // What finding 3 is about, stated: an element's wire floor is below its object size, and + // above the one byte the former bound charged. + ASSERT_GT(perElement, 1u); + ASSERT_LT(perElement, sizeof(UniformReflection)); + const Uint64 forgedCount = static_cast(available / perElement) + 1; ASSERT_LT(forgedCount, static_cast(available)) << "fixture must pass the old count <= remaining check"; std::memcpy(bytes.data() + countOffset, &forgedCount, sizeof(forgedCount)); @@ -365,10 +473,12 @@ TEST(ProgramArtifactsCodec, AVectorCountCannotReservePastTheRemainingArchiveByte // bytes - DecodeProgramArtifacts answers false either way (red-once: it stays green). What PH-5 // actually changed is the ALLOCATION a count may cause before the reads, so this case forges the // count BETWEEN the two bounds - `count == Remaining()`, in an archive padded to 7 MiB - and -// decodes it in a forked child whose address space has 64 MiB of headroom. PH-5 refuses before -// the resize and the child exits 0; the old bound resizes to Remaining() elements of -// sizeof(UniformReflection) each, far past the headroom, and the child dies of std::bad_alloc. -// The same shape crosses the wire in PeerLatchTest's D11ArchiveVectorCount row. +// decodes it in a forked child whose address space has 64 MiB of headroom. PH-5 charges each +// element its minimum encoded size (a ResourceReflection's is ~100 bytes; the codex closeout +// finding 3 fix, not sizeof), refuses before the resize, and the child exits 0; the old bound +// resizes to Remaining() elements of sizeof(UniformReflection) each, far past the headroom, and +// the child dies of std::bad_alloc. The same shape crosses the wire in PeerLatchTest's +// D11ArchiveVectorCount row. #if MOBILEGL_PIPE_PUSH && !defined(_WIN32) namespace { [[noreturn]] void DecodeACountOnlyTheByteBoundAdmitsUnderACapAndExit() { @@ -405,8 +515,8 @@ TEST(ProgramArtifactsCodec, ACountOnlyTheByteBoundAdmitsIsRefusedBeforeItsResize #if MOBILEGL_PIPE_PUSH && !defined(_WIN32) EXPECT_EXIT(DecodeACountOnlyTheByteBoundAdmitsUnderACapAndExit(), ::testing::ExitedWithCode(0), ".*") << "std::bad_alloc escaping the child is the resize PH-5 exists to refuse: the vector count was " - "charged one byte per element instead of sizeof(value_type); exit 3 is a decode that did not " - "refuse; 64+ is the fixture"; + "charged one byte per element instead of its minimum encoded size; exit 3 is a decode that " + "did not refuse; 64+ is the fixture"; #else GTEST_SKIP() << "MOBILEGL_PIPE_PUSH is off (the archive codec is push-only), or no RLIMIT_AS / fork here"; #endif diff --git a/MobileGL/MG_Test/Wire/PeerLatchTest.cpp b/MobileGL/MG_Test/Wire/PeerLatchTest.cpp index 588bff1d..706fe0eb 100644 --- a/MobileGL/MG_Test/Wire/PeerLatchTest.cpp +++ b/MobileGL/MG_Test/Wire/PeerLatchTest.cpp @@ -750,8 +750,10 @@ namespace { #if MOBILEGL_PIPE_PUSH // PH-5's RAW-PEER CONTROL, MADE TO DISCRIMINATE. What PH-5 changed (ProgramArtifactsCodec.cpp // TakeCount, 1ae7474d) is the CHARGE for a vector count: it used to be one byte per element - // (`count <= Remaining()`), and PH-5 charges sizeof(value_type) per element before the - // resize. So the control is a count BETWEEN the two bounds - one the byte bound admits and + // (`count <= Remaining()`), and PH-5 charges each element its minimum encoded size before the + // resize (sizeof(value_type) until codex closeout finding 3 showed that refused valid compact + // archives; ~100 bytes for a ResourceReflection either way). So the control is a count + // BETWEEN the two bounds - one the byte bound admits and // PH-5 refuses: `count == Remaining()`, in an archive padded out to kArchiveBytes. Under PH-5 // the decode refuses before anything is allocated and the session latches by name. Under the // byte bound the same count reaches `resize(count)` - Remaining() elements of @@ -1299,7 +1301,7 @@ namespace { // decoder's CreateShaderState.Reflection latch, but what it CONTROLS is TakeCount's // element charge (see ArchiveWhoseCountOnlyTheByteBoundAdmits), so the site map does // not credit it to the decoder's site (CreateShaderStateReflectionGarbage covers that). - {"D11ArchiveVectorCount", "ProgramArtifactsCodec.cpp", "PH-5 TakeCount count x sizeof(element)", + {"D11ArchiveVectorCount", "ProgramArtifactsCodec.cpp", "PH-5 TakeCount count x min encoded element", Outcome::Latched, "Fatal{ProtocolCorruption, \"CreateShaderState.Reflection\"}", false, [](Client::ClientSession& c, PeerReport& r) { std::size_t element = 0;