diff --git a/MobileGL/MG_Test/Wire/RingTest.cpp b/MobileGL/MG_Test/Wire/RingTest.cpp index 49b7b85d..56d8e0c7 100644 --- a/MobileGL/MG_Test/Wire/RingTest.cpp +++ b/MobileGL/MG_Test/Wire/RingTest.cpp @@ -12,6 +12,10 @@ #include #include +// For MGPipeCallFlags. This file is the only place in the tree that sees BOTH flag +// spaces: MG_Pipe is below MG_Remote and may not include Ring.h, so the cross-enum table +// below cannot live in a generated .inc beside the call flags themselves. +#include #include @@ -669,3 +673,119 @@ TEST(RingTest, AWrapFillerDoesNotAdvanceTheRecordSequence) { << "the two sides' sequence spaces have drifted by the fillers between them"; EXPECT_TRUE(ring.Invariants()); } + +// --------------------------------------------------------------------------- +// The two flag spaces (P5, after w1's finding). MGPWireRecHeader::Flags IS +// RingRecordHeader::flags - the two structs are the same eight bytes - and the +// two enums that name those bits OVERLAP AND DISAGREE. Stamping +// MGPipeCallFlagsFor(op) into the header is therefore a silent, data-dependent +// corruption, and the first two bits agreeing is what makes it look right in a +// debugger. +// +// The table below is exhaustive over both enums, and deliberately spells the +// agreements as well as the collisions: "these two mean the same thing" is a +// fact the encoder's translation relies on, so it has to be checked too. +// --------------------------------------------------------------------------- + +namespace { + constexpr std::uint32_t Call(MobileGL::MG_Pipe::MGPipeCallFlags f) { + return static_cast(f); + } + constexpr std::uint32_t Rec(RingRecordFlags f) { return static_cast(f); } +} // namespace + +TEST(RingTest, TheTwoFlagSpacesAreDisjointByTranslation) { + using namespace MobileGL::MG_Pipe; + + // The two bits that agree. An encoder may pass these through, and w1's does. + static_assert(Call(kNeedsAck) == Rec(kRecNeedsAck), "bit 0 stopped agreeing"); + static_assert(Call(kHasBlob) == Rec(kRecHasBlob), "bit 1 stopped agreeing"); + + // The three that collide. Each of these is a live defect if it is ever passed through, + // and the middle one is the worst: RingConsumer::Pop treats kRecPad as a wrap filler and + // SKIPS the record, so a stamped kVarTail deletes every variable-tail call from the + // stream with no error raised anywhere. + static_assert(Call(kVarTail) == Rec(kRecPad), "the kVarTail/kRecPad collision moved"); + static_assert(Call(kHostSpan) == Rec(kRecBorrowSlot), "the kHostSpan/kRecBorrowSlot collision moved"); + static_assert(Call(kReplySlot) == Rec(kRecVarTail), "the kReplySlot/kRecVarTail collision moved"); + + // kOptional has no ring counterpart at all: bit 5 is unused over there today. If a sixth + // ring flag is ever added it lands on this bit, so this is where that is noticed. + static_assert(Call(kOptional) == (1u << 5), "kOptional moved"); + + // Exhaustiveness, from both ends. The generator pins kMGPipeCallFlagsAllBits from + // MGPipe.h; this pins the ring's own set against it, so ADDING a flag to either enum is a + // build break here rather than a wrong decode in the field. + static_assert(kMGPipeCallFlagsAllBits == 0x3Fu, "MGPipeCallFlags grew or shrank"); + constexpr std::uint32_t kAllRingFlags = + Rec(kRecNeedsAck) | Rec(kRecHasBlob) | Rec(kRecPad) | Rec(kRecBorrowSlot) | Rec(kRecVarTail); + static_assert(kAllRingFlags == 0x1Fu, "RingRecordFlags grew or shrank"); + + // ---- the runtime half, and it is NOT the story the collision table alone suggests ---- + // + // RingProducer::Reserve MASKS kRecPad OUT of whatever the caller passes + // (Ring.cpp: `flags & ~kRecPad`). So the worst of the three collisions - a real record + // framed as a wrap filler and skipped by Pop - is ALREADY DEFENDED for anyone who goes + // through Reserve. That defence is worth knowing about and worth pinning, because it is + // also exactly one bit wide: the other two collisions pass straight through. + RingFixture ring(1024); + const std::uint32_t drawVboCallFlags = MGPipeCallFlagsFor(MGPWireOp::DrawVbo); + ASSERT_NE(drawVboCallFlags & Call(kVarTail), 0u) << "draw_vbo stopped being a var-tail call"; + ASSERT_NE(drawVboCallFlags & Call(kHostSpan), 0u) << "draw_vbo stopped being a host-span call"; + + void* payload = ring.Producer().Reserve( + static_cast(MGPWireOp::DrawVbo), + static_cast(drawVboCallFlags), // the mistake, made on purpose + 16); + ASSERT_NE(payload, nullptr); + std::memset(payload, 0xAB, 16); + ring.Producer().Publish(); + + RingRecordView view{}; + ASSERT_TRUE(ring.Consumer().Pop(view)) << "Reserve stopped masking kRecPad"; + EXPECT_EQ(view.flags & static_cast(kRecPad), 0u) + << "Reserve is what keeps a stamped kVarTail from deleting this record"; + // ... and here is what IS wrong with it. kHostSpan landed on kRecBorrowSlot and kReplySlot + // on kRecVarTail, neither of which Reserve masks. The consumer now believes this record + // borrowed a slot into the GPU timeline - so it retires late, on completedFrameSerial + // instead of on apply - and that it carries a variable tail it does not have. + EXPECT_NE(view.flags & static_cast(kRecBorrowSlot), 0u) + << "the kHostSpan/kRecBorrowSlot collision is what makes a stamped header lie about " + "this record's lifetime"; + EXPECT_TRUE(ring.Invariants()); + + // The one that Reserve cannot defend: a producer that writes the header ITSELF rather than + // letting Reserve write it - which is precisely what a codec with its own header struct + // does, since MGPWireRecHeader and RingRecordHeader are the same eight bytes. Then the + // mask is not in the path, Pop sees kRecPad, and the record is skipped as a wrap filler + // with no error raised anywhere. + void* second = ring.Producer().Reserve(static_cast(MGPWireOp::DrawVbo), + kRecNone, 16); + ASSERT_NE(second, nullptr); + std::memset(second, 0xAB, 16); + RingRecordHeader stamped{}; + std::memcpy(&stamped, static_cast(second) - sizeof(RingRecordHeader), + sizeof(stamped)); + stamped.flags = static_cast(drawVboCallFlags); // no mask in this path + std::memcpy(static_cast(second) - sizeof(RingRecordHeader), &stamped, + sizeof(stamped)); + ring.Producer().Publish(); + + EXPECT_FALSE(ring.Consumer().Pop(view)) + << "a header stamped with the CALL flags outside Reserve should vanish into Pop's " + "wrap-filler skip - if it did not, the collision has moved and this case is no " + "longer the control it was"; + + // And the same record framed the way the encoder actually frames it - translated, with the + // ring's own var-tail bit - round-trips intact. + void* honest = ring.Producer().Reserve(static_cast(MGPWireOp::DrawVbo), + kRecVarTail, 16); + ASSERT_NE(honest, nullptr); + std::memset(honest, 0xCD, 16); + ring.Producer().Publish(); + ASSERT_TRUE(ring.Consumer().Pop(view)); + EXPECT_EQ(view.kind, static_cast(MGPWireOp::DrawVbo)); + EXPECT_EQ(view.flags, static_cast(kRecVarTail)); + EXPECT_EQ(view.payloadSize, 16u); + EXPECT_TRUE(ring.Invariants()); +} diff --git a/scripts/gen_pipe.py b/scripts/gen_pipe.py index 7f2b3f11..4f61c184 100644 --- a/scripts/gen_pipe.py +++ b/scripts/gen_pipe.py @@ -487,13 +487,34 @@ def check_call_payloads_have_field_lists(calls, payloads): "would be blind to them: %s" % ", ".join(missing)) -# The MGPipeCallFlags enumerators, MGPipe.h:39-54. Kept here rather than parsed out of the -# header because this list is what the generated kMGPipeCallFlags[] table spells into C++: -# a flag token in PipeCalls.def that is not one of these would generate an expression that -# does not compile, and a build break several minutes later is a worse diagnosis than this -# one line. kNone is listed but is NOT a flag - it is the empty set, and it may not be -# combined with anything. -KNOWN_CALL_FLAGS = ("kNeedsAck", "kHasBlob", "kVarTail", "kHostSpan", "kReplySlot", "kOptional") +FLAG_ENUM_RE = re.compile(r"enum\s+MGPipeCallFlags\s*:\s*Uint32\s*\{(.*?)\}\s*;", re.S) +FLAG_MEMBER_RE = re.compile(r"^\s*(k\w+)\s*=\s*1u\s*<<\s*(\d+)\s*,", re.M) + + +def parse_call_flags(): + """The MGPipeCallFlags enumerators and their bit positions, READ OUT OF MGPipe.h. + + Parsed rather than hard-coded, and that is the point: this list is what the generated + kMGPipeCallFlags[] table spells into C++, and it is also what the emitted assertions pin + against MG_Remote's RingRecordFlags. A seventh flag added to the header has to reach both, + so it must reach this function - a copy here would go stale in exactly the case that + matters. kNone is deliberately absent: it is the empty set, not a flag.""" + text = read(os.path.join(PIPE_DIR, "MGPipe.h")) + body = FLAG_ENUM_RE.search(text) + if not body: + sys.exit("MGPipe.h: enum MGPipeCallFlags : Uint32 is missing or has changed shape") + flags = [(name, int(bit)) for name, bit in FLAG_MEMBER_RE.findall(body.group(1))] + if not flags: + sys.exit("MGPipe.h: enum MGPipeCallFlags declares no 1u << N members") + seen = {} + for name, bit in flags: + if bit in seen: + sys.exit("MGPipe.h: %s and %s are both bit %d" % (seen[bit], name, bit)) + seen[bit] = name + return flags + + +KNOWN_CALL_FLAGS = tuple(name for name, _ in parse_call_flags()) def check_call_flags_are_known(calls): @@ -651,7 +672,33 @@ def gen_wire(calls, residual_fields=None): struct MGPWireRecHeader { Uint16 Op; // MGPWireOp - Uint16 Flags; // MGPipeCallFlags of the call, for asserts and tracing + // RING FRAMING FLAGS - MG_Remote::Transport::RingRecordFlags - NOT MGPipeCallFlags. + // + // This field IS RingRecordHeader::flags (Ring.h): the two structs are the same eight bytes + // and the ring writes this one. The two flag spaces OVERLAP AND DISAGREE, so stamping + // MGPipeCallFlagsFor(op) in here is a silent, data-dependent corruption: + // + // bit 0 kNeedsAck == kRecNeedsAck agree + // bit 1 kHasBlob == kRecHasBlob agree + // bit 2 kVarTail vs kRecPad DISAGREE + // bit 3 kHostSpan vs kRecBorrowSlot DISAGREE + // bit 4 kReplySlot vs kRecVarTail DISAGREE + // bit 5 kOptional vs (nothing) no ring counterpart + // + // The first two agreeing is exactly what makes this dangerous: a naive stamp looks right + // in a debugger. It is not. RingProducer::Reserve masks kRecPad out of a caller's flags, + // so bit 2 is defended for anyone who goes through it - and ONLY bit 2, and only on that + // path. A producer that writes this header itself (these are the same eight bytes, so a + // codec with its own header struct does exactly that) loses even that: Pop treats kRecPad + // as a WRAP FILLER AND SKIPS THE RECORD. Bits 3 and 4 are undefended everywhere - a + // stamped kHostSpan tells the consumer this record borrowed a slot into the GPU timeline + // and must retire late, and a stamped kReplySlot claims a tail that is not there. + // + // THE TWO SPACES ARE DISJOINT BY TRANSLATION, NOT SHARED. The encoder maps one to the + // other explicitly (MG_Remote/Wire/PipeWireCodec.cpp) and nothing else may write this + // field. The CALL's flags are not on the wire at all and do not need to be: the opcode is, + // and MGPipeCallFlagsFor(op) recovers them exactly on either side. + Uint16 Flags; Uint32 Size; // bytes of this record including the header and the variable tail }; static_assert(sizeof(MGPWireRecHeader) == 8, "the wire header is 8 bytes"); @@ -718,6 +765,32 @@ static_assert((MGPipeCallFlagsFor(MGPWireOp::DrawVbo) & static_cast(kHostSpan | kVarTail)) == static_cast(kHostSpan | kVarTail), "draw_vbo is the conditional-tail plus host-span shape the codec is measured on"); """) + # The bit layout, pinned from the header the generator just read. This exists because + # MGPWireRecHeader::Flags is RingRecordHeader::flags and three of these bits mean something + # ELSE over there (kVarTail/kRecPad, kHostSpan/kRecBorrowSlot, kReplySlot/kRecVarTail). The + # cross-enum table cannot live here - MG_Pipe is below MG_Remote and may not include + # Ring.h - so it lives in MG_Test/Wire/RingTest.cpp, which sees both. What CAN be stated + # here is the half this side owns: these are the flags, at these bits, and this is all of + # them. Renumber one and the translation on the other side is wrong; both ends go red. + flag_list = parse_call_flags() + out.append("") + out.append("// The bit layout of MGPipeCallFlags, read out of MGPipe.h by the generator.") + out.append("// MG_Test/Wire/RingTest.cpp holds the other half: what each of these bits means") + out.append("// in MG_Remote::Transport::RingRecordFlags, which is NOT the same thing.") + for name, bit in flag_list: + out.append("static_assert(static_cast(%s) == (1u << %d)," % (name, bit)) + out.append(" \"MGPipeCallFlags::%s moved bit; MG_Remote's translation is keyed on it\");" + % name) + union = " | ".join(name for name, _ in flag_list) + total = 0 + for _, bit in flag_list: + total |= 1 << bit + out.append("// ... and this is ALL of them. A seventh flag changes this number, which is the") + out.append("// build break that sends its author to the ring's flag space before it ships.") + out.append("inline constexpr Uint32 kMGPipeCallFlagsAllBits = static_cast(%s);" % union) + out.append("static_assert(kMGPipeCallFlagsAllBits == 0x%02Xu," % total) + out.append(" \"the MGPipeCallFlags bit set changed; see RingTest's cross-enum table\");") + out.append("") for call in calls: out.append("struct alignas(8) MGPWireRec_%s {" % call.Name) out.append(" MGPWireRecHeader Header;")