diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index c1d81e2e..ca022ed7 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -1074,57 +1074,32 @@ jobs: # Split entries SKIP and ctest reports green whatever the knob says, so an unconditional # control would be red for the whole of P5 for a reason that is not a defect. # - # So the expected state is DERIVED rather than assumed, from the same fact the lanes derive - # it from: MG_IntegrationTest/CMakeLists.txt puts MGITEST_REMOTE_CLIENT_PRESENT=1 into the - # Split entries' ENVIRONMENT exactly when MG_Remote carries no c0 signature stub, and that - # string is in the generated ctest include files this artifact ships. When it is there the - # controls MUST fire; when it is not, the step says so loudly and does not pretend. + # So the expected state is DERIVED FROM BEHAVIOUR rather than assumed. The first version read + # MGITEST_REMOTE_CLIENT_PRESENT out of the generated *_tests.cmake, which was a restatement of + # the CMake source probe review finding M-1 falsified; the arming condition is a runtime fact + # inside each test process (MG_Config::Transport, ClientSession::Active() and + # ImplementedVerbCount(), read by Harness/SplitRuntimePeek), so the only honest way to ask it + # from a shell is to look at what the entries DID. When entries passed, the controls MUST + # fire; when every one of them skipped, the step says so loudly and does not pretend. + # + # THE BODY OF THIS STEP IS scripts/ci/split_negative_controls.sh, and the move is the point + # rather than tidiness. A `run:` block executes nowhere but on a runner, so these lines were + # unreviewable and untestable: when the wave-1 cross-family review said they were broken, + # CONFIRMING it needed a hand-made copy of them (wave1-codex-verify.md 8), and a copy is not + # the thing. scripts/ci/control_smoke_test.sh now drives the very file this step runs. + # + # What that smoke test pins, and what ID-46 finding 8 found missing: each control asserts its + # OWN failure reason. A non-zero ctest exit used to be enough, so a timeout, a setup abort or + # any unrelated assertion printed "turned N selected entries red, as it must" and this step + # went green. The arming run's `|| true` had the matching defect - it counted a case that ran + # and FAILED as evidence the lane was live, so the controls could be measured against a + # baseline that was already red. - name: Negative controls - the verb barrier and the persistent-map push must be load-bearing working-directory: build-split env: MOBILEGL_ITEST_REQUIRE_GPU: "1" - run: | - # THE ARMED STATE IS DERIVED FROM BEHAVIOUR, not from a marker string in the generated - # ctest files. The first version read MGITEST_REMOTE_CLIENT_PRESENT out of - # *_tests.cmake, which was a restatement of the CMake source probe review finding M-1 - # falsified; the arming condition is now a runtime fact inside each test process, so the - # only honest way to ask it from a shell is to look at what the entries DID. - ctest -L integration-split -j 4 --no-tests=error --output-junit "${RUNNER_TEMP}/isplit.xml" || true - armed=$(python3 - "${RUNNER_TEMP}/isplit.xml" <<'PY' - import sys, xml.etree.ElementTree as ET - ran = 0 - for case in ET.parse(sys.argv[1]).getroot().iter('testcase'): - if case.find('skipped') is None and case.get('status') not in ('notrun', 'disabled'): - ran += 1 - print(ran) - PY - ) - echo "split entries that actually ran: ${armed}" - if [ "${armed}" -lt 1 ]; then - echo "::warning::every DirectGLES.Split. entry SKIPPED, so neither negative control can fire. The arming condition is a runtime fact - MG_Config::Transport, ClientSession::Active() and ImplementedVerbCount(), read by Harness/SplitRuntimePeek - and it becomes true on the commit that lands the last of c1/s1/v1. This step becomes a gate then, with no edit; it is not a green that asserted anything today." - exit 0 - fi - run_control() { - name="$1"; filter="$2"; shift 2 - matched=$(ctest -N -L integration-split -R "${filter}" | grep -cE '^ *Test *#[0-9]+:') - if [ "${matched}" -lt 1 ]; then - echo "::error::${name} selected ${matched} tests; its filter no longer matches anything" - exit 1 - fi - if env "$@" ctest --output-on-failure -L integration-split -R "${filter}" --no-tests=error; then - echo "::error::${name} left ${matched} split entries GREEN, so the knob it turns is not load-bearing and the gate it controls proves nothing." - exit 1 - fi - echo "${name} turned ${matched} selected entries red, as it must" - } - # E1: R-1's lockstep verb barrier. Without it the client keeps pulling fields from a live - # GLContext while the server runs ahead, so the server reads future values. - run_control "negative control E1 (MOBILEGL_IPC_VERB_BARRIER=0)" \ - 'DirectGLES\.Split\.(Triangle|ClearThenReadPixels)' MOBILEGL_IPC_VERB_BARRIER=0 - # E3(a): the persistent-map push. 0 is admitted by ConfigLoader on purpose and is - # documented there as this control. - run_control "negative control E3(a) (MOBILEGL_IPC_PERSISTENT_BLOCK_KB=0)" \ - 'DirectGLES\.Split\.PersistentCoherentMapScenario' MOBILEGL_IPC_PERSISTENT_BLOCK_KB=0 + CONTROL_TMPDIR: ${{ runner.temp }} + run: bash "${GITHUB_WORKSPACE}/scripts/ci/split_negative_controls.sh" - name: Upload split lane logs if: always() @@ -1959,37 +1934,25 @@ jobs: # The rerun replays into the same case directory, so the good run's images are put aside and # restored whichever way the control goes; "Upload actual image" below runs `if: always()` # and would otherwise ship the deliberately-wrong run's output under the good run's name. + # THE BODY OF THIS STEP IS scripts/ci/retrace_pull_library_control.sh, for the reason the + # split lane's control gives: a `run:` block cannot be executed off a runner, so these lines + # could not be tested until they ran in CI. scripts/ci/control_smoke_test.sh drives that file. + # + # Two holes ID-46 finding 8(b) found in this block, both CONFIRMED against the REAL ctest in a + # REAL build tree, both closed in the script: it had NO selection guard at all - unlike the + # split lane's run_control - so a case/backend regex matching nothing exited 8 through + # `--no-tests=error` and was read as "the pull library turned it red"; and only "non-zero + # ctest" was checked after the nm identity check, so a loader failure, a missing fixture or a + # timeout passed it. The red must now carry run_trace_case.cmake's own sentence. - name: Negative control - the PULL library must red this split retrace working-directory: build-retrace/tools/trace_replay - run: | - set +e - GOOD_OUTPUT="${RUNNER_TEMP}/split-verified-output" - rm -rf "${GOOD_OUTPUT}" - if [ -d "${{ matrix.case }}" ]; then cp -a "${{ matrix.case }}" "${GOOD_OUTPUT}"; fi - # The pull library, unpacked from build-linux's artifact, over the frozen path every - # case has baked in. It defines no MG_Remote symbol, so ConfigLoader has no transport - # parser and MOBILEGL_TRANSPORT=inproc is accepted and ignored - the exact shape of "the - # split lane ran monolith". - cp "${GITHUB_WORKSPACE}/pull-runtime/build-linux/libMobileGL.so" \ - "${GITHUB_WORKSPACE}/build-linux/libMobileGL.so" - if nm --defined-only "${GITHUB_WORKSPACE}/build-linux/libMobileGL.so" | grep -q -i MG_Remote; then - echo "::error::the control's own library defines MG_Remote symbols, so it is not a pull build and this control would prove nothing" - exit 1 - fi - export MOBILEGL_TRANSPORT=inproc - ctest -V --no-tests=error --timeout 10800 \ - -R "^MobileGLTraceReplay\.${{ matrix.case }}\.${{ matrix.backend }}$" - control_rc=$? - set -e - if [ -d "${GOOD_OUTPUT}" ]; then - rm -rf "${{ matrix.case }}"; mv "${GOOD_OUTPUT}" "${{ matrix.case }}" - echo "restored the verified run's output over the control's" - fi - if [ "${control_rc}" -eq 0 ]; then - echo "::error::a PULL library passed the split retrace. OpenRA scores ssim 1.000000 under a monolith library too (measured), so the picture is not and cannot be this lane's gate - run_trace_case.cmake's transport-resolution assertion is, and it has stopped working. Every green in this job is then a monolith run under a name that says split." - exit 1 - fi - echo "the pull library turned the split retrace red, as it must (ctest exit ${control_rc})" + env: + CONTROL_TMPDIR: ${{ runner.temp }} + PULL_LIBRARY: ${{ github.workspace }}/pull-runtime/build-linux/libMobileGL.so + FROZEN_LIBRARY: ${{ github.workspace }}/build-linux/libMobileGL.so + run: >- + bash "${GITHUB_WORKSPACE}/scripts/ci/retrace_pull_library_control.sh" + '${{ matrix.case }}' '${{ matrix.backend }}' # The refusal census, recorded rather than gated. run_trace_case.cmake already REDS the case # on any Fatal{, so reaching here means the count is zero - but the number and the distinct @@ -2314,6 +2277,21 @@ jobs: python3 scripts/gen_pipe_field_ownership.py --check python3 scripts/gen_pipe_field_ownership.py --self-test + # R-16 APPLIED TO THE NEGATIVE CONTROLS THEMSELVES. The split lane's E1/E3(a) controls and the + # retrace lane's pull-library control are gates, and until ID-46 finding 8 neither could be + # made red by anyone: their bodies were `run:` blocks, which execute only on a runner. Both + # bodies now live in scripts/ci/, and this step runs them against a stubbed ctest that + # reproduces the finding - a NON-EMPTY selection failing with UNRELATED_CONTROL_FAILURE, and a + # case/backend regex matching no tests - and requires each control to report FAILED. The same + # stub, failing with the diagnostics the scenarios really emit, must make them report PASSED. + # + # NO BRANCH GUARD: this asks "do the negative controls still reject a red that is not theirs", + # which is a question every branch can answer and none of which depends on the TEMPORARY + # feat/disaggregated trigger at the top of this file. It costs a couple of seconds and needs + # no build. + - name: The split and retrace negative controls still reject a red that is not theirs (R-16) + run: bash scripts/ci/control_smoke_test.sh + # A GATE as of P3a (G5). "pool 与延迟释放原样搬" (ROADMAP.md:19) is meant literally: the # buffer pool, the deferred-release drain and the three persistently mapped rings move # VERBATIM, and ARCHITECTURE.md:515 says why - their retire happens only inside Present, so @@ -2325,8 +2303,9 @@ jobs: # per preprocessor arm, and a push build compiles only FlushPendingRangesFrom while the # untouched FlushPendingRangesNow lives in the `#else`. Hashing the pull name alone would # protect text no shipping build compiles, so both are hashed - the pull ladder against - # BASELINE, the push ladder against a sha pinned in the script at 3e298c9a, because that - # one was born in P3a and has no body at the base ref to compare with. + # BASELINE, the push ladder against a sha pinned in the script at 3dadd4c1 (ID-41), because + # that one was born in P3a and is compared against the reviewed body rather than against the + # base ref. # # Scoped to the disaggregation branch and to a manual dispatch, deliberately: the question # is "did these eleven move since P3a started", and BASELINE is P3a's base ref. On dev, @@ -2336,9 +2315,10 @@ jobs: # # --self-test is the half that keeps it honest, and it is not optional: a comparison that # silently stopped comparing produces exactly the same green as eleven untouched bodies. It - # runs six canned controls - eleven bodies extracted, an untouched copy compared equal, an - # edit OUTSIDE them ignored, and each of the three perturbation targets (ClearBufferPool - # and BOTH flush ladders) reported BY NAME - and fails if any of them does not answer. + # runs eight canned controls - eleven bodies extracted, an untouched copy compared equal, an + # edit OUTSIDE them ignored, each of the three perturbation targets (ClearBufferPool and BOTH + # flush ladders) reported BY NAME, plus the pin-precedence control and a one-token edit to the + # pinned ladder compared against the pin - and fails if any of them does not answer. # Same shape as gen_pipe.py --self-test above. - name: The buffer pool, the deferred-release drain and the rings did not move (G5) if: ${{ github.ref == 'refs/heads/feat/disaggregated' || github.event_name == 'workflow_dispatch' }} @@ -2369,10 +2349,11 @@ jobs: # --self-test is the half that keeps it honest and is not optional: a comparison that silently # stopped comparing produces exactly the same green as seventeen untouched regions. It runs # three positive controls (seventeen regions extracted, an untouched copy compared equal, an - # edit OUTSIDE them invisible in all three files) and FOUR negative ones - ClearBufferPool, + # edit OUTSIDE them invisible in all three files) and NINE negative ones - ClearBufferPool, # FlushPendingRangesNow, RecomputeBackendColorSlots and StageBlocksIntoUnpackRing, each - # perturbed on its own and each required to be named BY NAME - and fails if any of them does - # not answer. + # perturbed at its HEAD and at its TAIL and each required to be named BY NAME, plus a + # one-token edit to the pinned FlushPendingRangesFrom body compared against the pin - and + # fails if any of them does not answer. - name: The unpack ring, the attachment permutation, the D24S8 core and the format caveat did not move (G5) if: ${{ github.ref == 'refs/heads/feat/disaggregated' || github.event_name == 'workflow_dispatch' }} run: bash scripts/p4a_untouched_regions.sh "${BASELINE}" HEAD diff --git a/MobileGL/MG_IntegrationTest/Scenarios/PersistentCoherentMapScenario.cpp b/MobileGL/MG_IntegrationTest/Scenarios/PersistentCoherentMapScenario.cpp index c6e08c73..7ee9be12 100644 --- a/MobileGL/MG_IntegrationTest/Scenarios/PersistentCoherentMapScenario.cpp +++ b/MobileGL/MG_IntegrationTest/Scenarios/PersistentCoherentMapScenario.cpp @@ -43,6 +43,16 @@ // address really is valid in this process, which is precisely why E3(d) is worth gating at all - // and a direct count of declines would need a counter from packages b1/v1. // +// AND MEMBERSHIP FOLLOWS THE ARM (ID-42). The client's live-persistent-map set is the set of maps +// the client still has to PUSH, so an ADOPTED store is deliberately not in it - +// PersistentMapTracker::IsLivePersistentMap's second row is IsBackendPersistentMapped(), and an +// adopted store's bytes are already in coherent GPU memory. AssertMembership therefore expects +// `live == (arm == emulated)`, from the same peek AssertOrRecordArm reads. The arm-INDEPENDENT +// claim - "the predicate reads the chain, not the adopt tier" - belongs to a unit case that can +// drive both answers on demand (SplitBufferSet. +// TheAdoptedArmIsNotAMemberAndItIsTheChainRowThatSaysSo) and not to an integration entry, which +// only ever sees whichever arm its driver and transport happen to give it. +// // `mpr` (map-persistent-roundtrips) is counted per ACQUISITION ATTEMPT, mint or decline // (ARCHITECTURE.md:492), so it is the SAME NUMBER on both arms and in both transports: that is // what makes exit gate E3(c)'s "mpr equal to the monolith arm's" checkable by one process. Both @@ -243,6 +253,11 @@ void main() { oColor = vec4(vColor, 1.0); } ASSERT_TRUE(PeekBufferIsAdoptedPersistentMap(vbo, &adopted)) << "no frontend BufferObject behind GL buffer " << vbo << " " << when; const char* arm = adopted ? "adopted" : "emulated"; + // REMEMBERED, because AssertMembership's expectation is a function of it (ID-42) + // and the two have to be talking about ONE observation of ONE buffer. A second + // peek would be a second question, and a scenario that asked it twice could + // straddle a change and then compare two different moments. + m_observedArm = adopted ? ObservedArm::Adopted : ObservedArm::Emulated; RecordProperty("persistent_map_arm", arm); if (declared.empty()) return; ASSERT_EQ(declared, std::string(arm)) @@ -259,21 +274,63 @@ void main() { oColor = vec4(vColor, 1.0); } // b1-v1.md 4.1 item 2: the client's live-persistent-map set is meant to be exactly // SyncPersistentMappedRange's early-out chain. A drift between the two stops the push // silently; asking here makes it a named failure instead. + // + // MEMBERSHIP IS A PROPERTY OF THE ARM, NOT OF THE FLAGS - INTEGRATOR DECISION ID-42, + // and the first cut of this function got it wrong in a way no lane could see. It + // asserted `live` UNCONDITIONALLY, on the grounds that "a PERSISTENT|WRITE|COHERENT map + // that is not FLUSH_EXPLICIT is a member by construction". That sentence is true only + // on the EMULATED arm. PersistentMapTracker::IsLivePersistentMap's second row is + // `if (buffer.IsBackendPersistentMapped()) return false;` - deliberately, because the + // predicate must read the CHAIN and not the tier: an adopted store's bytes are already + // in coherent GPU memory and there is nothing for the push to ship, so it is not a + // member and must not be one. On the adopted arm `live` is false BY DESIGN. + // + // It survived the first wave because the two builds that ran it could not contradict + // it: the push build compiles no MG_Remote, so MGITEST_PERSISTENT_MAP_TRACKER is + // undefined and this function returned at the line above before asserting anything; + // the split build's monolith lane, where the tracker IS compiled, adopts + // (MGPipeApplyMapPersistent's R-6 decline is ANDed with `Transport != Monolith`, + // PipeApply.cpp:2005), and seven entries went red the first time the assertion ran at + // all. + // + // So the expectation is `live == (arm == emulated)`, taken from the SAME peek + // AssertOrRecordArm read. The arm-independent half - "the predicate reads the chain, + // not the tier" - is not an integration claim and is pinned where it can be driven + // directly, by MG_Test/Buffer/SplitBufferTest.cpp's + // SplitBufferSet.TheAdoptedArmIsNotAMemberAndItIsTheChainRowThatSaysSo. void AssertMembership(unsigned int vbo, const char* when) { if (!PersistentMapTrackerAvailable()) { RecordProperty("persistent_map_membership", "unavailable"); return; } + if (m_observedArm == ObservedArm::NotLookedAt) { + // AssertOrRecordArm could not look, so there is no arm to condition on and + // "could not look" is not "it was emulated" (PersistentMapPeek.h). A lane that + // DECLARES an arm has already been skipped by AssertOrRecordArm in this case. + RecordProperty("persistent_map_membership", "arm unknown"); + return; + } + const bool emulated = (m_observedArm == ObservedArm::Emulated); bool live = false; ASSERT_TRUE(PeekBufferIsLivePersistentMap(vbo, &live)) << "the tracker is compiled in but could not answer for GL buffer " << vbo << " " << when; - EXPECT_TRUE(live) - << "a PERSISTENT|WRITE|COHERENT map that is not FLUSH_EXPLICIT is a member of " - "the client's live-persistent-map set by construction, and it is not one " - << when + RecordProperty("persistent_map_membership", live ? "member" : "not a member"); + EXPECT_EQ(live, emulated) + << "the client's live-persistent-map set is the set of maps the client still has " + "to PUSH, so membership follows the arm: on the emulated arm this " + "PERSISTENT|WRITE|COHERENT non-FLUSH_EXPLICIT map must be a member, and on " + "the adopted arm it must not be - IsLivePersistentMap's " + "IsBackendPersistentMapped() row takes it out, because an adopted store's " + "bytes are already in coherent GPU memory and there is nothing to ship. This " + "map landed in the " + << (emulated ? "emulated" : "adopted") << " arm " << when << " and the predicate " + << (live ? "made it a member" : "did not make it a member") << ". The set is supposed to BE SyncPersistentMappedRange's early-out chain; if " - "they have drifted, the push stops shipping this buffer's blocks and nothing " - "else says so."; + "they have drifted, the push either stops shipping this buffer's blocks or " + "starts shipping an adopted store's, and nothing else says so. (ID-42. The " + "arm-independent statement - that the predicate reads the chain and not the " + "adopt tier - is pinned by SplitBufferSet." + "TheAdoptedArmIsNotAMemberAndItIsTheChainRowThatSaysSo, not here.)"; } void TearDown() override { @@ -320,6 +377,13 @@ void main() { oColor = vec4(vColor, 1.0); } return RegionIsMostly(image, 8, image.Width() - 9, 8, image.Height() - 9, color, 0.0, when); } + // The arm this scenario OBSERVED, as opposed to the one its lane declared. A lane may + // declare none (the monolith counting lane does not), and the peek may be unable to + // look at all, so the third state is not "assume emulated" - it is "there is no arm to + // condition on", and AssertMembership records rather than asserts under it. + enum class ObservedArm { NotLookedAt, Adopted, Emulated }; + ObservedArm m_observedArm = ObservedArm::NotLookedAt; + unsigned int m_program = 0; unsigned int m_vao = 0; unsigned int m_vbo = 0; diff --git a/MobileGL/MG_Remote/CONTRACT-P5.md b/MobileGL/MG_Remote/CONTRACT-P5.md index 3be6b4ee..cdee8c4f 100644 --- a/MobileGL/MG_Remote/CONTRACT-P5.md +++ b/MobileGL/MG_Remote/CONTRACT-P5.md @@ -144,7 +144,7 @@ These three are the reason `kHasBlob` had to be given an exact meaning (table 0) |---|---|---|---|---| | 21 | `MapPersistent` (5) | `kReplySlot\|kOptional` | **Returns `nullptr` under split, always** (R-6/R-2.4). Its `const void* seedBytes` companion (`PipeApply.h:917`) therefore never crosses in P5 and needs no carrier. The three frontend sites already tolerate a decline (`BufferObject.cpp:238`, `:603-606`, `:657-660`). Answer travels as `Status = DECLINED` with a zero-length payload. | `map_persistent.decline` | | 22 | `ResourceReadback` (52) | `kReplySlot` | Bytes go **server → client** in `SEG_EVENT` via `OnBufferWriteback` (#3), not in the reply slot: the destination is the client's shadow and the size is the resource's, not a fixed slot's. The reply slot carries only completion. **The ordering rule is load-bearing:** the writeback is applied **before** the mutation epoch bumps, never after (`ARCHITECTURE.md:292-294`, `Managers.cpp:2120-2136`). | `resource_readback.done` | -| 23 | `ReadPixels` (58) / `GetTextureImage` (55) | `kReplySlot` | **`ReadPixels` blocks in P5** and its pixels come back in the reply slot, which is why `ReplyPool::SlotBytes()` is sized from the scenario's largest read rather than guessed. `MGPReadbackInfo` has `DstOffset`/`DstSize` but **no `Seg`** (`MGPipeTypes.h:1197-1206`): ruling — the destination is **always `SEG_REPLY`** in P5, so no `Seg` field is added; the PBO destination (fire-and-forget plus a client-side `MarkGpuWritten`) is b1's and also needs none, because a PBO destination is a resource handle rather than a segment. `GetTextureImage` is **not on P5's reduced path** and its slot stays `Fatal{UnmigratedVerb}`. | `read_pixels.pixels` | +| 23 | `ReadPixels` (58) / `GetTextureImage` (55) | `kReplySlot` | **`ReadPixels` blocks in P5** and its pixels come back in the reply slot, which is why `ReplyPool::SlotBytes()` is sized from the scenario's largest read rather than guessed. **ID-47: `SEG_REPLY` is 16 MiB, eight slots of 2 MiB, `MaxReplyBytes = 2 MiB − 16 = 2,097,136`** — the canonical sizes are 8/32/16 MiB + 256 KiB (`ProtocolSmokeTest` pins them on the wire, `SessionTest` on the mapping); the largest P5 read is E2's full-surface 640×480 RGBA8 snapshot, 1,228,800 bytes, which the previous 8 MiB pool refused. **A read whose answer would exceed `MaxReplyBytes` is refused at the CLIENT before emission** with `Fatal{ReplyTooLarge, "ReadPixels x > "}` (`ReplySlotPool::RequireReadPixelsFits`, forwarded by `ClientSession::RequireReadPixelsReplyFits`, called once by c1's `OnReadPixels` emitter) — never truncated, never a server-side abort the client cannot name; `Post`'s own refusal stays as the last line of defence. Reads larger than 2 MiB (a 2400×1080 RGBA8 device surface) are a P6 debt, not a bigger pool; §5 has no knob for this segment by design. `MGPReadbackInfo` has `DstOffset`/`DstSize` but **no `Seg`** (`MGPipeTypes.h:1197-1206`): ruling — the destination is **always `SEG_REPLY`** in P5, so no `Seg` field is added; the PBO destination (fire-and-forget plus a client-side `MarkGpuWritten`) is b1's and also needs none, because a PBO destination is a resource handle rather than a segment. `GetTextureImage` is **not on P5's reduced path** and its slot stays `Fatal{UnmigratedVerb}`. | `read_pixels.pixels` | --- diff --git a/MobileGL/MG_Remote/CapsCodec.cpp b/MobileGL/MG_Remote/CapsCodec.cpp index 0ba6150e..051bd501 100644 --- a/MobileGL/MG_Remote/CapsCodec.cpp +++ b/MobileGL/MG_Remote/CapsCodec.cpp @@ -175,21 +175,6 @@ namespace MobileGL::MG_Remote { constexpr Uint64 kFormatCells = static_cast(MG_Backend::kFormatCapabilityTargetCount) * static_cast(MG_Backend::kFormatCapabilityFormatCount); - // FNV-1a, the same mixer the tree already uses for build-stamp style fingerprints. - constexpr Uint64 kFnvOffset = 1469598103934665603ull; - constexpr Uint64 kFnvPrime = 1099511628211ull; - - Uint64 FnvBytes(Uint64 hash, const void* bytes, SizeT size) { - const auto* p = static_cast(bytes); - for (SizeT i = 0; i < size; ++i) { - hash ^= static_cast(p[i]); - hash *= kFnvPrime; - } - return hash; - } - - Uint64 FnvU64(Uint64 hash, Uint64 value) { return FnvBytes(hash, &value, sizeof(value)); } - } // namespace // --------------------------------------------------------------------------------- @@ -469,7 +454,7 @@ namespace MobileGL::MG_Remote { // The ABI assertion the handshake carries // --------------------------------------------------------------------------------- - Uint64 CapsAbiFingerprint() { + Transport::AbiFingerprintInputs CapsAbiFingerprintInputs() { // MGPCaps has only a COMPOSITIONAL size assertion (MGPipeTypes.h:145-146) because // DynamicBackendParameters still carries SizeT and GLenum members - P0.5's fixed-width // rewrite did not happen and P5 does not do it either (table 0's ABI row; the rewrite @@ -479,23 +464,28 @@ namespace MobileGL::MG_Remote { // The git stamp is in it because two builds of the same sizes can still disagree about // a FIELD ORDER, which no sizeof can see; P6's spawn is same-machine and same-binary, // so it inherits this unchanged rather than needing a looser rule. - Uint64 hash = kFnvOffset; - hash = FnvU64(hash, sizeof(MG_Backend::DynamicBackendParameters)); - hash = FnvU64(hash, sizeof(MG_Pipe::MGPCaps)); - hash = FnvU64(hash, sizeof(MG_Backend::GLFunctionsTable)); - hash = FnvU64(hash, static_cast(MG_Backend::kFormatCapabilityTargetCount)); - hash = FnvU64(hash, static_cast(MG_Backend::kFormatCapabilityFormatCount)); - hash = FnvU64(hash, kFormatCapabilitiesCodecVersion); - hash = FnvU64(hash, kRendererInfoCodecVersion); - hash = FnvU64(hash, static_cast(MG_Pipe::MGPWireOp::kOpCount)); + Transport::AbiFingerprintInputs inputs; + inputs.DynamicParamsSize = sizeof(MG_Backend::DynamicBackendParameters); + inputs.CapsSize = sizeof(MG_Pipe::MGPCaps); + inputs.FunctionTableSize = sizeof(MG_Backend::GLFunctionsTable); + inputs.FormatCapabilityTargets = static_cast(MG_Backend::kFormatCapabilityTargetCount); + inputs.FormatCapabilityFormats = static_cast(MG_Backend::kFormatCapabilityFormatCount); + inputs.FormatCapabilitiesCodecVersion = kFormatCapabilitiesCodecVersion; + inputs.RendererInfoCodecVersion = kRendererInfoCodecVersion; + inputs.OpCount = static_cast(MG_Pipe::MGPWireOp::kOpCount); // The declared protocol ABI, carried over from s1's version of this function at // integration (ID-33). The sizeofs above catch a struct that changed shape; this // catches a peer that changed the PROTOCOL while every struct stayed the same size, // which is the one break the rest of the mix is blind to. - hash = FnvU64(hash, - static_cast(MOBILEGL_ABI_VERSION(MOBILEGL_PROTOCOL_ABI_MAJOR, MOBILEGL_PROTOCOL_ABI_MINOR))); - hash = FnvBytes(hash, GIT_COMMIT_HASH_SHORT, std::strlen(GIT_COMMIT_HASH_SHORT)); - return hash; + inputs.AbiVersion = + static_cast(MOBILEGL_ABI_VERSION(MOBILEGL_PROTOCOL_ABI_MAJOR, MOBILEGL_PROTOCOL_ABI_MINOR)); + inputs.BuildStamp = GIT_COMMIT_HASH_SHORT; + return inputs; } + // ONE implementation, deliberately a one-liner: the wave-1 review (ID-46 finding 6) found a + // second hand-rolled FNV loop here while the mixer under Transport/ had no production caller, + // so the sensitivity test could not see this function change. Now it starts from here. + Uint64 CapsAbiFingerprint() { return Transport::MixAbiFingerprint(CapsAbiFingerprintInputs()); } + } // namespace MobileGL::MG_Remote diff --git a/MobileGL/MG_Remote/CapsCodec.h b/MobileGL/MG_Remote/CapsCodec.h index a26bde20..5fe14c9f 100644 --- a/MobileGL/MG_Remote/CapsCodec.h +++ b/MobileGL/MG_Remote/CapsCodec.h @@ -34,6 +34,8 @@ #include #include +#include "Transport/SessionRings.h" // AbiFingerprintInputs / MixAbiFingerprint + namespace MobileGL::MG_Remote { // ---- CallMask's layout (c0's ruling, extending R-8) --------------------------------- @@ -90,10 +92,17 @@ namespace MobileGL::MG_Remote { // ---- the ABI assertion the handshake carries ---------------------------------------- // - // Mixes sizeof(DynamicBackendParameters), sizeof(MGPCaps), sizeof(GLFunctionsTable) and - // the compile-time build fingerprint. Compared in Hello/Welcome; a mismatch is - // Fatal{AbiMismatch} and never a downgrade, because every alternative silently reads one - // struct as another. + // The inputs, as this build sees them: sizeof(DynamicBackendParameters), sizeof(MGPCaps), + // sizeof(GLFunctionsTable), the format-capability table's extents, the two caps-blob codec + // versions, MGPWireOp::kOpCount, the protocol ABI version and the compile-time git stamp. + // Public so that the sensitivity control can pin every one of them to the real value AND + // perturb them one at a time through the same mixer the handshake uses. + Transport::AbiFingerprintInputs CapsAbiFingerprintInputs(); + + // What Hello/Welcome carry and compare: EXACTLY Transport::MixAbiFingerprint( + // CapsAbiFingerprintInputs()) - one implementation, no second hash (ID-46 finding 6). A + // mismatch is Fatal{AbiMismatch} and never a downgrade, because every alternative silently + // reads one struct as another. Uint64 CapsAbiFingerprint(); } // namespace MobileGL::MG_Remote diff --git a/MobileGL/MG_Remote/Client/ClientSession.cpp b/MobileGL/MG_Remote/Client/ClientSession.cpp index 42dad770..843ff968 100644 --- a/MobileGL/MG_Remote/Client/ClientSession.cpp +++ b/MobileGL/MG_Remote/Client/ClientSession.cpp @@ -25,6 +25,7 @@ #include #include #include +#include #include namespace MobileGL::MG_Remote::Client { @@ -341,7 +342,25 @@ namespace MobileGL::MG_Remote::Client { // ---- 1. the control plane and the two bells. The transport owns the bells; THE // SESSION owns the rings, and the accessors stay off ITransport (contract §3.9). - Transport::InProcessTransport::CreatePair(m_clientTransport, m_serverTransport); + std::unique_ptr clientEnd; + std::unique_ptr serverEnd; + Transport::InProcessTransport::CreatePair(clientEnd, serverEnd); + return StartOverTransportPair(std::move(clientEnd), std::move(serverEnd)); + } + + MobileGLResult ClientSession::StartOverTransportPair( + std::unique_ptr clientEnd, + std::unique_ptr serverEnd) { + if (m_started) { + return MOBILEGL_ERR_INVALID_ARGUMENT; + } + if (clientEnd == nullptr || serverEnd == nullptr) { + MGLOG_E("MG_Remote client: StartOverTransportPair needs both ends of one " + "InProcessTransport::CreatePair"); + return MOBILEGL_ERR_INVALID_ARGUMENT; + } + m_clientTransport = std::move(clientEnd); + m_serverTransport = std::move(serverEnd); m_transport = m_clientTransport.get(); // ---- 2. Hello. Sent before the server accepts: InProcessTransport queues whole @@ -834,6 +853,15 @@ namespace MobileGL::MG_Remote::Client { Uint32 ClientSession::MaxReplyBytes() const { return m_replies.MaxReplyBytes(); } + Bool ClientSession::ReplyCanHold(Uint64 bytes) const { return m_replies.CanHold(bytes); } + + // ID-47. Forwarded verbatim so that the message, the boundary and the abort are the pool's + // and are pinned once, in SessionTest, rather than re-derived per caller. + void ClientSession::RequireReadPixelsReplyFits(Uint32 width, Uint32 height, Uint32 format, + Uint32 type, Uint64 bytes) const { + m_replies.RequireReadPixelsFits(width, height, format, type, bytes); + } + Transport::EventRingConsumer& ClientSession::Events() { return m_events; } Transport::RingControl* ClientSession::Control() { return m_shm.CmdControl(); } diff --git a/MobileGL/MG_Remote/Client/ClientSession.h b/MobileGL/MG_Remote/Client/ClientSession.h index a80542d2..5671d5b7 100644 --- a/MobileGL/MG_Remote/Client/ClientSession.h +++ b/MobileGL/MG_Remote/Client/ClientSession.h @@ -69,6 +69,19 @@ namespace MobileGL::MG_Remote::Client { // ran monolith and went green" failure, and it must be loud. MobileGLResult Start(MG_Config::TransportMode mode, const String& endpoint); + // Start()'s second half: the handshake and everything after it, over a transport pair + // the CALLER made with InProcessTransport::CreatePair. Start() refuses every mode but + // `inproc`, makes the pair, and calls this; it is public for exactly one reason. The + // Welcome guard below (envelope->msg_as_Welcome() == nullptr, ID-46 finding 7) can only + // be reached by a frame that arrives on the server->client direction BEFORE the + // server's own Welcome, and Start() builds that pair itself, so no control could put + // one there. SessionHandshakeTest does it through here, and the null-union Welcome must + // come back as MOBILEGL_ERR_PROTOCOL_MISMATCH with the guard's own line. Not a second + // way to start a session: MG_Backend::Init() calls Start(), and nothing else may call + // this with a pair it did not just create. + MobileGLResult StartOverTransportPair(std::unique_ptr clientEnd, + std::unique_ptr serverEnd); + // Teardown order matters and is table 3's fourth column: publish and let the server // drain, Doorbell::Kill() (the ONLY thing that wakes an apply thread parked on // kWaitForever, Doorbell.h:211-221), then join, and only then release anything an @@ -157,6 +170,18 @@ namespace MobileGL::MG_Remote::Client { // What one answer may carry. A ReadPixels bigger than this is Fatal rather than // chunked, so the client checks BEFORE it emits. Uint32 MaxReplyBytes() const; + // ID-47, the fifth primitive: true exactly when an answer of `bytes` can be posted. + Bool ReplyCanHold(Uint64 bytes) const; + // ID-47's named refusal, forwarded verbatim to ReplySlotPool::RequireReadPixelsFits. + // Returns when the answer fits; otherwise + // Fatal{ReplyTooLarge, "ReadPixels x > "} + // and abort - AT THE CLIENT, BEFORE EMISSION. Package c1's OnReadPixels emitter calls + // this once, immediately before EmitAndWait(MGPWireOp::ReadPixels, ...), with the + // record's box, its Format/Type enums and the DstSize it computed; the server's Post + // keeps its own refusal as the last line of defence, but that one fires on the apply + // thread with the record already on the wire, where all the client sees is a hang. + void RequireReadPixelsReplyFits(Uint32 width, Uint32 height, Uint32 format, Uint32 type, + Uint64 bytes) const; // The reverse channel's reading end: OnBufferWriteback / OnGpuWritten / // OnSurfaceChanged. Drained by the GL thread between verbs. diff --git a/MobileGL/MG_Remote/Client/EmitTables.cpp b/MobileGL/MG_Remote/Client/EmitTables.cpp index 7f97dd73..53c3d4bf 100644 --- a/MobileGL/MG_Remote/Client/EmitTables.cpp +++ b/MobileGL/MG_Remote/Client/EmitTables.cpp @@ -265,20 +265,6 @@ namespace MobileGL::MG_Remote::Client { ReadbackBytesPerPixel(format, type); } - // ID-47's refusal, in its own function so the boundary pair can drive it without a - // session. See EmitTables.h. - void RefuseOversizeReadback(GLsizei width, GLsizei height, GLenum format, Uint64 bytes, - Uint64 capacity) { - if (bytes <= capacity) return; - MGLOG_F("MGPipe: Fatal{ReplyTooLarge, \"ReadPixels %dx%d 0x%04x %llu > %llu\"} - P5 " - "does not chunk a readback (R-10) and must not truncate one; grow " - "MOBILEGL_IPC_REPLY_MB or read less", - static_cast(width), static_cast(height), - static_cast(format), static_cast(bytes), - static_cast(capacity)); - std::abort(); - } - void EmitReadPixels(GLint x, GLint y, GLsizei width, GLsizei height, GLenum format, GLenum type, void* pixels) { ClientSession& session = RequireSession("ReadPixels"); @@ -305,13 +291,20 @@ namespace MobileGL::MG_Remote::Client { info.DstOffset = 0; info.DstSize = tight; - // CHECKED BEFORE THE EMISSION, not after the answer (ID-47). A reply bigger than a - // slot is Fatal on the SERVER, and a Fatal there is a dead apply thread with a - // client parked in the barrier for ever, naming a byte count and not a read; here - // it is one line naming the read. The capacity is read LIVE from the pool rather - // than compared against a constant, so s1's growth of SEG_REPLY to 16 MiB / eight - // 2 MiB slots needs no edit in this file. - RefuseOversizeReadback(width, height, format, tight, session.MaxReplyBytes()); + // CHECKED BEFORE THE EMISSION, not after the answer (ID-47), and through S1's + // HELPER rather than a copy of it here. A reply bigger than a slot is Fatal on the + // SERVER too, and s1 keeps that as the last line of defence - but it fires on the + // apply thread with the record already on the wire, where all the client sees is a + // hang. This one names the read, at the call site that knows what the read was. + // + // THE NUMBER IS ID-49's TIGHT EXTENT and not the packed one: the pack state never + // crosses, so the answer that has to fit a slot is w*h*bytesPerPixel. The cap is + // the pool's own, read live, so s1's growth of SEG_REPLY to 16 MiB / eight 2 MiB + // slots needed no edit in this file - only the merge. + session.RequireReadPixelsReplyFits(static_cast(width), + static_cast(height), + static_cast(format), + static_cast(type), tight); PixelStoreParameters pack{}; if (MG_State::pGLContext != nullptr) { @@ -588,14 +581,6 @@ namespace MobileGL::MG_Remote::Client { void SetDropClearEmissionForNegativeControl(Bool drop) { g_dropClearEmission = drop; } Uint64 DroppedClearEmissions() { return g_droppedClearEmissions; } - // ID-47's refusal, exported so the boundary pair drives THE EMITTER'S OWN decision rather - // than a copy of it. One line, because the arithmetic that produced `bytes` is - // TightReadbackBytes' and the capacity is the pool's - this function only decides. - void RefuseReadbackLargerThanTheReplySlot(GLsizei width, GLsizei height, GLenum format, - Uint64 bytes, Uint64 capacity) { - RefuseOversizeReadback(width, height, format, bytes, capacity); - } - Bool ReadbackPackStateIsTightForTest(GLsizei width, Uint64 bytesPerPixel, const PixelStoreParameters& pack) { return ReadbackPackStateIsTight(width, bytesPerPixel, pack); diff --git a/MobileGL/MG_Remote/Client/EmitTables.h b/MobileGL/MG_Remote/Client/EmitTables.h index ace528fe..1a7719eb 100644 --- a/MobileGL/MG_Remote/Client/EmitTables.h +++ b/MobileGL/MG_Remote/Client/EmitTables.h @@ -117,18 +117,13 @@ namespace MobileGL::MG_Remote::Client { Uint64 DroppedClearEmissions(); // ID-47. The CLIENT refuses a readback whose answer would not fit a reply slot, BEFORE it - // emits the record, and names the read. Never truncated (a short write is a silently - // truncated picture, the one failure an SSIM comparison cannot see) and never left to the - // server's `Post` abort (which happens on the apply thread, after the client is already - // parked in the barrier, and names a byte count rather than a read). - // - // IT IS A FREE FUNCTION SO THE BOUNDARY PAIR CAN DRIVE IT. `EmitReadPixels` needs a live - // session before it reaches any of this, so a control over the emitter could only ever - // observe Fatal{NoClientSession}; a control over THIS observes the decision and its exact - // message, and it is the same function the emitter calls rather than a second copy of the - // arithmetic. Returns when the read fits; aborts when it does not. - void RefuseReadbackLargerThanTheReplySlot(GLsizei width, GLsizei height, GLenum format, - Uint64 bytes, Uint64 capacity); + // emits the record, and names the read. THE REFUSAL ITSELF IS s1's - + // ClientSession::RequireReadPixelsReplyFits, forwarding to + // ReplySlotPool::RequireReadPixelsFits - and this package does not own a second copy of the + // message: two spellings of one refusal is how the two sides come to disagree about which + // reads are legal. What c1 owns is the CALL SITE and the number it passes, which is ID-49's + // tight extent; the control below is over that, and s1-v3.md §1's cases are over the + // helper. // ID-49. `MGPReadbackInfo::DstSize` is the TIGHT w*h*bytesPerPixel extent - the reply // payload - and nothing about the application's pack state crosses the wire. The server diff --git a/MobileGL/MG_Remote/Transport/ReplySlot.h b/MobileGL/MG_Remote/Transport/ReplySlot.h index 5595d6a6..67574741 100644 --- a/MobileGL/MG_Remote/Transport/ReplySlot.h +++ b/MobileGL/MG_Remote/Transport/ReplySlot.h @@ -28,10 +28,13 @@ // uploads from the other side, and a client that folds it into OK accepts a // pointer the server never handed out. // -// A REPLY LARGER THAN ONE SLOT IS FATAL, NOT CHUNKED. P5's only large answer is -// ReadPixels, and the client knows its size before it emits the record, so an -// overflow means the two sides disagree about the frame rather than that the -// pool is too small. Chunking is P8's; growing the pool is an operator's. +// A REPLY LARGER THAN ONE SLOT IS FATAL, NOT CHUNKED, AND THE CLIENT SAYS SO +// FIRST (ID-47). P5's only large answer is ReadPixels, and the client knows its +// size before it emits the record, so it refuses an oversize read BY NAME before +// emission (RequireReadPixelsFits: Fatal{ReplyTooLarge, "ReadPixels x +// > "}); Post's own refusal is the server's last line of +// defence, and reaching it means the two sides disagree about the frame rather +// than that the pool is too small. Chunking is a P6 debt; the pool has no knob. // // ORDERING. The client only looks at a slot after it has seen // RingControl::appliedSeq >= its own seq with an ACQUIRE load, and the server @@ -77,15 +80,28 @@ namespace MobileGL::MG_Remote::Transport { kReplyStatusError = 2, }; - // Eight slots of a 8 MiB SEG_REPLY is 1 MiB per answer. + // Eight slots of a 16 MiB SEG_REPLY is 2 MiB per slot, and MaxReplyBytes() is + // 2 MiB minus the 16-byte header = 2,097,136 bytes per answer (ID-47). // // Why eight and not sixty-four: while the verb barrier holds (R-1) the client // blocks at every verb boundary, so the in-flight depth is exactly ONE and // every extra slot buys nothing but a smaller maximum answer. The trade is // the other way round - fewer slots, bigger replies - and P5's only large - // answer is a blocking ReadPixels. 1 MiB covers a 512x512 RGBA8 read. P9, - // which is what makes the pool asynchronous, re-chooses this geometry with - // real depth to size it against. + // answer is a blocking ReadPixels. P9, which is what makes the pool + // asynchronous, re-chooses this geometry with real depth to size it against. + // + // WHY 2 MiB AND NOT 1. The first version of this file said "1 MiB covers a + // 512x512 RGBA8 read", and it did not: 512*512*4 is exactly 1 MiB and the + // slot header takes 16 of those bytes, so Post refused it by sixteen. Worse, + // the E2 retrace harness's snapshot is a full-surface GL_RGBA/GL_UNSIGNED_BYTE + // read of OpenRA's 640x480 surface = 1,228,800 bytes through the interposer, + // 17% over the old cap - Post would have aborted on the first snapshot the + // day the client's ReadPixels emitter landed. Contract §2 row 23 says the + // slot is "sized from the scenario's largest read rather than guessed"; + // 2 MiB is that size for every P5 exit-gate read (E2 is the largest at + // 640x480). A 2400x1080 RGBA8 device surface is ~10.4 MB and is a P6 debt + // (chunked readback or a dedicated readback carrier), recorded in the + // ROADMAP by the integrator - the geometry here is not the place it is paid. inline constexpr std::uint32_t kDefaultReplySlotCount = 8; // Slot 0 exists and is used: seq is 1-based, so seq % slotCount hits slot 0 @@ -152,6 +168,44 @@ namespace MobileGL::MG_Remote::Transport { : m_slotBytes - static_cast(sizeof(ReplySlotHeader)); } + // ID-47: THE CLIENT'S HALF of "a reply larger than one slot is fatal". True + // exactly when an answer of `bytes` can be posted into this pool: the pool + // is configured and `bytes <= MaxReplyBytes()`. The boundary is inclusive + // and SessionTest pins it from both sides. + bool CanHold(std::uint64_t bytes) const { return m_base != nullptr && bytes <= MaxReplyBytes(); } + + // ID-47's named refusal, AT THE CLIENT, BEFORE EMISSION. Returns when the + // answer fits; otherwise + // Fatal{ReplyTooLarge, "ReadPixels x > "} + // and abort. Never truncated, never chunked, and never a server-side + // abort the client cannot name: Post's own refusal below stays as the + // last line of defence, but it fires on the apply thread with the record + // already on the wire, where the only thing the client sees is a hang. + // + // `format`/`type` are the GL enums the record carries (MGPReadbackInfo:: + // Format/Type), printed as the hex pair every pipe diagnostic uses; the + // caller passes the byte count it computed for the record's DstSize, so + // what is refused is exactly what would have been posted. This is the one + // call c1's OnReadPixels emitter makes before EmitAndWait; the + // ClientSession forwards it verbatim (RequireReadPixelsReplyFits). + void RequireReadPixelsFits(std::uint32_t width, std::uint32_t height, std::uint32_t format, + std::uint32_t type, std::uint64_t bytes) const { + if (CanHold(bytes)) { + return; + } + WireLogFatal("MGPipe: Fatal{ReplyTooLarge, \"ReadPixels %ux%u 0x%04X/0x%04X %llu > %u\"} - " + "the answer does not fit one SEG_REPLY slot (%u slots of %u bytes, payload " + "cap %u; a cap of 0 means no reply pool is configured). Refused at the " + "client before emission (ID-47): P5 neither truncates nor chunks a reply, " + "and a read larger than the cap is a P6 debt (chunked readback), not a " + "bigger pool", + static_cast(width), static_cast(height), + static_cast(format), static_cast(type), + static_cast(bytes), + static_cast(MaxReplyBytes()), static_cast(m_slots), + static_cast(m_slotBytes), static_cast(MaxReplyBytes())); + } + // Zeroes every header, so a stale seq from a previous session cannot be // mistaken for this session's answer. Called on the server side at Accept. void Clear() { @@ -170,24 +224,25 @@ namespace MobileGL::MG_Remote::Transport { // chunked - see the file header. void Post(std::uint64_t seq, std::int32_t status, const void* bytes, std::uint64_t size) { if (m_base == nullptr) { - WireLogError("MG_Remote reply pool: Post(seq=%llu) on an unconfigured pool", + WireLogFatal("MG_Remote reply pool: Fatal{ProtocolCorruption} - Post(seq=%llu) on an " + "unconfigured pool", static_cast(seq)); - std::abort(); } if (seq == 0) { - WireLogError("MG_Remote reply pool: seq 0 is \"no record\" and can never name a " - "slot (R-3: seq is 1-based)"); - std::abort(); + WireLogFatal("MG_Remote reply pool: Fatal{ProtocolCorruption} - seq 0 is \"no " + "record\" and can never name a slot (R-3: seq is 1-based)"); } if (size > MaxReplyBytes()) { - WireLogError("MG_Remote reply pool: Fatal{ProtocolCorruption} - a %llu byte answer " + // The server's LAST line of defence, not the first: the client refuses an + // oversize ReadPixels by name before it emits (RequireReadPixelsFits, ID-47), + // so reaching this means the two sides disagree about the frame. + WireLogFatal("MG_Remote reply pool: Fatal{ProtocolCorruption} - a %llu byte answer " "for seq %llu does not fit a %u byte slot (payload cap %u). P5 does " "not chunk replies: the client knows an answer's size before it emits " "the record, so this means the two sides disagree about the frame", static_cast(size), static_cast(seq), static_cast(m_slotBytes), static_cast(MaxReplyBytes())); - std::abort(); } std::uint8_t* slot = SlotAt(seq); if (size != 0 && bytes != nullptr) { diff --git a/MobileGL/MG_Remote/Transport/Ring.cpp b/MobileGL/MG_Remote/Transport/Ring.cpp index 60188ae8..b0617e92 100644 --- a/MobileGL/MG_Remote/Transport/Ring.cpp +++ b/MobileGL/MG_Remote/Transport/Ring.cpp @@ -9,6 +9,7 @@ #include "Ring.h" #include "SessionRings.h" +#include "WireLog.h" #include @@ -364,13 +365,15 @@ namespace MobileGL::MG_Remote::Transport { const char* name) { const std::uint64_t current = watermark.load(std::memory_order_relaxed); if (to < current) { - MGLOG_F("MGPipe: Fatal{ProtocolCorruption, \"watermark\"} %s moved backwards, " - "%llu -> %llu. A waiter that already resumed on the higher value cannot " - "be un-resumed, and every later advance of this watermark would be a " - "no-op, so the verb barrier and every reply wait would block for ever", - name, static_cast(current), - static_cast(to)); - std::abort(); + // WireLogFatal, not MGLOG_F + abort: the line has to reach stderr + // for SessionTestDeath's control to name it (WireLog.h). + WireLogFatal("MGPipe: Fatal{ProtocolCorruption, \"watermark\"} %s moved backwards, " + "%llu -> %llu. A waiter that already resumed on the higher value " + "cannot be un-resumed, and every later advance of this watermark " + "would be a no-op, so the verb barrier and every reply wait would " + "block for ever", + name, static_cast(current), + static_cast(to)); } if (to == current) { return; @@ -611,13 +614,12 @@ namespace MobileGL::MG_Remote::Transport { // The ABI fingerprint's mixer // ----------------------------------------------------------------------- - std::uint64_t MixAbiFingerprint(std::uint64_t dynamicParamsSize, std::uint64_t capsSize, - std::uint64_t functionTableSize, std::uint32_t abiVersion, - const char* buildStamp) { - // FNV-1a over the four numbers and the stamp. Not a hash with any - // security property and not meant to be one: it has to (a) change when - // ANY input changes and (b) be computable identically in two processes - // built from one source tree, which rules out anything seeded at runtime. + std::uint64_t MixAbiFingerprint(const AbiFingerprintInputs& inputs) { + // FNV-1a over every field, in declaration order, then the stamp. Not a + // hash with any security property and not meant to be one: it has to + // (a) change when ANY input changes and (b) be computable identically in + // two processes built from one source tree, which rules out anything + // seeded at runtime. std::uint64_t hash = 1469598103934665603ull; const auto mix = [&hash](std::uint64_t value) { for (int byte = 0; byte < 8; ++byte) { @@ -625,12 +627,21 @@ namespace MobileGL::MG_Remote::Transport { hash *= 1099511628211ull; } }; - mix(dynamicParamsSize); - mix(capsSize); - mix(functionTableSize); - mix(abiVersion); - if (buildStamp != nullptr) { - for (const char* c = buildStamp; *c != '\0'; ++c) { + mix(inputs.DynamicParamsSize); + mix(inputs.CapsSize); + mix(inputs.FunctionTableSize); + mix(inputs.FormatCapabilityTargets); + mix(inputs.FormatCapabilityFormats); + mix(inputs.FormatCapabilitiesCodecVersion); + mix(inputs.RendererInfoCodecVersion); + mix(inputs.OpCount); + mix(inputs.AbiVersion); + // A presence marker before the bytes, so that "no stamp" (nullptr) and + // "an empty stamp" ("") are different inputs rather than the same + // absence of bytes. + mix(inputs.BuildStamp != nullptr ? 1u : 0u); + if (inputs.BuildStamp != nullptr) { + for (const char* c = inputs.BuildStamp; *c != '\0'; ++c) { hash ^= static_cast(static_cast(*c)); hash *= 1099511628211ull; } diff --git a/MobileGL/MG_Remote/Transport/RoleMemory.h b/MobileGL/MG_Remote/Transport/RoleMemory.h index 4fc61061..2c96b5b7 100644 --- a/MobileGL/MG_Remote/Transport/RoleMemory.h +++ b/MobileGL/MG_Remote/Transport/RoleMemory.h @@ -34,6 +34,7 @@ #pragma once +#include #include namespace MobileGL::MG_Remote::Transport { @@ -55,6 +56,19 @@ namespace MobileGL::MG_Remote::Transport { // from one that was not sampled. std::uint64_t ProcessCurrentRssBytes(); + // BOTH, FROM ONE PASS OVER /proc/self/status. The kernel does not keep + // hiwater_rss up to date on growth - it stores it only when RSS is about to + // DROP, and task_mem() reports max(stored hiwater, rss-at-this-read) - so + // two separate reads are not comparable: the fopen of the second read can + // itself grow RSS past the VmHWM the first read reported. That is exactly + // what GitHub run 35079459114 caught (VmHWM 4,784,128 < VmRSS 4,849,664 on + // ubuntu-24.04) and what ~/w7/p5-s1-probe reproduced locally: two reads + // disagree by 128 KiB with nothing allocated between them, one read never + // does (0/1000 with 64 KiB of growth per iteration). One pass is therefore + // the only way to read a pair, and even then the pair is a snapshot, not a + // bound - see SampleRoleMemoryInto. + void ProcessRssBytes(std::uint64_t* outPeakBytes, std::uint64_t* outCurrentBytes); + // The ledger. ShmSegment does NOT update it itself: a segment is also created // by tests and by P6's adopt path, and a ledger that counted those would stop // meaning "this session's footprint". The SESSION books its own segments. @@ -68,6 +82,17 @@ namespace MobileGL::MG_Remote::Transport { std::uint64_t LedgerMappedBytesAllRoles(); // One sample, both halves, for one role. + // + // PeakRssBytes IS THE LEDGER'S OWN RUNNING MAXIMUM, not the kernel's VmHWM. + // It is the largest of every kernel peak and every kernel current this + // process has folded in through SampleRoleMemoryInto, so it is >= this + // sample's CurrentRssBytes BY CONSTRUCTION and never decreases. The kernel's + // VmHWM feeds it (it is a lower bound on the true peak that this process's + // own samples may have missed) but is never reported on its own: as + // ProcessRssBytes explains, the kernel's peak is not a monotone bound on the + // kernel's current at read time, so a sample that reported VmHWM verbatim + // could show a peak below its own current - which is a number t1 cannot put + // in MEASUREMENTS and a test cannot assert against. GitHub run 35079459114. struct RoleMemorySample { std::uint64_t PeakRssBytes = 0; std::uint64_t CurrentRssBytes = 0; @@ -75,8 +100,21 @@ namespace MobileGL::MG_Remote::Transport { MemoryRole Role = MemoryRole::Client; }; + // One pass over /proc/self/status folded into the PROCESS's running peak. RoleMemorySample SampleRoleMemory(MemoryRole role); + // The fold itself, over a running peak the CALLER owns: `runningPeak` + // becomes max(runningPeak, kernelPeakRssBytes, kernelCurrentRssBytes) and + // the sample reports that as PeakRssBytes beside `kernelCurrentRssBytes`. + // SampleRoleMemory calls this with the process-wide running peak and the + // numbers it just read; SessionTest calls it with a running peak of its own + // and a stubbed reader whose current EXCEEDS its peak, which must not fail + // (R-16's control on this rule: revert PeakRssBytes to the kernel's peak and + // it does). + RoleMemorySample SampleRoleMemoryInto(std::atomic& runningPeak, MemoryRole role, + std::uint64_t kernelPeakRssBytes, + std::uint64_t kernelCurrentRssBytes); + // Emits one line at INFO level, which is what every P5 lane builds at, so // t1's harness can grep it out of a lane log without a new log sink. Not // DEBUG, which the INFO build compiles out; not ERROR, which this is not. diff --git a/MobileGL/MG_Remote/Transport/SessionRings.h b/MobileGL/MG_Remote/Transport/SessionRings.h index 4a78f2ef..a25d9f3e 100644 --- a/MobileGL/MG_Remote/Transport/SessionRings.h +++ b/MobileGL/MG_Remote/Transport/SessionRings.h @@ -53,7 +53,8 @@ // // SegmentRef.sizeBytes therefore announces the MAPPING size (ring + page), which // is what a spawn peer must mmap. The four numbers a reader recognises - 8 MiB / -// 32 MiB / 8 MiB / 256 KiB - are the RING sizes, which is what the knobs name. +// 32 MiB / 16 MiB / 256 KiB (ID-47) - are the RING sizes, which is what the +// knobs name. // // SEG_STAGE has no control page of its own: RingControl carries TWO cursor // triples (Ring.h:101-109) and the stage triple is the second. So SEG_STAGE is @@ -86,8 +87,13 @@ namespace MobileGL::MG_Remote::Transport { // two either. Rounding was a ring requirement and keeping it would have // silently turned an operator's MOBILEGL_IPC_STAGE_MB=24 into 16. std::uint64_t StageBytes = 32ull * 1024 * 1024; - std::uint64_t ReplyBytes = 8ull * 1024 * 1024; // slot pool, not a ring - std::uint64_t EventRingBytes = 256ull * 1024; // + one control page + // SEG_REPLY: a slot pool, not a ring, and NO KNOB MOVES IT (contract §5 has + // none). ID-47: 16 MiB = eight slots of 2 MiB, sized from the largest P5 + // read - the E2 retrace's full-surface 640x480 RGBA8 snapshot, 1,228,800 + // bytes - which the previous 8 MiB / 1 MiB-per-slot pool could not hold + // (ReplySlot.h says how it was found). ProtocolSmokeTest pins the number. + std::uint64_t ReplyBytes = 16ull * 1024 * 1024; + std::uint64_t EventRingBytes = 256ull * 1024; // + one control page std::uint32_t ReplySlotCount = kDefaultReplySlotCount; }; @@ -473,17 +479,48 @@ namespace MobileGL::MG_Remote::Transport { }; // ----------------------------------------------------------------------- - // The ABI fingerprint's mixer. + // The ABI fingerprint's mixer - THE ONE IMPLEMENTATION. // - // It lives under Transport/ rather than in CapsCodec.cpp so that it can be - // tested without the GL frontend's umbrella header, and so that the SIZES it - // mixes are the caller's - CapsCodec.cpp passes the three real sizeofs, a - // unit test passes made-up ones and can then prove a one-byte difference - // changes the answer. A fingerprint that cannot be shown to change is - // indistinguishable from one that is never compared. + // CapsCodec.cpp's CapsAbiFingerprint(), the value both handshakes compare, + // is exactly MixAbiFingerprint(CapsAbiFingerprintInputs()). It lives under + // Transport/ so that it can be tested without the GL frontend's umbrella + // header, and it takes its inputs as a struct so that the SAME function the + // handshake calls can be driven with one field perturbed at a time. + // + // The wave-1 review (ID-46 finding 6) found the previous shape - a + // five-argument mixer here and a SEPARATE hand-rolled FNV loop in + // CapsCodec.cpp - had exactly one caller of this function: the sensitivity + // test. Production never called it, so replacing CapsAbiFingerprint() with + // `return 1;` left every fingerprint test green and both peers agreeing on + // nothing. Now there is one mixer, the sensitivity case starts from the + // production entry point, and that perturbation turns it red. // ----------------------------------------------------------------------- - std::uint64_t MixAbiFingerprint(std::uint64_t dynamicParamsSize, std::uint64_t capsSize, - std::uint64_t functionTableSize, std::uint32_t abiVersion, - const char* buildStamp); + struct AbiFingerprintInputs { + // The three struct shapes table 0's ABI-agreement row names. + std::uint64_t DynamicParamsSize = 0; + std::uint64_t CapsSize = 0; + std::uint64_t FunctionTableSize = 0; + // The caps blob's own geometry and the two blob codecs' versions: the + // format-capability table's extents and the codec version stamps. + std::uint64_t FormatCapabilityTargets = 0; + std::uint64_t FormatCapabilityFormats = 0; + std::uint64_t FormatCapabilitiesCodecVersion = 0; + std::uint64_t RendererInfoCodecVersion = 0; + // The catalogue's length (ID-33): a peer with one more opcode is a + // different wire even if every struct kept its size. + std::uint64_t OpCount = 0; + // MOBILEGL_ABI_VERSION(major, minor): a protocol change that left every + // struct the same size, which nothing above can see. + std::uint32_t AbiVersion = 0; + // GIT_COMMIT_HASH_SHORT: two builds of the same sizes can still disagree + // about a FIELD ORDER, which no sizeof can see. nullptr and "" are + // distinct inputs and neither equals a real stamp. + const char* BuildStamp = nullptr; + }; + + // FNV-1a over every field above, in declaration order. Never 0: that value is + // reserved for "not stated", so a peer that forgot to fill the field cannot + // accidentally agree with one that did. + std::uint64_t MixAbiFingerprint(const AbiFingerprintInputs& inputs); } // namespace MobileGL::MG_Remote::Transport diff --git a/MobileGL/MG_Remote/Transport/ShmSegment.cpp b/MobileGL/MG_Remote/Transport/ShmSegment.cpp index 3ec969ae..16f5bdcf 100644 --- a/MobileGL/MG_Remote/Transport/ShmSegment.cpp +++ b/MobileGL/MG_Remote/Transport/ShmSegment.cpp @@ -78,43 +78,73 @@ namespace MobileGL::MG_Remote::Transport { return role == MemoryRole::Server ? "server" : "client"; } - // One pass over /proc/self/status for a "VmHWM:" / "VmRSS:" line. The - // values are in kB and the unit suffix is part of the line, so it is - // parsed rather than assumed. - std::uint64_t ProcStatusBytes(const char* key) { -#if defined(__linux__) || defined(__ANDROID__) - std::FILE* file = std::fopen("/proc/self/status", "re"); - if (file == nullptr) { - return 0; - } + // ":\t kB" -> bytes. The unit suffix is part of the + // line, so it is parsed rather than assumed; anything else is a kernel + // this code has not seen, and 0 ("not measured") is the honest answer. + bool ParseKilobyteLine(const char* line, const char* key, std::uint64_t* outBytes) { const std::size_t keyLength = std::strlen(key); - char line[256]; - std::uint64_t bytes = 0; - while (std::fgets(line, sizeof(line), file) != nullptr) { - if (std::strncmp(line, key, keyLength) != 0) { - continue; - } - unsigned long long kilobytes = 0; - // The format is ":\t kB". Anything else is a - // kernel this code has not seen, and 0 ("not measured") is the - // honest answer for it. - if (std::sscanf(line + keyLength, ": %llu kB", &kilobytes) == 1) { - bytes = static_cast(kilobytes) * 1024ull; - } - break; + if (std::strncmp(line, key, keyLength) != 0) { + return false; } - std::fclose(file); - return bytes; -#else - (void)key; - return 0; -#endif + unsigned long long kilobytes = 0; + if (std::sscanf(line + keyLength, ": %llu kB", &kilobytes) == 1) { + *outBytes = static_cast(kilobytes) * 1024ull; + } + return true; + } + + // The process-wide running peak SampleRoleMemory folds into. One per + // process, like VmHWM itself; under inproc both roles share it and the + // log line says so. + std::atomic& ProcessRunningPeak() { + static std::atomic peak{0}; + return peak; } } // namespace - std::uint64_t ProcessPeakRssBytes() { return ProcStatusBytes("VmHWM"); } + // ONE PASS FOR BOTH KEYS - see RoleMemory.h. A pass per key was the wave-1 + // shape and it is what GitHub run 35079459114 caught: the second fopen grew + // RSS past the VmHWM the first pass had reported, because the kernel only + // stores hiwater_rss when RSS is about to drop and reports max(stored, now) + // otherwise, so the two reads were snapshots of different "now"s. + void ProcessRssBytes(std::uint64_t* outPeakBytes, std::uint64_t* outCurrentBytes) { + std::uint64_t peak = 0; + std::uint64_t current = 0; +#if defined(__linux__) || defined(__ANDROID__) + std::FILE* file = std::fopen("/proc/self/status", "re"); + if (file != nullptr) { + char line[256]; + bool sawPeak = false; + bool sawCurrent = false; + while ((!sawPeak || !sawCurrent) && std::fgets(line, sizeof(line), file) != nullptr) { + if (!sawPeak && ParseKilobyteLine(line, "VmHWM", &peak)) { + sawPeak = true; + } else if (!sawCurrent && ParseKilobyteLine(line, "VmRSS", ¤t)) { + sawCurrent = true; + } + } + std::fclose(file); + } +#endif + if (outPeakBytes != nullptr) { + *outPeakBytes = peak; + } + if (outCurrentBytes != nullptr) { + *outCurrentBytes = current; + } + } - std::uint64_t ProcessCurrentRssBytes() { return ProcStatusBytes("VmRSS"); } + std::uint64_t ProcessPeakRssBytes() { + std::uint64_t peak = 0; + ProcessRssBytes(&peak, nullptr); + return peak; + } + + std::uint64_t ProcessCurrentRssBytes() { + std::uint64_t current = 0; + ProcessRssBytes(nullptr, ¤t); + return current; + } void LedgerAddSegment(MemoryRole role, std::uint64_t bytes) { LedgerSlot(role).fetch_add(bytes, std::memory_order_relaxed); @@ -140,15 +170,35 @@ namespace MobileGL::MG_Remote::Transport { return total; } - RoleMemorySample SampleRoleMemory(MemoryRole role) { + RoleMemorySample SampleRoleMemoryInto(std::atomic& runningPeak, MemoryRole role, + std::uint64_t kernelPeakRssBytes, + std::uint64_t kernelCurrentRssBytes) { + // max(running, kernel peak, kernel current), as a CAS loop: two threads + // sampling at once (the GL thread and the apply thread both log memory + // at phase boundaries) must not let a lower value overwrite a higher. + const std::uint64_t observed = + kernelPeakRssBytes > kernelCurrentRssBytes ? kernelPeakRssBytes : kernelCurrentRssBytes; + std::uint64_t peak = runningPeak.load(std::memory_order_relaxed); + while (observed > peak && + !runningPeak.compare_exchange_weak(peak, observed, std::memory_order_relaxed)) { + } RoleMemorySample sample; sample.Role = role; - sample.PeakRssBytes = ProcessPeakRssBytes(); - sample.CurrentRssBytes = ProcessCurrentRssBytes(); + // THE RUNNING MAXIMUM, never the kernel's peak verbatim - RoleMemory.h + // says why, and SessionTest's stubbed-reader control is the gate on it. + sample.PeakRssBytes = observed > peak ? observed : peak; + sample.CurrentRssBytes = kernelCurrentRssBytes; sample.MappedSegmentBytes = LedgerMappedBytes(role); return sample; } + RoleMemorySample SampleRoleMemory(MemoryRole role) { + std::uint64_t kernelPeak = 0; + std::uint64_t kernelCurrent = 0; + ProcessRssBytes(&kernelPeak, &kernelCurrent); + return SampleRoleMemoryInto(ProcessRunningPeak(), role, kernelPeak, kernelCurrent); + } + void LogRoleMemory(const char* phase, const RoleMemorySample& sample) { // INFO, not DEBUG: t1 greps this out of a lane log and MGLOG_D is compiled out at the // INFO level every P5 lane builds at. It is a handful of lines per session - the diff --git a/MobileGL/MG_Remote/Transport/WireLog.cpp b/MobileGL/MG_Remote/Transport/WireLog.cpp index 34d18a53..4dba41d5 100644 --- a/MobileGL/MG_Remote/Transport/WireLog.cpp +++ b/MobileGL/MG_Remote/Transport/WireLog.cpp @@ -12,6 +12,7 @@ #include #include +#include namespace MobileGL::MG_Remote::Transport { @@ -30,4 +31,23 @@ namespace MobileGL::MG_Remote::Transport { MGLOG_E("%s", line); } + void WireLogFatal(const char* format, ...) { + 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) { + std::snprintf(line, sizeof(line), + "MG_Remote wire: unformattable Fatal diagnostic (format=%s)", format); + } + MGLOG_F("%s", line); + // The stderr echo is the half a death test can see (WireLog.h). Unbuffered + // by default and flushed anyway: abort() does not flush stdio. + std::fputs(line, stderr); + std::fputc('\n', stderr); + std::fflush(stderr); + std::abort(); + } + } // namespace MobileGL::MG_Remote::Transport diff --git a/MobileGL/MG_Remote/Transport/WireLog.h b/MobileGL/MG_Remote/Transport/WireLog.h index 1be056d3..46b8e8f9 100644 --- a/MobileGL/MG_Remote/Transport/WireLog.h +++ b/MobileGL/MG_Remote/Transport/WireLog.h @@ -19,9 +19,9 @@ // 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. +// ERROR and FATAL 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 @@ -35,4 +35,25 @@ namespace MobileGL::MG_Remote::Transport { void WireLogError(const char* format, ...); + // Formats one line, emits it at FATAL level (MGLOG_F), ECHOES IT TO STDERR, + // and aborts. Every `Fatal{...}` this layer raises goes through here. + // + // WHY STDERR AS WELL. Defines.h builds the logger with the console sink OFF + // and the file sink ON, so an MGLOG line reaches exactly one place: the log + // file. A Fatal is the last thing the process says, and three readers need + // it somewhere other than a file whose path they may not know: a terminal, + // a CI job log, and a gtest death test - which matches its regex against + // the CHILD'S STDERR and nothing else. The wave-1 review (ID-46 finding 10) + // found both SessionTestDeath controls written with an empty regex for + // precisely that reason: with the diagnostic reachable only through the + // file sink there was nothing on stderr to name, so a bare std::abort() or + // a segfault satisfied them. The echo is what lets a death control name + // its diagnostic (R-16: a negative control asserts its own failure reason). + [[noreturn]] +#if defined(__GNUC__) || defined(__clang__) + __attribute__((format(printf, 1, 2))) +#endif + void + WireLogFatal(const char* format, ...); + } // namespace MobileGL::MG_Remote::Transport diff --git a/MobileGL/MG_Remote/Wire/PipeWireCodec.cpp b/MobileGL/MG_Remote/Wire/PipeWireCodec.cpp index 5ba4709f..1f901c27 100644 --- a/MobileGL/MG_Remote/Wire/PipeWireCodec.cpp +++ b/MobileGL/MG_Remote/Wire/PipeWireCodec.cpp @@ -31,6 +31,9 @@ #include #include #include +// R-6's tier gate, and the ONE spelling of it (b1's file, unchanged by this package): the +// MapPersistent arm below asks it the same question MGPipeApplyMapPersistent asks. +#include #include #include #include @@ -457,6 +460,36 @@ namespace MobileGL::MG_Remote::Wire { WireOpName(op), static_cast(blob.Size)); std::abort(); } + // R-2.3's SECOND HALF: "inside SOME segment" IS NOT THE RULE. Contract table 1 gives + // every client->server content blob - groups A, B and C, all nineteen rows - the ONE + // carrier SEG_STAGE, and R-10 sends blobs there whole. Until this arm existed the only + // test was that the run resolved, so `CreateSamplerState.Parameters={Seg=SEG_REPLY,...}` + // was accepted and APPLIED: a server-owned segment, whose reuse is the reply pool's + // business and has nothing to do with stage retirement, carrying bytes the applier + // then read. It also went unpoisoned - NoteResolvedRun skipped every non-stage carrier + // - so rule C's only mechanical control read zero on exactly the record that needed it. + // + // The segment is checked BEFORE the resolve, deliberately: a forged SEG_REPLY run that + // happens to lie inside a mapped reply pool must be refused for naming the wrong + // carrier, not left to pass or fail on whether that pool is mapped at all. + // + // NOT A NEW FATAL FAMILY. The review suggested `Fatal{BlobNotStaged}`; this is + // ProtocolCorruption like every other R-2 honesty arm, because the families are the + // vocabulary the operator and the CI greps share (ProtocolCorruption, AbiMismatch, + // UnmigratedVerb, UnmigratedPipeInput, UnsetCallMask, RingOverrun) and a one-off + // seventh name would be a token nothing else in the tree recognises. The SEGMENT is in + // the message, which is what has to be greppable. + if (blob.Seg != kSegStage) { + MGLOG_F("MGPipe: Fatal{ProtocolCorruption, \"%s.blob\"} seg=%u offset=%llu " + "size=%llu is not SEG_STAGE(%u); every client->server content blob is " + "staged whole in SEG_STAGE (contract table 1 groups A/B/C, R-10) and no " + "other segment may carry one", + WireOpName(op), static_cast(blob.Seg), + static_cast(blob.Offset), + static_cast(blob.Size), + static_cast(kSegStage)); + std::abort(); + } if (segments.Resolve(blob.Seg, blob.Offset, blob.Size) == nullptr) { MGLOG_F("MGPipe: Fatal{ProtocolCorruption, \"%s.blob\"} seg=%u offset=%llu size=%llu " "does not lie inside that segment (R-2.3)", @@ -717,6 +750,12 @@ namespace MobileGL::MG_Remote::Wire { } for (int attempt = 0; attempt < 2; ++attempt) { + // THE WRAP SKIP MAY ONLY BE CHARGED AGAINST BYTES THAT ARE STILL IN FLIGHT. When + // there are none the allocator starts over at offset zero, so a blob the segment + // can hold whole is never refused (see RebaseEmptyStage). On attempt 1 this runs + // AFTER ReclaimStagedBytes, which is the case the finding describes: 8 MiB + // allocated, then retired, then a 28 MiB request that used to abort. + RebaseEmptyStage(); const Uint64 offset = m_stageHead % m_stageCapacity; // A run is always contiguous: one that would straddle the end skips the remainder, // exactly as the ring's wrap pad does, and the skipped bytes are reclaimed with @@ -936,6 +975,32 @@ namespace MobileGL::MG_Remote::Wire { return m_emitSeq; } + // THE EMPTY-STAGE REBASE. Head and tail are monotonic byte counts, so "every staged byte + // has retired" reads head == tail, NOT head == tail == 0, and `head % capacity` is left + // wherever the last run ended. Charging a wrap skip against that offset then costs the + // unused suffix a second time: with head == tail == 64 in a 256 KiB stage, the allocator's + // test became `2*capacity - 64 <= capacity`, which is false at EVERY occupancy, so a blob + // that fits the segment whole was refused with `Fatal{RingOverrun, "SEG_STAGE"}` - whose + // own message then reported `0 bytes still in flight`. ReclaimStagedBytes cannot help, + // because an already-empty tail has nothing left to move. + // + // THE MARK QUEUE COMES WITH IT. A mark holds the ABSOLUTE head cursor it was pushed at and + // ReclaimStagedBytes assigns that value straight to m_stageTail. Every mark not yet + // consumed has StageCursor <= head == tail - the head is monotonic and marks are pushed in + // order - so each of them names a region that is already reclaimed and zero is the + // truthful rebasing of it. Without that, one reclaim after a rebase would put the tail + // AHEAD of the head and StagedBytesInFlight() would underflow to about 2^64. + void PipeWireEncoder::RebaseEmptyStage() { + if (m_stageHead != m_stageTail || m_stageHead == 0) { + return; + } + m_stageHead = 0; + m_stageTail = 0; + for (SizeT i = 0; i < m_stageMarks.size(); ++i) { + m_stageMarks[i].StageCursor = 0; + } + } + void PipeWireEncoder::ReclaimStagedBytes() { if (m_control == nullptr) { return; @@ -1083,8 +1148,22 @@ namespace MobileGL::MG_Remote::Wire { } void PipeWireDecoder::NoteResolvedRun(MGPWireOp op, const MGPBlobRef& blob) { + // UNREACHABLE NOW, AND LOUD RATHER THAN SILENT. This used to `return`, and that made + // the audit's bookkeeping quietly optional: a record naming a non-SEG_STAGE carrier + // was applied AND recorded nothing, so PoisonedStageBytes() stayed zero and rule C's + // only mechanical control was dark on exactly the record it existed to catch. The one + // caller is ResolveOrFatal, which runs RequireDeclaredBlob first, and that now refuses + // both an undeclared blob and a non-SEG_STAGE one by name. If either ever arrives here + // the audit has stopped covering the carrier, which is the same failure as no audit at + // all - the reason the run-count overflow just below is a Fatal too. if (blob.Size == 0 || blob.Seg != kSegStage) { - return; + MGLOG_F("MGPipe: Fatal{ProtocolCorruption, \"%s\"} the audit was asked to record a " + "resolved run with seg=%u size=%llu; only declared SEG_STAGE(%u) runs " + "reach the poison fill (R-2.5)", + WireOpName(op), static_cast(blob.Seg), + static_cast(blob.Size), + static_cast(kSegStage)); + std::abort(); } if (m_resolvedCount >= sizeof(m_resolved) / sizeof(m_resolved[0])) { // LOUD, NOT A SILENT DROP. This array is what the 0xDD fill covers, and a poison @@ -1323,6 +1402,19 @@ namespace MobileGL::MG_Remote::Wire { // // DECLINED is a real answer, not a failure: the three frontend sites already // tolerate it (BufferObject.cpp:238, :603-606, :657-660). + // + // THE TIER IS CONSULTED HERE, AND IT IS THE SAME CONJUNCTION THE MONOLITH APPLIER + // USES (PipeApply.cpp's `Transport != Monolith && AdoptTierIsEmulate()`). The arm + // used to decline UNCONDITIONALLY and AdoptTier had no reference anywhere on the + // codec path, so MOBILEGL_IPC_ADOPT_TIER=0 and =1 - which contract §5 promises + // "parse and are Fatal at use, naming P11" - decoded as an ordinary DECLINED and + // the operator got a run that looked like a working T0. AdoptTierIsEmulate returns + // true at T2 and ABORTS at T0/T1 on its own named diagnostic, so the return value + // is deliberately not a branch: P5 declines at every tier it survives (R-6), and + // the two forbidden ones never get this far. + if (MG_Config::Transport != MG_Config::TransportMode::Monolith) { + (void)MG_Remote::Client::AdoptTierIsEmulate(); + } PostReply(op, seq, ReplySink::kStatusDeclined, nullptr, 0); return true; diff --git a/MobileGL/MG_Remote/Wire/PipeWireCodec.h b/MobileGL/MG_Remote/Wire/PipeWireCodec.h index 1f87f2df..ea2360bb 100644 --- a/MobileGL/MG_Remote/Wire/PipeWireCodec.h +++ b/MobileGL/MG_Remote/Wire/PipeWireCodec.h @@ -290,6 +290,17 @@ namespace MobileGL::MG_Remote::Wire { // RingControl - see ReclaimStagedBytes above. Uint8* StageAllocate(Uint64 size); + // AN EMPTY STAGE STARTS OVER AT ZERO, so that the wrap skip is only ever charged + // against bytes that are really still in flight. Head and tail are monotonic, so once + // everything has retired they are EQUAL BUT NOT ZERO, and `head % capacity` is + // wherever the last run happened to end - a wrap skip charged against that offset + // costs the suffix a second time and refused a blob the whole segment could hold, with + // a message that reported zero bytes in flight while it did so. Rebasing also rewrites + // the marks still held: a mark stores an ABSOLUTE head cursor and a later reclaim + // assigns it to m_stageTail, so leaving a stale one behind would drive the tail past + // the head and underflow StagedBytesInFlight(). + void RebaseEmptyStage(); + Transport::RingControl* m_control = nullptr; Transport::RingProducer* m_cmd = nullptr; Transport::RingProducer* m_stage = nullptr; diff --git a/MobileGL/MG_Test/Buffer/SplitBufferTest.cpp b/MobileGL/MG_Test/Buffer/SplitBufferTest.cpp index a1370ddb..d60f7aa1 100644 --- a/MobileGL/MG_Test/Buffer/SplitBufferTest.cpp +++ b/MobileGL/MG_Test/Buffer/SplitBufferTest.cpp @@ -41,6 +41,21 @@ namespace { using MG_Remote::Client::GpuWriteProducer; using MG_Remote::Client::PersistentMapTracker; + // A backend that MINTS a persistent mapping, for the one case that needs the adopted arm + // (TheAdoptedArmIsNotAMemberAndItIsTheChainRowThatSaysSo). Only AcquirePersistentMap is + // filled in: the frontend null-checks every op individually, and a table with one live + // member is the smallest thing that makes AcquireMemoryRange's legacy adoption arm fire. + // The storage is the CASE's, not this table's - AdoptPersistentMap keeps the pointer and + // never owns it - so the base travels through a file-scope variable the case sets and + // clears around the one acquisition it wants minted. + void* g_mintedPersistentBase = nullptr; + void* MintPersistentMap(BufferObject&) { return g_mintedPersistentBase; } + const MG_State::GLState::BufferBackendOps g_mintingBufferOps = [] { + MG_State::GLState::BufferBackendOps ops{}; + ops.AcquirePersistentMap = &MintPersistentMap; + return ops; + }(); + // Everything in this package is gated on `Transport != Monolith`, so every case has to // put the process into a split configuration and put it back. A fixture rather than a // lambda because the tracker is a process-wide singleton and a case that left an entry in @@ -64,6 +79,11 @@ namespace { MG_Remote::Client::ResetProducerMarkCountsForTest(); } void TearDown() override { + // Belt and braces for the one case that installs a minting backend: a table left + // behind would make the NEXT case's acquisition land in the adopted arm, and every + // membership assertion in this file would then be about a different code path. + MG_State::GLState::SetBufferBackendOps(nullptr); + g_mintedPersistentBase = nullptr; PersistentMapTracker::Instance().ClearForTest(); MG_State::pGLContext = Move(m_context); MG_Config::Transport = m_transport; @@ -258,6 +278,92 @@ TEST_F(SplitBufferSet, MembershipIsSyncPersistentMappedRangesOwnEarlyOutChain) { buffer->ReleaseMemory(false); } +// THE ADOPTED ARM IS NOT A MEMBER, AND IT IS THE CHAIN THAT SAYS SO - NOT THE TIER (ID-42). +// +// IsLivePersistentMap's second row is `if (buffer.IsBackendPersistentMapped()) return false;`, +// and its comment promises that the row answers for ITSELF: at tier T2 the arm is unreachable +// because MapPersistent declines, but a build that reaches T0/T1 later must get the same answer +// out of the same row. Nothing drove that promise. The case above only ever sees the declined +// arm, and PersistentCoherentMapScenario's membership assertion cannot pin it either - an +// integration entry sees whichever arm its driver and transport hand it, which is exactly how +// that assertion came to state the emulated arm's property as if it were universal and go red on +// seven split-monolith entries the first time it ran. +// +// So the statement is pinned HERE, where both answers can be produced on demand. The adoption is +// made by the PRODUCTION path - AcquireMemoryRange dispatching to the backend's +// AcquirePersistentMap and calling PipeResource::AdoptPersistentMap - and not by the case +// reaching into the buffer, because a hand-set flag would still be "true" with the production +// adoption deleted (R-16). The transport is Monolith for exactly that call, because R-6's decline +// is ANDed with `Transport != Monolith` and there is no other way to reach a mint in this build; +// the PREDICATE is then asked with the tier back at T2/InProcess, which is the whole point: the +// tier says "emulated, always" and the chain still says "not a member". +TEST_F(SplitBufferSet, TheAdoptedArmIsNotAMemberAndItIsTheChainRowThatSaysSo) { + constexpr SizeT kSize = 4096; + // The storage the backend "mints". Declared first so it outlives the buffer: AdoptPersistentMap + // stores the pointer and releases the shadow, and it never owns what it was handed. + Vector minted(kSize, static_cast(0)); + g_mintedPersistentBase = minted.data(); + + auto adopted = MakeBuffer(27u, kSize); + auto declined = MakeBuffer(28u, kSize); + + // The declined twin first, with no backend ops at all: same flags, same size, same call. + declined->AcquireMemoryRange(Range1D{0, kSize}, + BufferMappingAccessBit::Write | BufferMappingAccessBit::Persistent); + ASSERT_FALSE(declined->IsBackendPersistentMapped()); + ASSERT_TRUE(PersistentMapTracker::IsLivePersistentMap(*declined)) + << "the emulated arm IS a member - if this fails the two halves are not comparable and the " + "assertion below proves nothing"; + + { + MG_State::GLState::SetBufferBackendOps(&g_mintingBufferOps); + MG_Config::Transport = MG_Config::TransportMode::Monolith; + adopted->AcquireMemoryRange(Range1D{0, kSize}, + BufferMappingAccessBit::Write | BufferMappingAccessBit::Persistent); + MG_Config::Transport = MG_Config::TransportMode::InProcess; + MG_State::GLState::SetBufferBackendOps(nullptr); + } + ASSERT_TRUE(adopted->IsBackendPersistentMapped()) + << "the backend declined the mint, so there is no adopted arm here to ask about"; + + // THE TIER SAYS EMULATED. The chain must still say "not a member". + ASSERT_TRUE(MG_Remote::Client::AdoptTierIsEmulate()); + ASSERT_NE(MG_Config::Transport, MG_Config::TransportMode::Monolith); + EXPECT_FALSE(PersistentMapTracker::IsLivePersistentMap(*adopted)) + << "an adopted store's bytes are already in host-visible coherent GPU memory and there is " + "nothing for the push to ship, so IsBackendPersistentMapped() takes it out of the set. " + "This answer must come from that ROW and not from the adopt tier: the tier is T2 and the " + "transport is InProcess right now, which is the configuration in which R-6 says every " + "acquisition declines - and the store in front of the predicate is adopted anyway, " + "because a later phase's T0/T1 will mint one. A predicate that read the tier would call " + "it a member and the push would read an adopted store's Bytes() as if it were the " + "shadow."; + + // The SET agrees with the predicate, asked through the production entry point rather than by + // reading a member: NoteMapStateChanged is what every one of the five maintenance events + // calls, and it must refuse to enrol an adopted store. + PersistentMapTracker::Instance().NoteMapStateChanged(*adopted); + EXPECT_EQ(PersistentMapTracker::Instance().MemberCount(), 1u) + << "only the declined twin: enrolling an adopted store would make the push read its " + "Bytes() - which is the GPU map, not the shadow - as if it were bytes to ship"; + + // ...and the consequence, which is the one that would actually corrupt something. + const Uint64 before = MG_Util::PipeStats::TotalBytes(MG_Util::PipeStats::ByteClass::PersistentMapPush); + MG_Remote::Client::PushPersistentMapsBeforeVerb(); + EXPECT_EQ(MG_Util::PipeStats::TotalBytes(MG_Util::PipeStats::ByteClass::PersistentMapPush) - before, + static_cast(kSize)) + << "the declined twin's 4096 bytes and nothing else: the adopted buffer must contribute no " + "pushed bytes at all"; + + declined->ReleaseMemory(false); + // The adopted one is NOT released through ReleaseMemory: ReleasePersistentMap is for a store + // being redefined, and a persistent map the application holds outlives every unmap by + // definition (PipeResource.h:121-127). It is dropped here with the mapping still adopted, + // which is also the shape ~BufferObject has to survive. + adopted.reset(); + g_mintedPersistentBase = nullptr; +} + // pmap is non-zero, and it is non-zero in BLOCKS. TEST_F(SplitBufferSet, ThePushCutsTheMappedSpanIntoBlocksAndMovesPmap) { constexpr SizeT kSize = 4u * 64u * 1024u; // exactly four 64 KiB blocks @@ -402,7 +508,7 @@ TEST_F(SplitBufferSet, OnlyAdoptTierTwoIsImplemented) { #else -// THE SAME FIFTEEN NAMES, SO THE ctest NAME SET DOES NOT MOVE BETWEEN LANES. G2 compares the +// THE SAME SIXTEEN NAMES, SO THE ctest NAME SET DOES NOT MOVE BETWEEN LANES. G2 compares the // pull and push name lists line for line and G14 allows build-split to ADD names but never to // remove one, so a case that exists only where it can run would break both gates for a reason // that has nothing to do with what it tests. It skips instead, and says why. @@ -417,6 +523,7 @@ TEST(SplitBufferSet, Row4AReadPixelsIntoAPackPboMarksThePbo) { MGL_SPLIT_ONLY_OR TEST(SplitBufferSet, Row5EndTransformFeedbackMarksTheCaptureTargets) { MGL_SPLIT_ONLY_OR_SKIP(); } TEST(SplitBufferSet, TheWholeSetIsInertOnTheMonolithPath) { MGL_SPLIT_ONLY_OR_SKIP(); } TEST(SplitBufferSet, MembershipIsSyncPersistentMappedRangesOwnEarlyOutChain) { MGL_SPLIT_ONLY_OR_SKIP(); } +TEST(SplitBufferSet, TheAdoptedArmIsNotAMemberAndItIsTheChainRowThatSaysSo) { MGL_SPLIT_ONLY_OR_SKIP(); } TEST(SplitBufferSet, ThePushCutsTheMappedSpanIntoBlocksAndMovesPmap) { MGL_SPLIT_ONLY_OR_SKIP(); } TEST(SplitBufferSet, TheLastBlockIsTheRemainderAndNotAWholeBlock) { MGL_SPLIT_ONLY_OR_SKIP(); } TEST(SplitBufferSet, BothEdgesOfAWriteMapPublishOneStateRecord) { MGL_SPLIT_ONLY_OR_SKIP(); } diff --git a/MobileGL/MG_Test/Wire/CMakeLists.txt b/MobileGL/MG_Test/Wire/CMakeLists.txt index c5534b21..240e228a 100644 --- a/MobileGL/MG_Test/Wire/CMakeLists.txt +++ b/MobileGL/MG_Test/Wire/CMakeLists.txt @@ -66,21 +66,25 @@ endif () gtest_discover_tests(PipeWireCodecTest DISCOVERY_TIMEOUT 30 PROPERTIES LABELS unit) -# P5 c1's and v1's suites. Both are registered on their own, and both for -# PipeWireCodecTest's reason - MGLOG_F writes to stdout and to a named file and NEVER to -# stderr, so a Fatal arm can only be asserted by forking and reading that file back, which -# needs a main() of its own. Both also reach MG_Pipe, MG_Backend and MG_State, so both carry +# P5 c1's, v1's and s1's suites. All three are registered on their own, and all three for +# PipeWireCodecTest's reason: a Fatal arm reports through MGLOG_F, which writes to stdout and to +# a named file and NEVER to stderr (and with the console sink compiled out, Defines.h, only to +# the file), so asserting one means naming a log file before anything logs - which needs a +# main() of its own. All three also reach MG_Pipe, MG_Backend and MG_State, so all three carry # those include paths. # -# RemoteClientTest the 71-slot emit table, the caps mirror, R-8's liveness gates and -# R-17's routing (c1) -# ServerLoopTest the apply thread, the blocking control mailbox, the verb stamp and -# R-11's server-owned staging copy (v1) +# RemoteClientTest the 71-slot emit table, the caps mirror, R-8's liveness gates, R-17's +# routing and ID-47/ID-49's readback rules (c1) +# ServerLoopTest the apply thread, the blocking control mailbox, the verb stamp and +# R-11's server-owned staging copy (v1) +# SessionHandshakeTest the two null-union guards driven THROUGH ServerSession::Accept and +# ClientSession::StartOverTransportPair, and the ABI fingerprint's +# sensitivity case driven from CapsAbiFingerprint() (s1, ID-46 6 and 7) # -# THE ONLY CONFLICT IN THE c1/v1 MERGE, and it is the same-point-append shape BRIEF §5's -# ownership table exists to prevent: two packages each added a target at the end of one file. -# Both blocks are kept verbatim; nothing is chosen over anything. -foreach (wiretest IN ITEMS RemoteClientTest ServerLoopTest) +# THIS FILE IS THE PHASE'S ONE RECURRING MERGE CONFLICT, and it is the same-point-append shape +# BRIEF §5's ownership table exists to prevent: three packages, three targets, one end-of-file. +# The loop is what stops there being a fourth: a package adding a suite adds a NAME. +foreach (wiretest IN ITEMS RemoteClientTest ServerLoopTest SessionHandshakeTest) add_executable(${wiretest} ${wiretest}.cpp) target_include_directories(${wiretest} PRIVATE diff --git a/MobileGL/MG_Test/Wire/PipeWireCodecTest.cpp b/MobileGL/MG_Test/Wire/PipeWireCodecTest.cpp index f8eb11cc..4e1b8508 100644 --- a/MobileGL/MG_Test/Wire/PipeWireCodecTest.cpp +++ b/MobileGL/MG_Test/Wire/PipeWireCodecTest.cpp @@ -34,6 +34,8 @@ #include "Includes.h" +// MG_Config::Transport and MG_Config::Ipc.AdoptTier: the two knobs R-6's tier gate reads. +#include #include #include #include @@ -540,6 +542,37 @@ TEST_F(PipeWireCodecTest, KReplySlotMapPersistentIsAConstantDecline) { EXPECT_TRUE(wire.Answers().All[0].Bytes.empty()); } +TEST_F(PipeWireCodecTest, TierTwoUnderSplitTransportStillDeclinesRatherThanRefusing) { + // The POSITIVE half of the two AdoptTier death cases below. Without it, those two could + // be satisfied by an arm that aborted on every tier, which is the opposite mistake to the + // one wave1-codex-verify.md §4 found. T2 is the only tier P5 implements and R-6 says the + // answer there is DECLINED - a real answer, not a failure - even when the transport is + // the split one that makes the tier question live at all. + const MG_Config::TransportMode savedTransport = MG_Config::Transport; + const Uint32 savedTier = MG_Config::Ipc.AdoptTier; + struct Restore { + MG_Config::TransportMode T; + Uint32 A; + ~Restore() { + MG_Config::Transport = T; + MG_Config::Ipc.AdoptTier = A; + } + } restore{savedTransport, savedTier}; + MG_Config::Transport = MG_Config::TransportMode::InProcess; + MG_Config::Ipc.AdoptTier = 2u; + + Wire2 wire; + const MGPHandleOnly handle = HandleOnly(5, MGPipeKind::Buffer); + ASSERT_NE(wire.Encoder().EncodeRecord(MGPWireOp::MapPersistent, &handle, sizeof(handle)), + kInvalidSeq); + bool applied = false; + ASSERT_TRUE(wire.PumpOne(&applied)); + EXPECT_TRUE(applied); + ASSERT_EQ(wire.Answers().All.size(), 1u); + EXPECT_EQ(wire.Answers().All[0].Status, ReplySink::kStatusDeclined); + EXPECT_TRUE(wire.Answers().All[0].Bytes.empty()); +} + TEST_F(PipeWireCodecTest, KNeedsAckRespecifyCarriesItsRedefinitionScope) { // Contract table 1 row 19b. Without the carrier every per-level glTexImage*D would take // the whole-resource arm on the far side and eat the other levels' pending uploads, so @@ -1174,6 +1207,43 @@ TEST_F(PipeWireCodecTest, StagedBytesAreReclaimedOnlyBehindRetiredSeq) { ASSERT_TRUE(wire.PumpOne(&applied)); wire.Encoder().ReclaimStagedBytes(); EXPECT_EQ(wire.Encoder().StagedBytesInFlight(), 0u); + + // ---- AND AN EMPTY STAGE TAKES THE WHOLE SEGMENT --------------------------------- + // The verifier's extension of this case, kept (wave1-codex-verify.md §1). The 64-byte + // run has retired and in-flight bytes are ZERO, so every byte of SEG_STAGE is free - + // but head and tail are monotonic and both sit at 64, so `head % capacity` is 64 and the + // allocator used to charge a `capacity - 64` wrap skip against a capacity that had + // nothing in it. The test then read `2*capacity - 64 <= capacity`, false at every + // occupancy, and a blob the segment holds WHOLE aborted with + // `Fatal{RingOverrun, "SEG_STAGE"} ... with 0 bytes still in flight`. + // + // I made it red once, by doing X: X = deleting the `RebaseEmptyStage()` call at the top + // of StageAllocate's attempt loop (PipeWireCodec.cpp). The case then dies with SIGABRT + // inside PipeWireEncoder::StageAllocate on that message, exactly as the verifier + // recorded it. + // + // THE EXACT MAXIMUM. `need = Align8(size)` and the first bound is `need > capacity`, so + // an empty stage takes a blob of exactly the capacity the encoder adopted - here + // Wire2::kStageBytes, and in a real session the whole SEG_STAGE view, i.e. + // MOBILEGL_IPC_STAGE_MB (32 MiB by default; SessionRings.h keeps SEG_STAGE un-ringed and + // un-rounded, so there is no control page to subtract). + const Uint64 maxRecordBefore = wire.Encoder().MaxRecordBytesSeen(); + std::vector whole(Wire2::kStageBytes, 0x5A); + const MGPBlobRef full = wire.Encoder().StageBytes(whole.data(), whole.size()); + EXPECT_EQ(full.Offset, 0u) << "an empty stage must hand a whole-capacity blob offset zero"; + EXPECT_EQ(full.Size, Wire2::kStageBytes); + EXPECT_EQ(full.Seg, static_cast(kSegStage)); + EXPECT_EQ(wire.Encoder().StagedBytesInFlight(), Wire2::kStageBytes); + const void* back = wire.Segments().Resolve(full.Seg, full.Offset, full.Size); + ASSERT_NE(back, nullptr); + EXPECT_EQ(back, wire.StageBase()); + + // R-10's max-record counter DOES NOT SEE IT, and that is the point of R-10's carrier + // rule: EncodeRecord feeds m_maxRecordBytes from `layout.TotalBytes` - header + payload + + // tails, all of it SEG_CMD - while the blob leaves only {Seg, Offset, Size} in the + // record. A quarter-megabyte of staging moved the counter by zero bytes. SEG_STAGE has + // its own bound and its own named Fatal, and MaxRecordBytesSeen() is not it. + EXPECT_EQ(wire.Encoder().MaxRecordBytesSeen(), maxRecordBefore); } // ---- M2 / M3: SEG_STAGE's cursors and the mark queue -------------------------------------- @@ -1555,6 +1625,69 @@ TEST_F(PipeWireCodecTest, ARunThatLeavesItsSegmentIsFatal) { EXPECT_NE(r.Log.find("does not lie inside that segment"), std::string::npos) << r.Log; } +TEST_F(PipeWireCodecTest, AContentBlobCarriedOutsideSegStageIsFatalAtTheDecoder) { + // R-2.3's second half, and the verifier's finding-3 fixture kept as its own case + // (wave1-codex-verify.md §3). Contract table 1 row 17 puts CreateSamplerState's bytes in + // SEG_STAGE; here they sit in a mapped SEG_REPLY - the SERVER-owned reply pool, whose + // reuse has nothing to do with stage retirement - and the record names that segment. It + // used to be ACCEPTED and APPLIED, because the only test was that the run resolved + // somewhere: the verifier's probe printed `seg=3 accepted=1 poisoned=0`. + // + // The audit is armed, so the second half of the finding is nailed down too: with the + // poison ON, the record must DIE rather than be applied with PoisonedStageBytes() left at + // zero. NoteResolvedRun used to return silently for any non-stage carrier, which made + // rule C's only mechanical control dark on exactly the record it exists to catch; it is + // now a Fatal of its own and unreachable behind this arm. + // + // I made it red once, by doing X: X = deleting the `blob.Seg != kSegStage` arm in + // CheckBlobIsHonest (PipeWireCodec.cpp). The child then exits 0 instead of aborting and + // this case fails on DiedOfAbort - the verifier's `accepted=1` state. + const ChildResult r = RunInChild([] { + Wire2 wire; + std::vector replyBytes(4096, 0); + SamplerParameters params{}; + params.borderColorForm = BorderColorForm::Int; + std::memcpy(replyBytes.data(), ¶ms, sizeof(params)); + wire.Segments().Install(kSegReply, SegmentView{replyBytes.data(), replyBytes.size()}); + wire.Decoder().SetAuditPoison(true); + + MGPSamplerDesc desc{}; + desc.Cso = MakeHandle(88); + desc.Parameters.Seg = static_cast(kSegReply); + desc.Parameters.Offset = 0; + desc.Parameters.Size = sizeof(SamplerParameters); + ForgeAndDecode(wire, MGPWireOp::CreateSamplerState, &desc, sizeof(desc), nullptr, 0); + }); + ASSERT_TRUE(DiedOfAbort(r)) << DescribeStatus(r) << "\n" << r.Log; + EXPECT_NE(r.Log.find("is not SEG_STAGE"), std::string::npos) << r.Log; + EXPECT_NE(r.Log.find("CreateSamplerState.blob"), std::string::npos) << r.Log; + EXPECT_NE(r.Log.find("seg=3"), std::string::npos) << r.Log; +} + +TEST_F(PipeWireCodecTest, TheEncoderRefusesTheNonStageCarrierTheDecoderCallsFatal) { + // THE ENCODER MUST NOT ACCEPT A RECORD THE DECODER FATALS ON - the same symmetry + // TheEncoderRefusesThePerStageSpirvRunTheDecoderCallsFatal states one arm over. Under + // `inproc` a SEG_REPLY pointer resolves, so an emitter that staged into the reply pool + // would get a valid seq here and a Fatal on a peer, which is the asymmetry EncodeRecord's + // own honesty loop exists to prevent. + // + // I made it red once, by doing X: X = deleting the `blob.Seg != kSegStage` arm in + // CheckBlobIsHonest. EncodeRecord then returns a real seq and the child exits 0. + const ChildResult r = RunInChild([] { + Wire2 wire; + std::vector replyBytes(4096, 0); + wire.Segments().Install(kSegReply, SegmentView{replyBytes.data(), replyBytes.size()}); + MGPSamplerDesc desc{}; + desc.Cso = MakeHandle(88); + desc.Parameters.Seg = static_cast(kSegReply); + desc.Parameters.Offset = 0; + desc.Parameters.Size = sizeof(SamplerParameters); + (void)wire.Encoder().EncodeRecord(MGPWireOp::CreateSamplerState, &desc, sizeof(desc)); + }); + ASSERT_TRUE(DiedOfAbort(r)) << DescribeStatus(r) << "\n" << r.Log; + EXPECT_NE(r.Log.find("is not SEG_STAGE"), std::string::npos) << r.Log; +} + TEST_F(PipeWireCodecTest, AHalfDeclaredBlobIsFatalRatherThanReadAsAbsent) { // The shape a MONOLITH emitter produces - Seg None, Offset a host address, Size 0. Reading // it as "absent" would silently drop the bytes of every record an unconverted emitter sent. @@ -1810,6 +1943,59 @@ TEST_F(PipeWireCodecTest, ASecondProcessResolverIsFatalRatherThanASilentRace) { EXPECT_NE(r.Log.find("already installed"), std::string::npos) << r.Log; } +// ---- R-6 / contract §5: the two forbidden adoption tiers die ON THE WIRE PATH TOO -------- +// +// wave1-codex-verify.md §4: `AdoptTier` had ZERO references anywhere on the codec path, so +// MOBILEGL_IPC_ADOPT_TIER=0 and =1 - which contract §5 promises "parse and are Fatal at use, +// naming P11" - decoded as an ordinary DECLINED. The verifier set each forbidden tier inside +// KReplySlotMapPersistentIsAConstantDecline and watched its successful-decline assertions +// still pass, on BOTH tiers. +// +// THESE ARE FORKED, NOT EXPECT_DEATH, for the reason at the top of this file - and forking is +// what lets the case REQUIRE THE DIAGNOSTIC rather than any abort: r.Log is searched for the +// exact sentence AdoptTierIsEmulate prints. ID-46 finding 10 is an empty death regex; the +// EXPECT_NE lines below are the opposite of that, and a crash for any other reason fails the +// case on the log it prints. +// +// I made both red once, by doing X: X = restoring the unconditional decline in +// PipeWireCodec.cpp's MapPersistent arm (deleting the AdoptTierIsEmulate call). Both children +// then exit 0 having posted a clean DECLINED, and both cases fail on DiedOfAbort. + +TEST_F(PipeWireCodecTest, AdoptTierZeroIsFatalOnTheWirePathAndNamesP11) { + const ChildResult r = RunInChild([] { + // The child dies; nothing needs restoring. The transport half is the same conjunction + // MGPipeApplyMapPersistent uses - a monolith TRANSPORT mints like push (ID-42) and is + // not the arm this record can arrive on. + MG_Config::Transport = MG_Config::TransportMode::InProcess; + MG_Config::Ipc.AdoptTier = 0u; + Wire2 wire; + const MGPHandleOnly handle = HandleOnly(5, MGPipeKind::Buffer); + (void)wire.Encoder().EncodeRecord(MGPWireOp::MapPersistent, &handle, sizeof(handle)); + bool applied = false; + (void)wire.PumpOne(&applied); + }); + ASSERT_TRUE(DiedOfAbort(r)) << DescribeStatus(r) << "\n" << r.Log; + EXPECT_NE(r.Log.find("MOBILEGL_IPC_ADOPT_TIER=0 names adoption tier T0, which P11 implements"), + std::string::npos) + << r.Log; +} + +TEST_F(PipeWireCodecTest, AdoptTierOneIsFatalOnTheWirePathAndNamesP11) { + const ChildResult r = RunInChild([] { + MG_Config::Transport = MG_Config::TransportMode::InProcess; + MG_Config::Ipc.AdoptTier = 1u; + Wire2 wire; + const MGPHandleOnly handle = HandleOnly(5, MGPipeKind::Buffer); + (void)wire.Encoder().EncodeRecord(MGPWireOp::MapPersistent, &handle, sizeof(handle)); + bool applied = false; + (void)wire.PumpOne(&applied); + }); + ASSERT_TRUE(DiedOfAbort(r)) << DescribeStatus(r) << "\n" << r.Log; + EXPECT_NE(r.Log.find("MOBILEGL_IPC_ADOPT_TIER=1 names adoption tier T1, which P11 implements"), + std::string::npos) + << r.Log; +} + #else TEST_F(PipeWireCodecTest, TheFatalArmsNeedFork) { diff --git a/MobileGL/MG_Test/Wire/ProtocolSmokeTest.cpp b/MobileGL/MG_Test/Wire/ProtocolSmokeTest.cpp index 61d4f865..8800677a 100644 --- a/MobileGL/MG_Test/Wire/ProtocolSmokeTest.cpp +++ b/MobileGL/MG_Test/Wire/ProtocolSmokeTest.cpp @@ -69,11 +69,15 @@ TEST(ProtocolSmokeTest, HelloRoundTrips) { EXPECT_EQ(envelope->msg_as_Welcome(), nullptr); } +// The four canonical sizes: 8 MiB / 32 MiB / 16 MiB / 256 KiB. SEG_REPLY is 16 MiB by ID-47 +// (eight slots of 2 MiB, sized from the largest P5 read - E2's 640x480 RGBA8 snapshot - which +// the previous 8 MiB pool could not hold); SessionTest pins the same number on the mapping and +// CONTRACT-P5 §2 row 23 on paper. All three move together or not at all. TEST(ProtocolSmokeTest, WelcomeCarriesTheFourSegmentAnnouncements) { ::flatbuffers::FlatBufferBuilder builder(1024); auto cmd = CreateSegmentRefDirect(builder, 1, SegmentKind::Cmd, 8ull * 1024 * 1024, "cmd"); auto stage = CreateSegmentRefDirect(builder, 2, SegmentKind::Stage, 32ull * 1024 * 1024, "stage"); - auto reply = CreateSegmentRefDirect(builder, 3, SegmentKind::Reply, 8ull * 1024 * 1024, "reply"); + auto reply = CreateSegmentRefDirect(builder, 3, SegmentKind::Reply, 16ull * 1024 * 1024, "reply"); auto event = CreateSegmentRefDirect(builder, 4, SegmentKind::Event, 256ull * 1024, "event"); auto welcome = CreateWelcome(builder, MOBILEGL_PROTOCOL_ABI_MAJOR, MOBILEGL_PROTOCOL_ABI_MINOR, /*serverPid=*/99, cmd, stage, reply, event); @@ -91,6 +95,11 @@ TEST(ProtocolSmokeTest, WelcomeCarriesTheFourSegmentAnnouncements) { EXPECT_EQ(parsed->cmdRing()->sizeBytes(), 8ull * 1024 * 1024); ASSERT_NE(parsed->stageRing(), nullptr); EXPECT_EQ(parsed->stageRing()->sizeBytes(), 32ull * 1024 * 1024); + // The pin never read the reply announcement back before ID-47; a size that moved on the + // wire and not here would have gone unnoticed. + ASSERT_NE(parsed->replyPool(), nullptr); + EXPECT_EQ(parsed->replyPool()->kind(), SegmentKind::Reply); + EXPECT_EQ(parsed->replyPool()->sizeBytes(), 16ull * 1024 * 1024); ASSERT_NE(parsed->eventRing(), nullptr); EXPECT_EQ(parsed->eventRing()->sizeBytes(), 256ull * 1024); } diff --git a/MobileGL/MG_Test/Wire/RemoteClientTest.cpp b/MobileGL/MG_Test/Wire/RemoteClientTest.cpp index 76f33738..1bd97a17 100644 --- a/MobileGL/MG_Test/Wire/RemoteClientTest.cpp +++ b/MobileGL/MG_Test/Wire/RemoteClientTest.cpp @@ -34,6 +34,7 @@ #include #include #include +#include #include #include @@ -407,32 +408,44 @@ TEST(RemoteEmitTable, TheE2DropSwitchStartsDisarmed) { // ===================================================================================== TEST(RemoteReadback, ExactlyTheCapacityPassesAndOneByteMoreIsRefusedByName) { - // THE BOUNDARY PAIR ID-47 ASKS FOR, driven through the emitter's own decision function. - // It is not driven through EmitReadPixels, and that is the point: EmitReadPixels needs a - // live session before it reaches any of this, so a control over the emitter could only - // ever observe Fatal{NoClientSession} and would be green for the wrong reason. - constexpr Uint64 kCap = 2u * 1024u * 1024u - 16u; // ID-47's post-growth MaxReplyBytes + // ID-47's boundary pair, and it is THE CALL SITE'S half. The refusal itself is s1's + // (ReplySlotPool::RequireReadPixelsFits, its own cases in s1-v3.md §1); what c1 owns is + // that the number handed to it is the one the emitter computes - ID-49's TIGHT extent - + // and that exactly the cap is legal while one byte more is not. So this drives the real + // pool with the real helper, over the real arithmetic, and never restates the message. + // + // A real pool over a real mapping, because CanHold answers false for a null base and a + // control built on a default-constructed pool would "refuse" everything for that reason. + constexpr std::uint32_t kSlots = 8; + constexpr std::uint64_t kSlotBytes = 2u * 1024u * 1024u; + std::vector backing(static_cast(kSlots) * kSlotBytes); + Transport::ReplySlotPool pool(backing.data(), backing.size(), kSlots); + const std::uint64_t cap = pool.MaxReplyBytes(); + ASSERT_GT(cap, 0u) << "the fixture's pool is not configured, so every answer would be refused"; - // The passing half runs IN THIS PROCESS, because "it did not abort" is only a statement if - // the thing that would have aborted is the same code. - RefuseReadbackLargerThanTheReplySlot(724, 724, 0x1908 /*GL_RGBA*/, kCap, kCap); + // ID-47's own number: the E2 retrace snapshot reads 640x480 RGBA8 and it must now FIT. + EXPECT_TRUE(pool.CanHold(TightReadbackByteCount(640, 480, 0x1908, 0x1401))) + << "the read ID-47 grew SEG_REPLY for still does not fit"; + + pool.RequireReadPixelsFits(724, 724, 0x1908, 0x1401, cap); SUCCEED() << "exactly the capacity is not an overflow"; #if MGTEST_HAVE_FORK const ChildResult child = RunInChild([&] { - RefuseReadbackLargerThanTheReplySlot(640, 480, 0x1908 /*GL_RGBA*/, kCap + 1, kCap); + Transport::ReplySlotPool inner(backing.data(), backing.size(), kSlots); + inner.RequireReadPixelsFits(640, 480, 0x1908, 0x1401, inner.MaxReplyBytes() + 1); }); EXPECT_TRUE(DiedOfAbort(child)) << "one byte over the slot did not abort: " << DescribeStatus(child); // ITS OWN FAILURE STRING, AND THE READ'S OWN NUMBERS. A control that only asserted // "something died" would pass on Fatal{NoClientSession}, Fatal{UnmigratedVerb} or a - // segfault, and this file has three other cases that abort for those reasons. + // segfault, and this file has four other cases that abort for those reasons. EXPECT_NE(child.Log.find("Fatal{ReplyTooLarge"), std::string::npos) << child.Log; EXPECT_NE(child.Log.find("ReadPixels 640x480"), std::string::npos) << "the message does not name the read, so an operator cannot tell which one: " << child.Log; - EXPECT_NE(child.Log.find(std::to_string(kCap + 1)), std::string::npos) + EXPECT_NE(child.Log.find(std::to_string(cap + 1)), std::string::npos) << "the message does not carry the byte count"; #else - GTEST_SKIP() << "the refusal reports through MGLOG_F + abort and needs fork() to read back"; + GTEST_SKIP() << "the refusal reports through a Fatal + abort and needs fork() to read back"; #endif } diff --git a/MobileGL/MG_Test/Wire/RingTest.cpp b/MobileGL/MG_Test/Wire/RingTest.cpp index 56d8e0c7..8ffa604e 100644 --- a/MobileGL/MG_Test/Wire/RingTest.cpp +++ b/MobileGL/MG_Test/Wire/RingTest.cpp @@ -702,9 +702,11 @@ TEST(RingTest, TheTwoFlagSpacesAreDisjointByTranslation) { 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. + // and the middle one was the worst: kRecPad is the flag RingConsumer::Pop reads as "wrap + // filler", so on the flag alone a stamped kVarTail would delete every variable-tail call + // from the stream with no error raised anywhere. Pop does not read it on the flag alone - + // it requires kind == kRingPadRecordKind beside it - and the runtime half below is the + // control on exactly that. 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"); @@ -757,8 +759,17 @@ TEST(RingTest, TheTwoFlagSpacesAreDisjointByTranslation) { // 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. + // mask is not in the path and Pop sees kRecPad on a record that is not a filler. + // + // What saves it is the KIND. RingConsumer::Pop skips a record only when it carries kRecPad + // AND kind == kRingPadRecordKind (Ring.cpp: `(header.flags & kRecPad) != 0 && header.kind == + // kRingPadRecordKind` - "BOTH, not just the flag"). Kind 0 is the wrap filler's and nothing + // else's, because the call catalogue starts at 1. So the stamped record is DELIVERED, lies + // and all, and the decoder can reject it by name - which it can only do because it got it. + // THE ASSERT AND THE EXPECTS BELOW ARE THE CONTROL ON THAT KIND CHECK: delete the + // `&& header.kind == kRingPadRecordKind` half of Pop's condition and this record vanishes + // into the wrap-filler skip again, exactly as it did before the pair was required, and this + // case goes red on the ASSERT's own message. That perturbation was run. void* second = ring.Producer().Reserve(static_cast(MGPWireOp::DrawVbo), kRecNone, 16); ASSERT_NE(second, nullptr); @@ -771,10 +782,51 @@ TEST(RingTest, TheTwoFlagSpacesAreDisjointByTranslation) { sizeof(stamped)); ring.Producer().Publish(); + ASSERT_TRUE(ring.Consumer().Pop(view)) + << "the stamped record vanished into Pop's wrap-filler skip. Pop's kind check - a filler " + "must carry kind == kRingPadRecordKind as well as kRecPad - is the only thing standing " + "between a header stamped with the CALL flags and every var-tail call being deleted " + "from the stream with nothing logged on either side"; + EXPECT_EQ(view.kind, static_cast(MGPWireOp::DrawVbo)) + << "kind is what Pop tells a real record from a filler by, and a filler's is " + "kRingPadRecordKind"; + + // Delivered is not the same as correct. The stamped bits arrive verbatim, and they are + // exactly the collisions the table above names: bit 2 (kVarTail -> kRecPad) is why this + // record looked like a filler at all, and bit 3 (kHostSpan -> kRecBorrowSlot) still makes it + // claim a slot in the GPU timeline it never borrowed. draw_vbo is not a kReplySlot call, so + // bit 4 (kReplySlot -> kRecVarTail) is clear here; on a blocking call it would lie too. + EXPECT_EQ(view.flags, static_cast(drawVboCallFlags)) + << "the header did not arrive as it was stamped"; + EXPECT_NE(view.flags & static_cast(kRecPad), 0u) + << "the kVarTail/kRecPad collision arrives intact - the kind check narrows the SKIP, it " + "does not scrub the bit, and naming this record is the decoder's job"; + 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_EQ(view.flags & static_cast(kRecVarTail), 0u) + << "draw_vbo started carrying kReplySlot; then the third collision lies here too"; + EXPECT_EQ(view.payloadSize, 16u); + EXPECT_TRUE(ring.Invariants()); + + // The other side of the same control, so that "the record is popped" cannot be satisfied by + // simply not skipping anything: kRecPad on a header whose kind IS kRingPadRecordKind is a + // genuine wrap filler and still vanishes. Pop's check was narrowed to the pair, not removed. + void* filler = ring.Producer().Reserve(kRingPadRecordKind, kRecNone, 16); + ASSERT_NE(filler, nullptr); + std::memset(filler, 0xEF, 16); + RingRecordHeader asFiller{}; + std::memcpy(&asFiller, static_cast(filler) - sizeof(RingRecordHeader), + sizeof(asFiller)); + asFiller.flags = static_cast(kRecPad); + std::memcpy(static_cast(filler) - sizeof(RingRecordHeader), &asFiller, + sizeof(asFiller)); + 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"; + << "a header carrying BOTH kRecPad and kind kRingPadRecordKind is a wrap filler and has " + "to be skipped; if it reaches a caller the skip is gone, not narrowed"; + EXPECT_TRUE(ring.Invariants()); // And the same record framed the way the encoder actually frames it - translated, with the // ring's own var-tail bit - round-trips intact. diff --git a/MobileGL/MG_Test/Wire/SessionHandshakeTest.cpp b/MobileGL/MG_Test/Wire/SessionHandshakeTest.cpp new file mode 100644 index 00000000..f79417be --- /dev/null +++ b/MobileGL/MG_Test/Wire/SessionHandshakeTest.cpp @@ -0,0 +1,292 @@ +// MobileGL - MobileGL/MG_Test/Wire/SessionHandshakeTest.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 + +// The two handshakes, driven for real (P5 s1, wave 1.5 - ID-46 findings 6 and 7). +// +// WHY THIS SUITE EXISTS BESIDE SessionTest. SessionTest is the ring-owning suite and keeps the +// GL frontend's umbrella header out on purpose. Both of the wave-1 review's findings against it +// were the same defect seen twice: a case that observed a property of the thing it built itself +// - a null-union frame it never sent anywhere, a mixer production never called - and so could +// not go red when the production code it was named for was deleted. The cure for both is to +// start from the production entry point, and the production entry points (ServerSession::Accept, +// ClientSession::StartOverTransportPair, CapsAbiFingerprint) all reach Includes.h. So they are +// exercised here, in a target that carries the include paths and links gtest rather than +// gtest_main: each guard's refusal is asserted BY MESSAGE, and with the console sink compiled +// out (Defines.h) an MGLOG line reaches exactly one place, the log file this process names +// before anything logs - PipeWireCodecTest's main() shape. +// +// EVERY CASE BELOW CARRIES ITS RED-ONCE LINE, and each of those perturbations was run. + +#include + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +#include "Includes.h" + +#include +#include +#include +#include +#include +#include + +#if __has_include() +#include +#define MGL_HANDSHAKE_TEST_HAS_GIT_HASH 1 +#else +#define MGL_HANDSHAKE_TEST_HAS_GIT_HASH 0 +#endif + +#if defined(_WIN32) +#include +#else +#include +#endif + +using namespace MobileGL; +using namespace MobileGL::MG_Remote; +namespace Transport = MobileGL::MG_Remote::Transport; + +namespace { + + std::string g_logPath; + + std::string ReadLog() { + std::ifstream in(g_logPath, std::ios::binary); + std::ostringstream ss; + ss << in.rdbuf(); + return ss.str(); + } + + long ProcessId() { +#if defined(_WIN32) + return static_cast(::_getpid()); +#else + return static_cast(::getpid()); +#endif + } + + bool Contains(const std::string& haystack, const char* needle) { + return haystack.find(needle) != std::string::npos; + } + + // The reviewer's 24-byte shape (SessionTest.ANullUnionFrameVerifiesWhichIsThePremiseOf- + // BothHandshakeGuards proves it verifies): the envelope's tag says `tag` and its union + // member is NULL, because FlatBuffers' Verifier::VerifyTable is `return !table || + // table->Verify(*this)`. + std::vector BuildNullUnionFrame(::MobileGL::Wire::CtrlMsg tag) { + ::flatbuffers::FlatBufferBuilder builder(256); + auto envelope = + ::MobileGL::Wire::CreateCtrlEnvelope(builder, tag, ::flatbuffers::Offset()); + ::MobileGL::Wire::FinishCtrlEnvelopeBuffer(builder, envelope); + const Uint8* begin = builder.GetBufferPointer(); + return std::vector(begin, begin + builder.GetSize()); + } + +} // namespace + +// --------------------------------------------------------------------------- +// The ABI fingerprint, from the production entry point (ID-46 finding 6) +// --------------------------------------------------------------------------- + +// The value Hello and Welcome carry is CapsAbiFingerprint(). This case starts THERE, requires it +// to be the mixer over its own published inputs, pins those inputs to the real sizeofs and +// constants (ID-33's list: the three struct sizes, kOpCount, the protocol ABI version, the git +// stamp - plus the caps blob's extents and the two codec versions), and then perturbs every +// input by one through the same mixer and requires the answer to move. RED ONCE by replacing +// CapsAbiFingerprint()'s body with `return 1;` - the verifier's exact perturbation, which left +// the whole unit lane green before this case existed - and it fails on the first EXPECT_EQ +// below. Also red, separately, by deleting any one `mix(...)` line from MixAbiFingerprint: the +// matching EXPECT_NE names the input that stopped being mixed. Both perturbations were run. +TEST(SessionHandshakeTest, TheAbiFingerprintChangesWhenAnyOfItsInputsDoes) { + const Uint64 production = CapsAbiFingerprint(); + EXPECT_NE(production, 0u) << "0 is reserved for \"not stated\""; + EXPECT_EQ(production, CapsAbiFingerprint()) << "not stable within one build"; + + const Transport::AbiFingerprintInputs inputs = CapsAbiFingerprintInputs(); + EXPECT_EQ(production, Transport::MixAbiFingerprint(inputs)) + << "CapsAbiFingerprint() is not MixAbiFingerprint over CapsAbiFingerprintInputs(): the " + "handshake compares a value this case cannot reach, which is finding 6 again"; + + // The inputs are the real ones, so a CapsAbiFingerprintInputs() that hard-coded a size + // would be caught here rather than agreed with by a peer built from a different tree. + EXPECT_EQ(inputs.DynamicParamsSize, sizeof(MG_Backend::DynamicBackendParameters)); + EXPECT_EQ(inputs.CapsSize, sizeof(MG_Pipe::MGPCaps)); + EXPECT_EQ(inputs.FunctionTableSize, sizeof(MG_Backend::GLFunctionsTable)); + EXPECT_EQ(inputs.FormatCapabilityTargets, + static_cast(MG_Backend::kFormatCapabilityTargetCount)); + EXPECT_EQ(inputs.FormatCapabilityFormats, + static_cast(MG_Backend::kFormatCapabilityFormatCount)); + EXPECT_NE(inputs.FormatCapabilitiesCodecVersion, 0u); + EXPECT_NE(inputs.RendererInfoCodecVersion, 0u); + EXPECT_EQ(inputs.OpCount, static_cast(MG_Pipe::MGPWireOp::kOpCount)); + EXPECT_EQ(inputs.AbiVersion, static_cast(MOBILEGL_ABI_VERSION( + MOBILEGL_PROTOCOL_ABI_MAJOR, MOBILEGL_PROTOCOL_ABI_MINOR))); + ASSERT_NE(inputs.BuildStamp, nullptr); + EXPECT_NE(inputs.BuildStamp[0], '\0'); +#if MGL_HANDSHAKE_TEST_HAS_GIT_HASH + EXPECT_STREQ(inputs.BuildStamp, GIT_COMMIT_HASH_SHORT); +#endif + + // Every input moves the answer. Each lambda changes exactly one field of a copy of the + // REAL inputs, so what is proven is that the production value depends on that field. + const auto perturbed = [&](auto&& mutate) { + Transport::AbiFingerprintInputs copy = inputs; + mutate(copy); + return Transport::MixAbiFingerprint(copy); + }; + EXPECT_NE(production, perturbed([](Transport::AbiFingerprintInputs& i) { ++i.DynamicParamsSize; })) + << "sizeof(DynamicBackendParameters) is not mixed"; + EXPECT_NE(production, perturbed([](Transport::AbiFingerprintInputs& i) { ++i.CapsSize; })) + << "sizeof(MGPCaps) is not mixed"; + EXPECT_NE(production, perturbed([](Transport::AbiFingerprintInputs& i) { ++i.FunctionTableSize; })) + << "sizeof(GLFunctionsTable) is not mixed"; + EXPECT_NE(production, + perturbed([](Transport::AbiFingerprintInputs& i) { ++i.FormatCapabilityTargets; })) + << "kFormatCapabilityTargetCount is not mixed"; + EXPECT_NE(production, + perturbed([](Transport::AbiFingerprintInputs& i) { ++i.FormatCapabilityFormats; })) + << "kFormatCapabilityFormatCount is not mixed"; + EXPECT_NE(production, perturbed([](Transport::AbiFingerprintInputs& i) { + ++i.FormatCapabilitiesCodecVersion; + })) + << "kFormatCapabilitiesCodecVersion is not mixed"; + EXPECT_NE(production, + perturbed([](Transport::AbiFingerprintInputs& i) { ++i.RendererInfoCodecVersion; })) + << "kRendererInfoCodecVersion is not mixed"; + EXPECT_NE(production, perturbed([](Transport::AbiFingerprintInputs& i) { ++i.OpCount; })) + << "MGPWireOp::kOpCount is not mixed (ID-33)"; + EXPECT_NE(production, perturbed([](Transport::AbiFingerprintInputs& i) { ++i.AbiVersion; })) + << "the protocol ABI version is not mixed (ID-33)"; + EXPECT_NE(production, + perturbed([](Transport::AbiFingerprintInputs& i) { i.BuildStamp = "not-this-build"; })) + << "the git stamp is not mixed"; + // A missing stamp is not the same as an empty one, and neither is the same as a real build. + EXPECT_NE(production, perturbed([](Transport::AbiFingerprintInputs& i) { i.BuildStamp = nullptr; })); + EXPECT_NE(production, perturbed([](Transport::AbiFingerprintInputs& i) { i.BuildStamp = ""; })); + EXPECT_NE(perturbed([](Transport::AbiFingerprintInputs& i) { i.BuildStamp = nullptr; }), + perturbed([](Transport::AbiFingerprintInputs& i) { i.BuildStamp = ""; })) + << "\"no stamp\" and \"an empty stamp\" collapsed into one input"; +} + +// --------------------------------------------------------------------------- +// The two null-union guards, driven THROUGH the handshakes (ID-46 finding 7) +// --------------------------------------------------------------------------- + +// A frame whose tag says Hello and whose Hello is NULL, sent on a real InProcessTransport pair to +// a real ServerSession::Accept - a session of this case's own, not the process singleton. Accept +// must answer MOBILEGL_ERR_PROTOCOL_MISMATCH with its guard's own line, and must not have +// dereferenced the member: nothing Fatal in the log, nothing accepted. RED ONCE by deleting +// `envelope->msg_as_Hello() == nullptr` from the guard in ServerSession.cpp: the tag check +// passes, `hello` is nullptr, and `hello->buildFingerprint()` reads address 0 - the case dies +// instead of returning. That perturbation was run. +TEST(SessionHandshakeTest, ANullUnionHelloIsRefusedByAcceptRatherThanDereferenced) { + std::unique_ptr client; + std::unique_ptr server; + Transport::InProcessTransport::CreatePair(client, server); + ASSERT_NE(client, nullptr); + ASSERT_NE(server, nullptr); + + const std::vector frame = BuildNullUnionFrame(::MobileGL::Wire::CtrlMsg::Hello); + ASSERT_EQ(client->SendFrame(MobileGLByteSpan{frame.data(), frame.size()}), MOBILEGL_OK); + + Server::ServerSession session; + const std::string before = ReadLog(); + EXPECT_EQ(session.Accept(*server), MOBILEGL_ERR_PROTOCOL_MISMATCH) + << "a null-union Hello was not refused by the handshake"; + EXPECT_FALSE(session.Accepted()); + const std::string delta = ReadLog().substr(before.size()); + EXPECT_TRUE(Contains(delta, "MG_Remote server: the first control frame is not a verifiable Hello")) + << "Accept refused, but not with the guard's own line. Log delta:\n" + << delta; + EXPECT_FALSE(Contains(delta, "Fatal{")) << "the refusal became a Fatal. Log delta:\n" << delta; + session.Close(); +} + +// The Welcome guard. ClientSession::Start builds its transport pair itself, so nothing could put +// a frame on the server->client direction ahead of the server's Welcome - which is why +// StartOverTransportPair, Start's second half, is public (ClientSession.h). The frame is queued +// there BEFORE Start sends Hello: the server's Accept then runs for real (segments, Welcome, +// resolver), its genuine Welcome queues behind the null-union one, the client reads the +// null-union one first and must refuse it with the guard's own line, and Stop()'s not-started +// path must have closed the server the handshake had accepted. RED ONCE by deleting +// `envelope->msg_as_Welcome() == nullptr` from the guard in ClientSession.cpp: `welcome` is then +// nullptr and `welcome->buildFingerprint()` reads address 0 - the case dies. That perturbation +// was run. +TEST(SessionHandshakeTest, ANullUnionWelcomeIsRefusedByStartRatherThanDereferenced) { + std::unique_ptr client; + std::unique_ptr server; + Transport::InProcessTransport::CreatePair(client, server); + ASSERT_NE(client, nullptr); + ASSERT_NE(server, nullptr); + + const std::vector frame = BuildNullUnionFrame(::MobileGL::Wire::CtrlMsg::Welcome); + ASSERT_EQ(server->SendFrame(MobileGLByteSpan{frame.data(), frame.size()}), MOBILEGL_OK); + + Client::ClientSession& session = Client::ClientSessionInstance(); + Server::ServerSession& serverSession = Server::ServerSessionInstance(); + ASSERT_FALSE(session.Started()); + ASSERT_FALSE(serverSession.Accepted()); + + const std::string before = ReadLog(); + EXPECT_EQ(session.StartOverTransportPair(std::move(client), std::move(server)), + MOBILEGL_ERR_PROTOCOL_MISMATCH) + << "a null-union Welcome was not refused by the handshake"; + EXPECT_FALSE(session.Started()); + EXPECT_EQ(Client::ClientSession::Active(), nullptr); + // Stop()'s not-started path closes the server FIRST (ClientSession.cpp); a server left + // m_accepted would refuse every later Start in this process. + EXPECT_FALSE(serverSession.Accepted()); + EXPECT_EQ(Server::ServerSession::Active(), nullptr); + + const std::string delta = ReadLog().substr(before.size()); + EXPECT_TRUE(Contains(delta, + "MG_Remote client: the server's first control frame is not a verifiable " + "Welcome")) + << "Start refused, but not with the guard's own line. Log delta:\n" + << delta; + EXPECT_FALSE(Contains(delta, "Fatal{AbiMismatch")) + << "the null-union Welcome reached the fingerprint compare. Log delta:\n" + << delta; + // And the server's half of the handshake DID run - the Hello it received was this + // client's real one - so the case drove Start past the point a stub would stop at. + EXPECT_TRUE(Contains(delta, "MG_Remote server: accepted with NO backend")) + << "ServerSession::Accept never ran, so the Welcome guard was not reached the way " + "Start reaches it. Log delta:\n" + << delta; +} + +int main(int argc, char** argv) { + // Before anything logs: MG_Util::Debug::InitFile() reads the variable once, on the first + // write, and caches the FILE*. The name carries this process's pid, because + // gtest_discover_tests runs every case as its own process, in parallel under ctest -j. + namespace fs = std::filesystem; + const fs::path path = fs::temp_directory_path() / + ("mobilegl-sessionhandshake-test-" + std::to_string(ProcessId()) + ".log"); + std::error_code ec; + fs::remove(path, ec); + g_logPath = path.string(); +#if defined(_WIN32) + _putenv_s("MOBILEGL_LOG_FILE_PATH", g_logPath.c_str()); +#else + setenv("MOBILEGL_LOG_FILE_PATH", g_logPath.c_str(), 1); +#endif + ::testing::InitGoogleTest(&argc, argv); + const int rc = RUN_ALL_TESTS(); + fs::remove(path, ec); + return rc; +} diff --git a/MobileGL/MG_Test/Wire/SessionTest.cpp b/MobileGL/MG_Test/Wire/SessionTest.cpp index e294d892..2bf12783 100644 --- a/MobileGL/MG_Test/Wire/SessionTest.cpp +++ b/MobileGL/MG_Test/Wire/SessionTest.cpp @@ -198,7 +198,8 @@ TEST(SessionTest, TheDefaultGeometryIsTheFourContractRingSizes) { ASSERT_EQ(segments.Create(SessionSegmentSizes{}, MemoryRole::Server), MOBILEGL_OK); EXPECT_EQ(segments.CmdRingCapacity(), 8ull * 1024 * 1024); EXPECT_EQ(segments.StageBytes(), 32ull * 1024 * 1024); - EXPECT_EQ(segments.ReplyBytes(), 8ull * 1024 * 1024); + // ID-47: 16 MiB, eight slots of 2 MiB. ProtocolSmokeTest pins the same number on the wire. + EXPECT_EQ(segments.ReplyBytes(), 16ull * 1024 * 1024); EXPECT_EQ(segments.EventRingCapacity(), 256ull * 1024); EXPECT_EQ(segments.AnnouncedSize(SessionSegmentSlot::Cmd), @@ -206,12 +207,60 @@ TEST(SessionTest, TheDefaultGeometryIsTheFourContractRingSizes) { // SEG_STAGE is not a ring at all - no control page, no cursor triple, no power-of-two // rounding - and neither is SEG_REPLY, so both announce exactly what was asked for. EXPECT_EQ(segments.AnnouncedSize(SessionSegmentSlot::Stage), 32ull * 1024 * 1024); - EXPECT_EQ(segments.AnnouncedSize(SessionSegmentSlot::Reply), 8ull * 1024 * 1024); + EXPECT_EQ(segments.AnnouncedSize(SessionSegmentSlot::Reply), 16ull * 1024 * 1024); EXPECT_EQ(segments.AnnouncedSize(SessionSegmentSlot::Event), 256ull * 1024 + sizeof(RingControl)); segments.Close(); } +// ID-47 (ID-46 finding 2). The DEFAULT reply pool must hold the largest P5 read: the E2 retrace +// harness snapshots OpenRA's whole 640x480 surface as GL_RGBA/GL_UNSIGNED_BYTE through the +// interposer, 1,228,800 bytes, and the previous geometry (8 MiB / 8 slots, 1 MiB minus a 16-byte +// header) refused it by 180,240 bytes - and refused the 512x512 RGBA8 read s1-v1.md:134 claimed +// it covered, by sixteen. This is the verifier's case, committed: it posts exactly 640*480*4 +// bytes into a pool built from SessionSegmentSizes{} and reads them back. RED ONCE by reverting +// SessionSegmentSizes::ReplyBytes to 8 MiB: Post then takes its oversize-abort branch and the +// case dies, which is the verifier's original outcome. That perturbation was run. +TEST(SessionTest, TheDefaultReplyGeometryHoldsTheLargestP5Read) { + SessionSegments segments; + ASSERT_EQ(segments.Create(SessionSegmentSizes{}, MemoryRole::Server), MOBILEGL_OK); + ReplySlotPool pool(segments.ReplyBase(), segments.ReplyBytes(), segments.ReplySlotCount()); + ASSERT_TRUE(pool.Valid()); + EXPECT_EQ(pool.SlotCount(), 8u); + EXPECT_EQ(pool.SlotBytes(), 2u * 1024 * 1024); + EXPECT_EQ(pool.MaxReplyBytes(), 2u * 1024 * 1024 - 16u); + + const std::uint64_t kOpenRaSnapshot = 640ull * 480 * 4; // E2's read, the largest in P5 + const std::uint64_t kHalfKSquare = 512ull * 512 * 4; // the read s1-v1.md:134 got wrong + EXPECT_TRUE(pool.CanHold(kOpenRaSnapshot)); + EXPECT_TRUE(pool.CanHold(kHalfKSquare)); + + std::vector answer(static_cast(kOpenRaSnapshot)); + for (std::size_t i = 0; i < answer.size(); ++i) { + answer[i] = static_cast(i * 7 + (i >> 12)); + } + pool.Post(1, kReplyStatusOk, answer.data(), answer.size()); + + std::vector back(answer.size(), 0); + std::int32_t status = -1; + std::uint64_t size = 0; + ASSERT_TRUE(pool.Read(1, back.data(), back.size(), &status, &size)) + << "the E2 snapshot does not fit the default reply pool"; + EXPECT_EQ(status, kReplyStatusOk); + EXPECT_EQ(size, kOpenRaSnapshot); + EXPECT_EQ(std::memcmp(back.data(), answer.data(), answer.size()), 0); + + // And the 512x512 read, in the next slot, so a geometry that only just clears 640x480 by + // some accident of rounding cannot pass this case either. + answer.resize(static_cast(kHalfKSquare)); + pool.Post(2, kReplyStatusOk, answer.data(), answer.size()); + back.assign(answer.size(), 0); + ASSERT_TRUE(pool.Read(2, back.data(), back.size(), &status, &size)); + EXPECT_EQ(size, kHalfKSquare); + EXPECT_EQ(std::memcmp(back.data(), answer.data(), answer.size()), 0); + segments.Close(); +} + // The ledger is per role and DOES double-count under inproc, deliberately: the two roles map // the same pages here and will not under spawn, so the per-role numbers are what t1 subtracts // with and a silently deduplicated total would hide exactly that difference. @@ -229,9 +278,12 @@ TEST(SessionTest, TheMemoryLedgerIsPerRoleAndIsReleasedOnClose) { const RoleMemorySample sample = SampleRoleMemory(MemoryRole::Client); EXPECT_EQ(sample.MappedSegmentBytes, clientBefore + mapped); #if defined(__linux__) || defined(__ANDROID__) - // VmHWM is the PROCESS's high-water mark, so it is the same number for both roles and - // is only meaningful beside the ledger - which is why RoleMemorySample carries both. + // The peak is the PROCESS's, so it is the same number for both roles and is only + // meaningful beside the ledger - which is why RoleMemorySample carries both. EXPECT_GT(sample.PeakRssBytes, 0u); + // Holds BY CONSTRUCTION now (the ledger's own running max, RoleMemory.h), not by + // the kernel's grace: GitHub run 35079459114 failed exactly this line with VmHWM + // 4,784,128 < VmRSS 4,849,664. The control on the construction is the next case. EXPECT_GE(sample.PeakRssBytes, sample.CurrentRssBytes); #endif } @@ -239,6 +291,50 @@ TEST(SessionTest, TheMemoryLedgerIsPerRoleAndIsReleasedOnClose) { EXPECT_EQ(LedgerMappedBytes(MemoryRole::Server), serverBefore); } +// The kernel's VmHWM is NOT a monotone bound on the kernel's VmRSS at read time: hiwater_rss +// is stored only when RSS is about to drop, task_mem() reports max(stored, rss-now), and the +// wave-1 sampler read the two keys in two passes, so the second fopen could grow RSS past the +// peak the first pass reported. GitHub run 35079459114 (ubuntu-24.04) caught it; the probe under +// ~/w7/p5-s1-probe reproduced it locally. The rule is therefore that the ledger keeps ITS OWN +// running peak and folds the kernel's two numbers into it, so a stubbed reader whose current +// exceeds its peak must not fail. RED ONCE by reverting SampleRoleMemoryInto to +// `sample.PeakRssBytes = kernelPeakRssBytes;` - the first EXPECT_EQ below then reads 100 against +// 200 and the EXPECT_GE beside it is the CI line again. That perturbation was run. +TEST(SessionTest, TheLedgersPeakIsItsOwnRunningMaxAndNeverTheKernelsHighWaterMarkVerbatim) { + std::atomic runningPeak{0}; + + // The CI shape: the kernel says peak 100, current 200. + RoleMemorySample sample = SampleRoleMemoryInto(runningPeak, MemoryRole::Client, 100, 200); + EXPECT_EQ(sample.PeakRssBytes, 200u) + << "the kernel's peak was reported verbatim although its current exceeded it"; + EXPECT_GE(sample.PeakRssBytes, sample.CurrentRssBytes); + EXPECT_EQ(sample.CurrentRssBytes, 200u); + EXPECT_EQ(runningPeak.load(), 200u); + + // A later, smaller sample does not lower it: a running max never decreases. + sample = SampleRoleMemoryInto(runningPeak, MemoryRole::Server, 150, 120); + EXPECT_EQ(sample.PeakRssBytes, 200u); + EXPECT_EQ(sample.CurrentRssBytes, 120u); + EXPECT_EQ(sample.Role, MemoryRole::Server); + + // The kernel's peak still counts when it IS the larger number: it is a lower bound on + // the true peak that this process's own samples may have missed. + sample = SampleRoleMemoryInto(runningPeak, MemoryRole::Client, 300, 100); + EXPECT_EQ(sample.PeakRssBytes, 300u); + EXPECT_EQ(runningPeak.load(), 300u); + + // And the production sampler reads BOTH keys in ONE pass, so the pair it folds is one + // snapshot: on Linux neither number is 0 and the pair is self-consistent. +#if defined(__linux__) || defined(__ANDROID__) + std::uint64_t kernelPeak = 0; + std::uint64_t kernelCurrent = 0; + ProcessRssBytes(&kernelPeak, &kernelCurrent); + EXPECT_GT(kernelPeak, 0u); + EXPECT_GT(kernelCurrent, 0u); + EXPECT_GE(kernelPeak, kernelCurrent) << "one pass over /proc/self/status disagreed with itself"; +#endif +} + // --------------------------------------------------------------------------- // Two real threads, twenty thousand records // --------------------------------------------------------------------------- @@ -571,14 +667,72 @@ TEST(SessionTest, DeclinedIsARealAnswerWithNoPayload) { #if defined(GTEST_HAS_DEATH_TEST) && GTEST_HAS_DEATH_TEST // A reply larger than a slot is FATAL, not chunked and not truncated: P5's only large answer is // a blocking ReadPixels whose size the client knows before it emits, so an overflow means the -// two sides disagree about the frame. A gate that cannot go red is not a gate. +// two sides disagree about the frame. A gate that cannot go red is not a gate - and a death +// control with an empty regex is not a gate either (ID-46 finding 10: a bare std::abort(), or a +// segfault in a broken refusal path, satisfied the previous `""`). The regex below is the +// diagnostic's own wording, which WireLogFatal echoes to stderr for exactly this reader. RED ONCE +// by replacing the oversize branch's WireLogFatal with a bare std::abort(): the process still +// dies, the regex finds nothing, and the case fails on "died but not with the expected error". +// That perturbation was run. The boundary is a pair: exactly MaxReplyBytes() posts and reads +// back, one more byte is the named refusal. TEST(SessionTestDeath, AReplyLargerThanItsSlotIsFatalRatherThanTruncated) { SessionSegments segments; ASSERT_EQ(segments.Create(TestSizes(), MemoryRole::Server), MOBILEGL_OK); ReplySlotPool pool(segments.ReplyBase(), segments.ReplyBytes(), segments.ReplySlotCount()); ASSERT_TRUE(pool.Valid()); - std::vector oversize(pool.SlotBytes() + 1, 0xAB); - EXPECT_DEATH(pool.Post(1, kReplyStatusOk, oversize.data(), oversize.size()), ""); + ASSERT_EQ(pool.SlotBytes(), 8192u); + ASSERT_EQ(pool.MaxReplyBytes(), 8176u); + + std::vector exact(pool.MaxReplyBytes(), 0xAB); + EXPECT_TRUE(pool.CanHold(exact.size())); + pool.Post(1, kReplyStatusOk, exact.data(), exact.size()); + std::vector back(exact.size(), 0); + std::uint64_t size = 0; + ASSERT_TRUE(pool.Read(1, back.data(), back.size(), nullptr, &size)); + EXPECT_EQ(size, exact.size()); + + std::vector oversize(pool.MaxReplyBytes() + 1, 0xAB); + EXPECT_FALSE(pool.CanHold(oversize.size())); + EXPECT_DEATH(pool.Post(2, kReplyStatusOk, oversize.data(), oversize.size()), + "reply pool: Fatal\\{ProtocolCorruption\\} - a 8177 byte answer for seq 2 does not " + "fit a 8192 byte slot \\(payload cap 8176\\)"); +} + +// ID-47's client half, on the DEFAULT geometry so the numbers are the ruling's: exactly +// MaxReplyBytes() = 2,097,136 passes, one more byte is refused BY NAME before emission, and the +// 1024x512 RGBA8 read - exactly 2 MiB, sixteen bytes over the cap, the same sixteen-byte shape +// that sank the previous geometry's 512x512 claim - is refused with its own dimensions in the +// line. The message is the contract's verbatim: `ReadPixels x > `. +// RED ONCE by replacing RequireReadPixelsFits's WireLogFatal with a bare std::abort() (the two +// death regexes then match nothing) and, separately, by making CanHold `<` instead of `<=` (the +// exact-cap call then dies). Both perturbations were run. +TEST(SessionTestDeath, AReadPixelsLargerThanAReplySlotIsRefusedAtTheClientByName) { + SessionSegments segments; + ASSERT_EQ(segments.Create(SessionSegmentSizes{}, MemoryRole::Server), MOBILEGL_OK); + ReplySlotPool pool(segments.ReplyBase(), segments.ReplyBytes(), segments.ReplySlotCount()); + ASSERT_TRUE(pool.Valid()); + const std::uint64_t cap = pool.MaxReplyBytes(); + ASSERT_EQ(cap, 2097136u); + constexpr std::uint32_t kGlRgba = 0x1908; + constexpr std::uint32_t kGlUnsignedByte = 0x1401; + + // Exactly the cap: 524,284 RGBA8 pixels in one row. Returns, no death. + ASSERT_EQ(524284ull * 1 * 4, cap); + EXPECT_TRUE(pool.CanHold(cap)); + pool.RequireReadPixelsFits(524284, 1, kGlRgba, kGlUnsignedByte, cap); + + // One more byte. + EXPECT_FALSE(pool.CanHold(cap + 1)); + EXPECT_DEATH(pool.RequireReadPixelsFits(524284, 1, kGlRgba, kGlUnsignedByte, cap + 1), + "Fatal\\{ReplyTooLarge, \"ReadPixels 524284x1 0x1908/0x1401 2097137 > 2097136\"\\}"); + + // The read a caller would actually make: 1024x512 RGBA8 = 2 MiB, 16 over. + EXPECT_DEATH(pool.RequireReadPixelsFits(1024, 512, kGlRgba, kGlUnsignedByte, 1024ull * 512 * 4), + "Fatal\\{ReplyTooLarge, \"ReadPixels 1024x512 0x1908/0x1401 2097152 > 2097136\"\\}"); + + // And E2's read, the reason for the geometry, is not refused. + pool.RequireReadPixelsFits(640, 480, kGlRgba, kGlUnsignedByte, 640ull * 480 * 4); + segments.Close(); } #endif @@ -766,27 +920,16 @@ TEST(SessionTest, ShutdownUnparksTheApplyThreadAndTheJoinIsBounded) { } // --------------------------------------------------------------------------- -// The ABI fingerprint's mixer +// The ABI fingerprint's mixer: MOVED to SessionHandshakeTest (ID-46 finding 6). +// +// The case that lived here drove MixAbiFingerprint with made-up sizes and never touched +// CapsAbiFingerprint(), the value the two handshakes actually compare - which had its own +// second FNV loop and no caller of the mixer at all, so `return 1;` in production left the +// whole unit lane green. The sensitivity case now starts from CapsAbiFingerprint(), which +// needs CapsCodec.h and therefore the GL frontend's umbrella header that this suite keeps +// out; it lives in SessionHandshakeTest.cpp beside the two handshake-guard controls. // --------------------------------------------------------------------------- -// A fingerprint that cannot be SHOWN to change is indistinguishable from one that is never -// compared, which is why the mixer takes its sizes as arguments instead of reading sizeof -// directly: a test can vary one byte and prove the answer moves. -TEST(SessionTest, TheAbiFingerprintChangesWhenAnyOfItsInputsDoes) { - const std::uint64_t base = MixAbiFingerprint(1024, 1080, 552, 0x00010000, "abc1234"); - EXPECT_NE(base, 0u) << "0 is reserved for \"not stated\""; - EXPECT_EQ(base, MixAbiFingerprint(1024, 1080, 552, 0x00010000, "abc1234")); - - EXPECT_NE(base, MixAbiFingerprint(1025, 1080, 552, 0x00010000, "abc1234")); - EXPECT_NE(base, MixAbiFingerprint(1024, 1081, 552, 0x00010000, "abc1234")); - EXPECT_NE(base, MixAbiFingerprint(1024, 1080, 553, 0x00010000, "abc1234")); - EXPECT_NE(base, MixAbiFingerprint(1024, 1080, 552, 0x00010001, "abc1234")); - EXPECT_NE(base, MixAbiFingerprint(1024, 1080, 552, 0x00010000, "abc1235")); - // A missing stamp is not the same as an empty one, and neither is the same as a real build. - EXPECT_NE(MixAbiFingerprint(1024, 1080, 552, 0x00010000, nullptr), - MixAbiFingerprint(1024, 1080, 552, 0x00010000, "abc1234")); -} - // --------------------------------------------------------------------------- // Fix round 1 - the cases the adversarial review's findings earned // --------------------------------------------------------------------------- @@ -796,13 +939,19 @@ TEST(SessionTest, TheAbiFingerprintChangesWhenAnyOfItsInputsDoes) { // appliedSeq is behind, every later advance is a no-op for ever and every WaitForApplied on // kWaitForever - the verb barrier and every reply wait - blocks permanently. A hang whose only // evidence is one ERROR line is not a diagnosis. +// +// The regex names the diagnostic (ID-46 finding 10; see the reply-pool death control for why an +// empty one was not a gate). RED ONCE by replacing AdvanceMonotonic's WireLogFatal block with a +// bare std::abort(): the case then fails on "died but not with the expected error". That +// perturbation was run. #if defined(GTEST_HAS_DEATH_TEST) && GTEST_HAS_DEATH_TEST TEST(SessionTestDeath, AWatermarkThatMovesBackwardsIsFatalRatherThanIgnored) { alignas(4096) RingControl control{}; InitRingControl(control); Watermark::AdvanceApplied(control, 10); ASSERT_EQ(control.appliedSeq.load(), 10u); - EXPECT_DEATH(Watermark::AdvanceApplied(control, 9), ""); + EXPECT_DEATH(Watermark::AdvanceApplied(control, 9), + "Fatal\\{ProtocolCorruption, \"watermark\"\\} appliedSeq moved backwards, 10 -> 9"); } #endif @@ -925,10 +1074,17 @@ TEST(SessionTest, TheProducerRemembersWhatItPublishedEvenIfTheWatermarkLags) { // M-3. FlatBuffers' Verifier::VerifyTable is `return !table || table->Verify(*this)`, so a NULL // union member PASSES verification: a 24-byte frame verifies, carries the file identifier, -// reports msg_type() == Hello, and returns nullptr from msg_as_Hello(). Both handshakes now -// fold that into their guard instead of dereferencing it. This case is the proof the shape is -// reachable at all, so the guard cannot be "simplified" away later. -TEST(SessionTest, AVerifiableFrameCanCarryANullUnionPayload) { +// reports msg_type() == Hello, and returns nullptr from msg_as_Hello(). Both handshakes fold +// that into their guard instead of dereferencing it. +// +// THIS CASE IS THE PREMISE, NOT THE CONTROL. It proves the shape is reachable - a FlatBuffers +// property - and nothing about MobileGL: the wave-1 review (ID-46 finding 7) deleted both +// `msg_as_*() == nullptr` clauses and this case stayed green, because it never calls Accept or +// Start. The controls on the two guards are SessionHandshakeTest's, which drive this exact +// frame THROUGH ServerSession::Accept and ClientSession::StartOverTransportPair and require +// each guard's own refusal line. s1-v1.md:338 claimed this case meant "the guard cannot be +// simplified away"; it did not, and s1-v3.md says so. +TEST(SessionTest, ANullUnionFrameVerifiesWhichIsThePremiseOfBothHandshakeGuards) { ::flatbuffers::FlatBufferBuilder builder(256); auto envelope = ::MobileGL::Wire::CreateCtrlEnvelope(builder, ::MobileGL::Wire::CtrlMsg::Hello, ::flatbuffers::Offset()); @@ -1028,26 +1184,69 @@ TEST(SessionTest, TheStageCursorTripleStaysDeadAcrossAWholeSession) { // have every var-tail record SKIPPED by Pop, silently, with the record lost and nothing logged on // either side. Pop now requires a filler to carry both the flag AND kind == kRingPadRecordKind, so // a real record wearing that bit is delivered instead of eaten. +// +// THE BIT HAS TO BE ON THE WIRE. The first version of this case asked Reserve(7, kRecPad, 16), +// and RingProducer::Reserve MASKS kRecPad OUT of whatever the caller passes (Ring.cpp: `flags & +// ~kRecPad`), so the header it stored had flags == 0, Pop's pad arm was never entered, and the +// case stayed green with the kind check deleted (ID-46 finding 5, executed by the verifier). So +// this producer writes the header ITSELF after Reserve, the way a codec with its own header +// struct does - MGPWireRecHeader and RingRecordHeader are the same eight bytes - which is the one +// path Reserve's mask cannot cover. RingTest.TheTwoFlagSpacesAreDisjointByTranslation (ID-43) is +// the same control at the ring; this one is at the SESSION, where what is pinned in addition is +// that appliedSeq counts the delivered record and does NOT count the genuine filler beside it. +// RED ONCE by deleting `&& header.kind == kRingPadRecordKind` from RingConsumer::Pop: the record +// vanishes into the wrap-filler skip, `seen` stays 0 and appliedSeq stays 0, and the case fails +// on the first EXPECT's message. That perturbation was run. TEST(SessionTest, ARecordWearingThePadBitIsDeliveredRatherThanEatenAsAFiller) { SessionFixture session; ASSERT_TRUE(session.Build(TestSizes())); // kRecPad is MGPipeCallFlags::kVarTail's bit. Kind 7 is a real opcode, not a filler. - void* payload = session.cmdProducer.Reserve(7, kRecPad, 16); + void* payload = session.cmdProducer.Reserve(7, kRecNone, 16); ASSERT_NE(payload, nullptr); std::memset(payload, 0xAB, 16); + // Stamp the bit past Reserve's mask, exactly where the codec's header struct would put it. + auto* headerBytes = static_cast(payload) - sizeof(RingRecordHeader); + RingRecordHeader stamped{}; + std::memcpy(&stamped, headerBytes, sizeof(stamped)); + ASSERT_EQ(stamped.kind, 7u); + ASSERT_EQ(stamped.flags & kRecPad, 0u) << "Reserve stopped masking kRecPad"; + stamped.flags = static_cast(stamped.flags | kRecPad); + std::memcpy(headerBytes, &stamped, sizeof(stamped)); session.producer.PublishAndNotify(1); int seen = 0; std::uint16_t seenKind = 0; + std::uint16_t seenFlags = 0; while (session.consumer.ApplyOne([&](const RingRecordView& view) { seenKind = view.kind; + seenFlags = view.flags; ++seen; })) { } - EXPECT_EQ(seen, 1) << "the record was skipped as a wrap filler because it wore bit 2"; + EXPECT_EQ(seen, 1) << "the record was skipped as a wrap filler because it wore bit 2 - Pop's " + "kind check (a filler is kRecPad AND kind == kRingPadRecordKind) is the " + "only thing between a stamped header and a silently deleted record"; EXPECT_EQ(seenKind, 7); + EXPECT_NE(seenFlags & kRecPad, 0u) + << "the bit arrives intact: the kind check narrows the SKIP, it does not scrub the bit"; EXPECT_EQ(session.Control().appliedSeq.load(), 1u); + + // The opposite control, so that "delivered" cannot be satisfied by not skipping anything: + // a header wearing kRecPad whose kind IS kRingPadRecordKind is a genuine wrap filler, still + // vanishes, and is NOT counted by appliedSeq (R-9: a filler does not advance seq). + void* filler = session.cmdProducer.Reserve(kRingPadRecordKind, kRecNone, 16); + ASSERT_NE(filler, nullptr); + headerBytes = static_cast(filler) - sizeof(RingRecordHeader); + std::memcpy(&stamped, headerBytes, sizeof(stamped)); + stamped.flags = static_cast(stamped.flags | kRecPad); + std::memcpy(headerBytes, &stamped, sizeof(stamped)); + session.producer.PublishAndNotify(2); + while (session.consumer.ApplyOne([&](const RingRecordView&) { ++seen; })) { + } + EXPECT_EQ(seen, 1) << "a genuine filler was delivered as a record: Pop's skip was removed, " + "not narrowed"; + EXPECT_EQ(session.Control().appliedSeq.load(), 1u) << "a filler advanced appliedSeq (R-9)"; } // The kHostSpan/kRecBorrowSlot collision, made loud. P5 implements no borrowed slots and is ruled diff --git a/scripts/ci/control_smoke_test.sh b/scripts/ci/control_smoke_test.sh new file mode 100755 index 00000000..d26fc2fc --- /dev/null +++ b/scripts/ci/control_smoke_test.sh @@ -0,0 +1,106 @@ +#!/bin/bash +# R-16 FOR THE CI NEGATIVE CONTROLS THEMSELVES: a control-run smoke test. +# +# BRIEF-P5 13 (R-16) says a negative control must assert its own failure reason and that every gate +# carries a line saying "I made it red once, by doing X". The two controls this file exercises ARE +# gates, and until ID-46 finding 8 nobody could make either of them red, because a workflow `run:` +# block only executes on a runner. The wave-1 verification agent had to hand-copy the blocks into +# throwaway harnesses to show they were broken (wave1-codex-verify.md 8). This file is that +# experiment, kept: it runs the REAL control scripts - the same files .github/workflows/test.yml +# invokes, not copies of them - against a stubbed ctest, and checks that each one passes exactly +# when it should. +# +# The case that matters is the first one. A stubbed ctest reports a NON-EMPTY selection and then +# fails with UNRELATED_CONTROL_FAILURE: a reason that has nothing to do with the knob the control +# turns. Before ID-48's fix both controls printed their success message and the step exited 0. They +# must now report FAILED. +# +# usage: control_smoke_test.sh +set -u + +HERE="$(cd "$(dirname "$0")" && pwd)" +WORK="$(mktemp -d)" +trap 'rm -rf "${WORK}"' EXIT + +STUB_DIR="${WORK}/stub" +mkdir -p "${STUB_DIR}" +cp "${HERE}/testdata/stub_ctest.sh" "${STUB_DIR}/ctest" +chmod +x "${STUB_DIR}/ctest" + +passes=0 +failures=0 + +# expect