diff --git a/MobileGL/MG_Remote/Client/PersistentMapTracker.cpp b/MobileGL/MG_Remote/Client/PersistentMapTracker.cpp index 5b4c6eb9..76925ed6 100644 --- a/MobileGL/MG_Remote/Client/PersistentMapTracker.cpp +++ b/MobileGL/MG_Remote/Client/PersistentMapTracker.cpp @@ -114,6 +114,119 @@ namespace MobileGL::MG_Remote::Client { // crashes honestly instead of refaulting forever). The page's own bit is // the discriminator - see RefaultOfANonWriteAccess. + // THE TWO ADDRESS SPACES THIS MODULE STRADDLES, AND THE ONE ROW WHERE THEY MEET. + // Bionic tags every heap pointer on a TBI-capable arm64 - POINTER_TAG 0xb4 in the + // top byte, turned on process-wide by PR_TAGGED_ADDR_ENABLE for every app built + // against a recent target SDK - so the shadow pointer MappedData() hands back, and + // with it every base/end this table publishes, carries that tag. THE KERNEL DOES + // NOT: a fault's si_addr is the untagged faulting virtual address. The tombstone of + // the crash that found this prints both forms of the one address on adjacent lines - + // "x0 b4000073ac9a5000" (the pointer the application was writing through) against + // "fault addr 0x00000073ac9a5000" (what the kernel put in si_addr). + // + // Comparing one space against the other makes `address < base` true for EVERY fault, + // so ownership test (2) never matched, every fault on a page this module had itself + // protected was chained to debuggerd, and the process died on the application's FIRST + // legitimate write through a persistent write map. It is invisible on x86-64, which + // has no top-byte tag, which is why every host lane stayed green while all three + // heavy-persistent-map traces died on the phone (device window #1, E0a/E1/E4a/E4b). + // + // So the table stores NORMALISED addresses and the handler normalises si_addr before + // it compares: one space, fixed at the two doors into this file. Protecting through a + // normalised base is exactly what protecting through the tagged one did - mprotect + // untags its own address argument (do_mprotect_pkey's untagged_addr) - and every + // other use of base/end here is either an mprotect or a difference against a shadow + // base normalised the same way, so nothing else has to change. Pointer DEREFERENCES + // keep the tagged pointer they came from (PushBlocksFor's two edge hashes), because + // under a future MTE tagging level the tag is load-bearing for the access itself. + // THE MASK IS NOT GUARDED BY THE ARCHITECTURE, deliberately, and that is what makes + // the defect reachable from a gate. Bits 56-63 of a 64-bit userspace address are a + // tag on arm64 and are ZERO everywhere else this builds: Linux/x86-64 caps + // TASK_SIZE_MAX below 2^56 even with five-level paging, so masking them off is the + // identity on the host and the fix on the phone. Written as one arch-independent row + // so a host unit case can drive it with the device's own two addresses - an + // `#if defined(__aarch64__)` here would have made the only gate that can catch this + // a gate that never runs in CI, which is how it got to the device in the first place. + constexpr uintptr_t UntagAddress(uintptr_t address) { + if constexpr (sizeof(uintptr_t) >= 8) { + return address & ((static_cast(1) << 56) - 1); + } else { + // 32-bit: there is no top byte to lose and the shift above would be UB. + return address; + } + } + + // Ownership test (2), as ONE row that the handler and the unit test both read, in + // the kernel's address space. `faultAddress` is si_addr exactly as delivered. + Bool SlotOwnsFault(const TrackedWriteMap& slot, uintptr_t faultAddress, SizeT& pageIndexOut) { + const uintptr_t base = UntagAddress(slot.base.load(std::memory_order_acquire)); + if (base <= kSlotSettingUp) return false; + const uintptr_t address = UntagAddress(faultAddress); + if (address < base) return false; + // The handler reads base first, so a base it can observe is one whose end is + // already visible - the publish order in TrackWriteMap is what makes that true. + const uintptr_t end = UntagAddress(slot.end.load(std::memory_order_acquire)); + if (address >= end) return false; + pageIndexOut = (address - base) >> kPageShift; + return true; + } + + // THE DECLINE RECORD. A fault this handler chains away is, for a page the tracker + // itself protected, a process kill with no evidence anywhere: debuggerd's tombstone + // names the application's memcpy and says nothing about who took the page's write + // permission away. So a decline leaves a record before it chains. Written with + // relaxed atomic stores and read back by DeclinedFaultReport() on the GL thread - + // every operation here is async-signal-safe, which rules out MGLOG (it formats and + // takes a lock) and leaves the raw write(2) below for the case where nothing on the + // GL thread ever runs again. + std::atomic g_declinedFaults{0}; + std::atomic g_declinedFaultAddress{0}; + std::atomic g_declinedFaultNearestBase{0}; + std::atomic g_declinedFaultNearestEnd{0}; + std::atomic g_declinedFaultAnnounced{false}; + + // Async-signal-safe hex, because snprintf is not on the safe list and this runs in a + // handler that is about to hand the process to debuggerd. + SizeT AppendHex(char* out, SizeT at, uintptr_t value) { + out[at++] = '0'; + out[at++] = 'x'; + Bool leading = true; + for (int shift = 60; shift >= 0; shift -= 4) { + const unsigned digit = static_cast((value >> shift) & 0xf); + if (digit == 0 && leading && shift != 0) continue; + leading = false; + out[at++] = static_cast(digit < 10 ? '0' + digit : 'a' + (digit - 10)); + } + return at; + } + + SizeT AppendText(char* out, SizeT at, const char* text) { + while (*text != '\0') out[at++] = *text++; + return at; + } + + // One line, once per process, straight to fd 2. It is the only channel a chained + // fault has: the next thing that happens is debuggerd's tombstone and then exit. + void AnnounceDeclinedFault(uintptr_t address, uintptr_t nearestBase, uintptr_t nearestEnd) { + Bool expected = false; + if (!g_declinedFaultAnnounced.compare_exchange_strong(expected, true)) return; + // 283 bytes at the longest (two full 16-digit addresses plus the two literals); + // the slack is deliberate, because this formats inside a signal handler where an + // overrun would be the second bug in the same crash. + char line[384]; + SizeT at = AppendText(line, 0, + "MGPipe: persistent-map tracker DECLINED a SEGV_ACCERR as foreign - si_addr="); + at = AppendHex(line, at, address); + at = AppendText(line, at, " nearest-tracked=["); + at = AppendHex(line, at, nearestBase); + at = AppendText(line, at, ","); + at = AppendHex(line, at, nearestEnd); + at = AppendText(line, at, + ") - if si_addr lies inside that span with the top byte masked off, the " + "table published a TAGGED base against an UNTAGGED fault address\n"); + (void)::write(2, line, at); + } + uintptr_t FaultPcFrom(void* ucontext) { #if defined(__aarch64__) return static_cast( @@ -225,14 +338,24 @@ namespace MobileGL::MG_Remote::Client { // scan is what makes the claim checkable rather than assumed. Bool matched = false; Bool bitWasSet = false; + uintptr_t nearestBase = 0; + uintptr_t nearestEnd = 0; for (SizeT i = 0; i < kMaxTrackedMaps; ++i) { - const uintptr_t base = - g_trackedMaps[i].base.load(std::memory_order_acquire); - if (base <= kSlotSettingUp || address < base) continue; - const uintptr_t end = - g_trackedMaps[i].end.load(std::memory_order_acquire); - if (address >= end) continue; - const SizeT pageIndex = (address - base) >> kPageShift; + SizeT pageIndex = 0; + if (!SlotOwnsFault(g_trackedMaps[i], address, pageIndex)) { + // Kept only to name the mapping in the decline record below: the + // first live slot is enough to tell "nothing was tracked at all" + // apart from "something was tracked and the comparison missed it". + if (nearestBase == 0) { + const uintptr_t base = + g_trackedMaps[i].base.load(std::memory_order_acquire); + if (base > kSlotSettingUp) { + nearestBase = base; + nearestEnd = g_trackedMaps[i].end.load(std::memory_order_acquire); + } + } + continue; + } std::atomic* bits = g_trackedMaps[i].pageBits; if (bits != nullptr) { const Uint64 mask = 1ull << (pageIndex & 63); @@ -251,10 +374,20 @@ namespace MobileGL::MG_Remote::Client { g_faultEpoch.fetch_add(1, std::memory_order_release); // Un-arm is a raw syscall, deliberately not the libc wrapper's // bookkeeping: the handler's whole job is to get out of the way. - mprotect(reinterpret_cast(address & ~(kPageBytes - 1)), + mprotect(reinterpret_cast(UntagAddress(address) & ~(kPageBytes - 1)), kPageBytes, PROT_READ | PROT_WRITE); return; } + // A SEGV_ACCERR this handler does not own. Legitimate (the JVM's implicit + // null checks land here every time), so it is recorded and chained, never + // refused - but recorded, because the ONE case where it is not legitimate + // is a page this module protected and then failed to recognise, and that + // case is otherwise a tombstone with no mention of the tracker at all. + g_declinedFaults.fetch_add(1, std::memory_order_relaxed); + g_declinedFaultAddress.store(address, std::memory_order_relaxed); + g_declinedFaultNearestBase.store(nearestBase, std::memory_order_relaxed); + g_declinedFaultNearestEnd.store(nearestEnd, std::memory_order_relaxed); + if (nearestBase != 0) AnnounceDeclinedFault(address, nearestBase, nearestEnd); } ChainToPreviousSegvHandler(sig, info, ucontext); } @@ -345,7 +478,12 @@ namespace MobileGL::MG_Remote::Client { // arm, where as an "untracked" member it would veto the epoch skip for every // tracked buffer in the process (measured on device); end is clamped up to // base so the [base, end) arithmetic never underflows. - const uintptr_t shadowBase = reinterpret_cast(shadow); + // NORMALISED AT THE DOOR (UntagAddress): everything this function publishes is + // compared against si_addr one day, and si_addr is the kernel's untagged + // address. Nothing below may reconstruct a pointer from these - the only + // consumers are mprotect, which untags its own argument, and differences + // against a shadow base PushBlocksFor normalises the same way. + const uintptr_t shadowBase = UntagAddress(reinterpret_cast(shadow)); const uintptr_t rawBegin = shadowBase + rangeBegin; const uintptr_t rawEnd = shadowBase + rangeEnd; constexpr uintptr_t kPageMask = ~static_cast(kPageBytes - 1); @@ -408,10 +546,11 @@ namespace MobileGL::MG_Remote::Client { // once" rule the hash arm keeps: the server has never seen these bytes, // so the first push must ship the whole interior, not just the pages the // application happened to write first. The first push then clears and - // re-arms page by page, and the set-bit read the fault discriminator - // does on a pre-first-push fault is answered once and unprotected - - // harmless, and the invariant (bit set <=> writable) holds from the - // first push onward. The last word is masked: a set bit past pageCount + // re-arms page by page. Nothing is protected until it does, so every set + // bit here names a page that IS writable and the invariant the fault + // discriminator rests on - bit set <=> page writable - holds from this + // line onward rather than only from the first push. The last word is + // masked: a set bit past pageCount // would re-arm a page outside the protected span - past the shadow's // extent, a foreign page, which is the process-killer both alignments // exist to keep out. @@ -423,11 +562,29 @@ namespace MobileGL::MG_Remote::Client { bitsInWord == 64 ? ~0ull : ((1ull << bitsInWord) - 1), std::memory_order_relaxed); } - if (end > base && - mprotect(reinterpret_cast(base), end - base, PROT_READ) != 0) { - g_trackedMaps[i].base.store(0, std::memory_order_release); - return false; - } + // AND THE RANGE IS *NOT* PROTECTED HERE. REGISTRATION ARMS NOTHING. + // + // It used to, and that is what let glMapBufferRange hand the application a + // pointer it could not write: AcquireMemoryRange sets m_isMapped, calls + // NotePersistentMapStateChanged - which lands here - and only THEN returns + // Bytes() + range.start (BufferObject.cpp:913-965). With an mprotect on this + // line, the returned range was PROT_READ before the caller ever saw it, and + // the application's first write was a fault that only the handler could + // rescue. A client may not hand out a pointer whose writability depends on a + // signal handler reaching the writing thread: the application is free to + // write a mapped arena from a worker thread that blocks every signal (the + // chunk builders this tracker exists for do exactly that), and a fault there + // with SIGSEGV blocked is an immediate, undebuggable process kill. + // + // Nothing is lost by waiting. Every bit was just SET, so the first push ships + // the whole interior no matter what faults before it, and it is that push + // that arms the pages (PushBlocksFor: clear the bit, mprotect PROT_READ, read + // the bytes) - which is where the dirty tracking has its first real question + // to ask. Arming here could only ever have produced faults whose answer was + // already known, one per page of the map: 6144 of them for the 24 MB arena + // that crashed on the device. The invariant the rest of this file rests on - + // bit set <=> page writable - is now true from registration onwards as well, + // where before it was knowingly false for the whole pre-first-push window. // Publish order is end, then base: the handler reads base first, and a base // it can observe is only ever one whose end is already visible. g_trackedMaps[i].end.store(end, std::memory_order_release); @@ -439,6 +596,26 @@ namespace MobileGL::MG_Remote::Client { // protected until something else faults - "first push ships everything // once" would then be true only of the read-only verbs' whole push. g_faultEpoch.fetch_add(1, std::memory_order_release); + // NAME THE FIRST MAPPING, ONCE PER PROCESS. This module's whole failure mode + // is silent - a range it protected, a fault it did not recognise, a tombstone + // that names the application's memcpy - and the device run that found it had + // no line anywhere saying a mapping had even been registered, let alone at + // what address, in whose allocation, or with which top byte. One line at the + // first registration costs nothing and carries every field the next + // unexplained SEGV_ACCERR on this device needs: the allocator, the pointer the + // application will be handed, its tag, the extent that pointer owns, the span + // this table published, and which alignment arm produced it. + const uintptr_t tagged = reinterpret_cast(shadow); + MGLOG_I_ONCE("MGPipe: persistent-map tracker armed (first map): lifetime=%llu " + "allocator=MapAlignedAllocator shadow=0x%llx tag=0x%02llx extent=%zu " + "range=[%zu,%zu) tracked=[0x%llx,0x%llx) pages=%zu align=%s " + "handed-out-writable=yes (pages arm at the first push, never here)", + static_cast(lifetimeId), + static_cast(tagged), + static_cast(tagged >> 56), shadowExtent, rangeBegin, + rangeEnd, static_cast(base), + static_cast(end), pageCount, + outward ? "outward" : "inward"); return true; } MGLOG_W("MGPipe: the mprotect tracker is full (%zu live persistent write maps); " @@ -616,6 +793,35 @@ namespace MobileGL::MG_Remote::Client { ResetCountersForTest(); } + uintptr_t PersistentMapTracker::UntagAddressForTest(uintptr_t address) { + return UntagAddress(address); + } + + // The ownership row against a slot the caller composed, WITHOUT protecting anything: an + // x86-64 host has no top byte of its own, so the only way to drive the arm64 defect on + // the gate that has to catch it is to hand the real predicate the two addresses the + // device produced. A scratch slot, never one of the live ones - the handler scans + // g_trackedMaps concurrently and a test may not publish a base into it. + Bool PersistentMapTracker::OwnershipProbeForTest(uintptr_t slotBase, uintptr_t slotEnd, + uintptr_t faultAddress, SizeT* pageIndexOut) { + TrackedWriteMap probe; + probe.base.store(slotBase, std::memory_order_relaxed); + probe.end.store(slotEnd, std::memory_order_relaxed); + SizeT pageIndex = 0; + const Bool owned = SlotOwnsFault(probe, faultAddress, pageIndex); + if (pageIndexOut != nullptr) *pageIndexOut = pageIndex; + return owned; + } + + PersistentMapTracker::DeclinedFaultReport PersistentMapTracker::DeclinedFaults() { + DeclinedFaultReport report; + report.count = g_declinedFaults.load(std::memory_order_relaxed); + report.address = g_declinedFaultAddress.load(std::memory_order_relaxed); + report.nearestBase = g_declinedFaultNearestBase.load(std::memory_order_relaxed); + report.nearestEnd = g_declinedFaultNearestEnd.load(std::memory_order_relaxed); + return report; + } + Bool PersistentMapTracker::MprotectArmAvailableForTest() { InstallWriteFaultHandler(); return g_segvInstalled.load(); @@ -705,7 +911,14 @@ namespace MobileGL::MG_Remote::Client { if (tracked != nullptr) { const uintptr_t trackedBase = tracked->base.load(std::memory_order_acquire); const uintptr_t trackedEnd = tracked->end.load(std::memory_order_acquire); - const uintptr_t shadowBase = reinterpret_cast(buffer.MappedData()); + // THE SAME NORMALISATION TrackWriteMap PUBLISHED WITH (UntagAddress), because + // every use of shadowBase below is a DIFFERENCE against trackedBase and the two + // must live in one address space. The bytes are read through `shadowBytes`, the + // tagged pointer as the allocator handed it out, and never by rebuilding a + // pointer from this value: an untagged pointer is fine to dereference under + // TBI, but it would be a tag fault the day heap tagging becomes real MTE. + const Uint8* const shadowBytes = buffer.MappedData(); + const uintptr_t shadowBase = UntagAddress(reinterpret_cast(shadowBytes)); if (trackedBase != 0 && trackedEnd > trackedBase && shadowBase != 0) { // The protected span in buffer offsets: <= begin / >= end when aligned // outward, >= begin / <= end when aligned inward. Both are read below only @@ -740,7 +953,7 @@ namespace MobileGL::MG_Remote::Client { }; if (interiorBegin > begin) { const Uint64 edgeHash = XXH3_64bits( - reinterpret_cast(shadowBase) + begin, + shadowBytes + begin, static_cast(interiorBegin - begin)); if (edgeHash != tracked->edgeHashHead) { tracked->edgeHashHead = edgeHash; @@ -797,7 +1010,7 @@ namespace MobileGL::MG_Remote::Client { } if (interiorEnd < end) { const Uint64 edgeHash = XXH3_64bits( - reinterpret_cast(shadowBase) + interiorEnd, + shadowBytes + interiorEnd, static_cast(end - interiorEnd)); if (edgeHash != tracked->edgeHashTail) { tracked->edgeHashTail = edgeHash; diff --git a/MobileGL/MG_Remote/Client/PersistentMapTracker.h b/MobileGL/MG_Remote/Client/PersistentMapTracker.h index eb1d0e4c..1deb23b5 100644 --- a/MobileGL/MG_Remote/Client/PersistentMapTracker.h +++ b/MobileGL/MG_Remote/Client/PersistentMapTracker.h @@ -174,6 +174,29 @@ namespace MobileGL::MG_Remote::Client { SizeT rangeEnd, SizeT shadowExtent); static void UntrackForTest(Uint64 lifetimeId); static Uint64 FaultEpochForTest(); + // Unit tests only, and the ONE row that carries the arm64 tagged-pointer defect: the + // kernel reports si_addr untagged while every base this table publishes comes from a + // bionic-tagged heap pointer, so the ownership test has to normalise both sides. + // UntagAddressForTest is that normalisation and OwnershipProbeForTest is the row + // itself, run against a slot the caller composes rather than a live one. Both are + // architecture-independent on purpose - the mask is the identity for every valid + // userspace address off arm64 - so a host CI lane can drive them with the device's + // own two addresses instead of the defect being reachable only on the phone. + static uintptr_t UntagAddressForTest(uintptr_t address); + static Bool OwnershipProbeForTest(uintptr_t slotBase, uintptr_t slotEnd, uintptr_t faultAddress, + SizeT* pageIndexOut); + // Unit tests and device triage: how many SEGV_ACCERRs the handler chained away as + // foreign, and the last one's si_addr beside the first live tracked span. Zero on a + // healthy client - the JVM's implicit null checks do land here, so a NON-zero count + // with a nearest-tracked span whose low 56 bits contain si_addr is the signature of + // the tagged/untagged mismatch and nothing else. + struct DeclinedFaultReport { + Uint64 count = 0; + uintptr_t address = 0; + uintptr_t nearestBase = 0; + uintptr_t nearestEnd = 0; + }; + static DeclinedFaultReport DeclinedFaults(); private: // One live member. tracked is the member's tracker slot when it is on the diff --git a/MobileGL/MG_Test/Wire/CMakeLists.txt b/MobileGL/MG_Test/Wire/CMakeLists.txt index 4277047d..fc37f416 100644 --- a/MobileGL/MG_Test/Wire/CMakeLists.txt +++ b/MobileGL/MG_Test/Wire/CMakeLists.txt @@ -113,11 +113,15 @@ gtest_discover_tests(PipeWireCodecTest DISCOVERY_TIMEOUT 30 PROPERTIES LABELS un # SurfaceControlFrameTest P5f fc: the value frame that replaced the control mailbox's # function-pointer payload, its wire codec against protocol.fbs's # SurfaceOp/SurfaceReply, and the two named refusals at the wire entry +# PersistentMapFaultTest P7 X1: the persistent-map tracker's two client-facing promises - +# that its fault-ownership row runs in the KERNEL's address space +# (arm64 tags every heap pointer, si_addr is untagged) and that a +# map hands back a range writable without the handler # # 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 StagedTextureStoreTest SurfaceControlFrameTest) +foreach (wiretest IN ITEMS RemoteClientTest ServerLoopTest SessionHandshakeTest StagedTextureStoreTest SurfaceControlFrameTest PersistentMapFaultTest) add_executable(${wiretest} ${wiretest}.cpp) target_include_directories(${wiretest} PRIVATE diff --git a/MobileGL/MG_Test/Wire/PersistentMapFaultTest.cpp b/MobileGL/MG_Test/Wire/PersistentMapFaultTest.cpp new file mode 100644 index 00000000..fad7d2de --- /dev/null +++ b/MobileGL/MG_Test/Wire/PersistentMapFaultTest.cpp @@ -0,0 +1,322 @@ +// MobileGL - MobileGL/MG_Test/Wire/PersistentMapFaultTest.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 + +// P7 wave 2 (package X1): the two facts the persistent-map mprotect tracker owes a client, +// both of which it broke, and both of which cost a process on the device while every host +// lane stayed green. +// +// 1. THE OWNERSHIP ROW WORKS IN THE KERNEL'S ADDRESS SPACE. bionic tags every heap +// pointer on a TBI-capable arm64 (POINTER_TAG 0xb4 in the top byte, process-wide under +// PR_TAGGED_ADDR_ENABLE), so the shadow pointer the tracker registers - and every +// base/end it publishes - carries that tag. si_addr does not. Comparing one against the +// other made `address < base` true for EVERY fault, so the handler chained every fault +// on a page it had itself protected straight to debuggerd. No host produces a tagged +// pointer of its own, so the cases below hand the real predicate the two addresses the +// DEVICE produced (docs/Disaggregated/notes/p7/device-window-1, +// improved-transparency-minecraft-26.3 under inproc: x0 b4000073ac9a5000 against +// "fault addr 0x00000073ac9a5000"). That works on any host only because the +// normalisation is written without an architecture guard - masking bits 56-63 is the +// identity for every valid userspace address off arm64 - and that was a deliberate +// choice: an `#if defined(__aarch64__)` would have put the only gate that can catch +// this defect on the one platform CI never runs, which is how it reached the phone. +// +// 2. A MAP HANDS BACK A RANGE THE APPLICATION CAN WRITE, FULL STOP. Registration used to +// mprotect the mapped range PROT_READ before AcquireMemoryRange returned the pointer +// (BufferObject.cpp:913-965), so the writability of a pointer the GL API had promised +// was writable depended on a SIGSEGV handler reaching the writing thread. It does not +// always: the application may write a mapped arena from a worker that blocks every +// signal, and a fault there with SIGSEGV blocked is an instant process kill with no +// handler run and no tombstone worth reading. The case below writes every page of a +// freshly registered range from a process that has BOTH blocked SIGSEGV and restored +// SIG_DFL, which is the strongest available spelling of "without relying on the +// handler". +// +// RED-ONCE (both, and each for its own reason): +// * restore `mprotect(base, end - base, PROT_READ)` at the end of TrackWriteMap's slot +// setup -> AMapHandsBackARangeWritableWithoutTheFaultHandler dies by SIGSEGV in the +// child, and the two page-protection cases go red; the ownership cases stay green. +// * drop the two UntagAddress calls in SlotOwnsFault -> the three ownership cases go red +// and the writability cases stay green. +// Neither revert can be hidden by the other, which is the property R-16 asks for. + +#include +#include +#include +#include + +#include + +#include +#include +#include +#include + +#if !defined(_WIN32) +#include +#include +#include +#endif + +namespace { + + using MobileGL::Bool; + using MobileGL::SizeT; + using MobileGL::Uint64; + using MobileGL::Uint8; + using MobileGL::MG_Remote::Client::PersistentMapTracker; + using MobileGL::MG_State::GLState::MapAlignedData; + using MobileGL::MG_State::GLState::ShadowAllocationBytesFor; + + constexpr SizeT kPageBytes = 4096; + + // The device's own two numbers, from the E0a tombstone. Kept as literals, not as + // "some pointer with the top byte set": the point of the case is that THESE two + // addresses name one page, and a reader who doubts it can check them against the + // tombstone quoted in the file header. + constexpr uintptr_t kDeviceTaggedShadowBase = 0xb4000073ac9a5000ull; + constexpr uintptr_t kDeviceKernelFaultAddress = 0x00000073ac9a5000ull; + constexpr uintptr_t kDeviceMappedBytes = 0x1800000ull; // 24 MiB, the memcpy's x2 + +} // namespace + +// ===================================================================================== +// 1. THE OWNERSHIP ROW, IN THE KERNEL'S ADDRESS SPACE +// ===================================================================================== + +// THE CASE THAT WOULD HAVE CAUGHT THE DEVICE CRASH ON A HOST, and it runs on every host: +// both addresses are the device's literal ones and the normalisation they need is written +// without an architecture guard precisely so this can be a gate rather than a phone-only +// hope. The predicate must say the kernel's untagged fault address belongs to the slot a +// bionic-tagged shadow pointer published. +TEST(PersistentMapFaultOwnership, AKernelsUntaggedFaultAddressIsOwnedByATaggedShadowsSlot) { + SizeT pageIndex = ~static_cast(0); + EXPECT_TRUE(PersistentMapTracker::OwnershipProbeForTest( + kDeviceTaggedShadowBase, kDeviceTaggedShadowBase + kDeviceMappedBytes, + kDeviceKernelFaultAddress, &pageIndex)) + << "the handler compared si_addr (untagged, as the kernel delivers it) against a base " + "carrying bionic's 0xb4 heap tag, so it owned nothing and chained every fault"; + EXPECT_EQ(pageIndex, 0u) << "the fault is on the first page of the mapped range"; +} + +// The same row without needing the host to ignore a top byte at all: the predicate is fed a +// composed slot, so it runs everywhere and goes red everywhere the normalisation is dropped. +// This is the case the gate rests on; the one above is the device's literal reproduction. +TEST(PersistentMapFaultOwnership, TheOwnershipRowNormalisesBothSidesBeforeItCompares) { + SizeT pageIndex = ~static_cast(0); + const uintptr_t base = PersistentMapTracker::UntagAddressForTest(kDeviceTaggedShadowBase); + // Untagged base against untagged fault: owned, on this host and on the device alike. + EXPECT_TRUE(PersistentMapTracker::OwnershipProbeForTest( + base, base + kDeviceMappedBytes, kDeviceKernelFaultAddress, &pageIndex)); + EXPECT_EQ(pageIndex, 0u); + // A fault one page in is owned, and names that page. + EXPECT_TRUE(PersistentMapTracker::OwnershipProbeForTest( + base, base + kDeviceMappedBytes, kDeviceKernelFaultAddress + kPageBytes + 7, &pageIndex)); + EXPECT_EQ(pageIndex, 1u); + // And the narrow claim stays narrow: a byte before the base and the first byte past the + // end are both foreign, so a genuinely broken access still crashes honestly. + EXPECT_FALSE(PersistentMapTracker::OwnershipProbeForTest( + base, base + kDeviceMappedBytes, kDeviceKernelFaultAddress - 1, nullptr)); + EXPECT_FALSE(PersistentMapTracker::OwnershipProbeForTest( + base, base + kDeviceMappedBytes, kDeviceKernelFaultAddress + kDeviceMappedBytes, nullptr)); + // A free slot (base 0) and one mid-setup (base 1) own nothing, whatever the address. + EXPECT_FALSE(PersistentMapTracker::OwnershipProbeForTest(0, 0, kDeviceKernelFaultAddress, nullptr)); + EXPECT_FALSE(PersistentMapTracker::OwnershipProbeForTest(1, 0, kDeviceKernelFaultAddress, nullptr)); +} + +TEST(PersistentMapFaultOwnership, UntagAddressClearsTheTopByteAndNothingBelowIt) { + EXPECT_EQ(PersistentMapTracker::UntagAddressForTest(kDeviceTaggedShadowBase), + kDeviceKernelFaultAddress) + << "bionic's 0xb4 heap tag must come off, or the table and the kernel disagree"; + EXPECT_EQ(PersistentMapTracker::UntagAddressForTest(kDeviceKernelFaultAddress), + kDeviceKernelFaultAddress) + << "normalising an already-normal address is the identity, or the handler would move " + "a legitimate fault out of its own slot"; + // Bit 55 is ADDRESS, not tag: userspace lives in TTBR0 on arm64 and below TASK_SIZE_MAX + // on x86-64, and both keep every bit under 56 meaningful. + constexpr uintptr_t kBit55 = static_cast(1) << 55; + EXPECT_EQ(PersistentMapTracker::UntagAddressForTest(kBit55), kBit55); + // A real pointer from this build's own allocator survives untouched, which is the + // property that makes the mask safe to run unguarded on a host with no tags at all. + MapAlignedData probe(kPageBytes); + const auto real = reinterpret_cast(probe.data()); + EXPECT_EQ(PersistentMapTracker::UntagAddressForTest(real), real); +} + +// ===================================================================================== +// 2. A MAP HANDS BACK A RANGE THE APPLICATION CAN WRITE +// ===================================================================================== + +#if !defined(_WIN32) + +namespace { + + // Every page of [begin, begin + bytes) as /proc/self/maps reports it. The protection is + // the fact under test and there is no portable getter for it, so the file is read + // directly; a host without it skips rather than passes. + Bool EveryPageIsWritable(const Uint8* begin, SizeT bytes, std::string& detail) { + std::FILE* maps = std::fopen("/proc/self/maps", "re"); + if (maps == nullptr) { + detail = "no /proc/self/maps on this host"; + return true; + } + const auto first = reinterpret_cast(begin); + const uintptr_t last = first + bytes - 1; + uintptr_t covered = first; + Bool writable = true; + char line[512]; + while (std::fgets(line, sizeof(line), maps) != nullptr) { + unsigned long long lo = 0; + unsigned long long hi = 0; + char perms[8] = {0}; + if (std::sscanf(line, "%llx-%llx %7s", &lo, &hi, perms) != 3) continue; + if (hi <= first || lo > last) continue; + if (perms[1] != 'w') { + writable = false; + detail = std::string("range [") + std::to_string(lo) + "," + std::to_string(hi) + + ") is '" + perms + "'"; + break; + } + if (lo <= covered && hi > covered) covered = hi; + } + std::fclose(maps); + if (writable && covered <= last) { + writable = false; + detail = "the mapped range is not fully covered by any writable mapping"; + } + return writable; + } + +} // namespace + +// THE INVARIANT. A freshly registered write map's whole range must be writable from a thread +// that can never run the handler - SIGSEGV blocked AND the disposition back at SIG_DFL - or +// the client has handed the application a pointer whose writability is a signal-delivery +// accident. Driven in a child, because the red form of this case is a process death by +// SIGSEGV and the suite has to survive observing it. +TEST(PersistentMapWritability, AMapHandsBackARangeWritableWithoutTheFaultHandler) { + if (!PersistentMapTracker::MprotectArmAvailableForTest()) { + GTEST_SKIP() << "no mprotect arm on this host (handler not installable, or the kernel " + "page is not the tracker's 4 KB): nothing here can protect a page"; + } + // Eight pages, so a per-page protection bug cannot hide inside one page's rounding. + constexpr SizeT kBytes = 8u * kPageBytes; + MapAlignedData store(kBytes); + ASSERT_EQ(reinterpret_cast(store.data()) % kPageBytes, 0u); + constexpr Uint64 kId = ~0ull - 21u; + const auto* slot = PersistentMapTracker::TrackForTest(kId, store.data(), 0, kBytes, + ShadowAllocationBytesFor(store.capacity())); + ASSERT_NE(slot, nullptr) << "a page-aligned, page-granular shadow registers on the mprotect arm"; + ASSERT_EQ(slot->pageCount, 8u); + + const pid_t child = ::fork(); + ASSERT_GE(child, 0) << "fork failed: " << std::strerror(errno); + if (child == 0) { + // No handler can rescue this process: the disposition is the default and the signal + // is blocked in this thread, so a fault here is a kill, not a fault. + struct sigaction dfl {}; + dfl.sa_handler = SIG_DFL; + ::sigemptyset(&dfl.sa_mask); + ::sigaction(SIGSEGV, &dfl, nullptr); + ::sigaction(SIGBUS, &dfl, nullptr); + sigset_t blocked; + ::sigemptyset(&blocked); + ::sigaddset(&blocked, SIGSEGV); + ::sigaddset(&blocked, SIGBUS); + ::pthread_sigmask(SIG_BLOCK, &blocked, nullptr); + Uint8* const mapped = store.data(); + for (SizeT page = 0; page < kBytes / kPageBytes; ++page) { + mapped[page * kPageBytes] = static_cast(0x5A + page); + mapped[page * kPageBytes + kPageBytes - 1] = static_cast(0xA5 - page); + } + ::_exit(0); + } + int status = 0; + ASSERT_EQ(::waitpid(child, &status, 0), child); + EXPECT_FALSE(WIFSIGNALED(status)) + << "the range glMapBufferRange hands back was not writable: the child died by signal " + << (WIFSIGNALED(status) ? WTERMSIG(status) : 0) + << ". Registration must arm nothing - the first push is what protects pages"; + EXPECT_TRUE(WIFEXITED(status) && WEXITSTATUS(status) == 0); + + PersistentMapTracker::UntrackForTest(kId); + store[0] = 1; + store[kBytes - 1] = 1; +} + +// The same fact read straight off the kernel's own bookkeeping, so a failure names the +// protection rather than only the death it caused. +TEST(PersistentMapWritability, RegistrationLeavesEveryPageOfTheRangeReadWrite) { + if (!PersistentMapTracker::MprotectArmAvailableForTest()) { + GTEST_SKIP() << "no mprotect arm on this host"; + } + constexpr SizeT kBytes = 8u * kPageBytes; + MapAlignedData store(kBytes); + constexpr Uint64 kId = ~0ull - 22u; + const auto* slot = PersistentMapTracker::TrackForTest(kId, store.data(), 0, kBytes, + ShadowAllocationBytesFor(store.capacity())); + ASSERT_NE(slot, nullptr); + std::string detail; + EXPECT_TRUE(EveryPageIsWritable(store.data(), kBytes, detail)) + << "registration protected the range it had just handed out: " << detail; + PersistentMapTracker::UntrackForTest(kId); +} + +// AND THE HANDLER IS STILL REACHED WHEN IT MATTERS. Deferring the arm must not turn the +// tracker off: a page the module protects by hand - which is what the first push does - has +// to be answered and made writable again, in this process, by this handler. Without this the +// two cases above would stay green with the whole mprotect arm deleted. +TEST(PersistentMapWritability, AnArmedPageIsStillAnsweredByTheHandlerAndMadeWritable) { + if (!PersistentMapTracker::MprotectArmAvailableForTest()) { + GTEST_SKIP() << "no mprotect arm on this host"; + } + constexpr SizeT kBytes = 2u * kPageBytes; + MapAlignedData store(kBytes); + constexpr Uint64 kId = ~0ull - 23u; + const auto* slot = PersistentMapTracker::TrackForTest(kId, store.data(), 0, kBytes, + ShadowAllocationBytesFor(store.capacity())); + ASSERT_NE(slot, nullptr); + // Arm the first page the way the push does, and write through it: the fault must be + // answered (the epoch moves) rather than chained (which would kill this process). + ASSERT_EQ(::mprotect(store.data(), kPageBytes, PROT_READ), 0) << std::strerror(errno); + const Uint64 epochBefore = PersistentMapTracker::FaultEpochForTest(); + const Uint64 declinedBefore = PersistentMapTracker::DeclinedFaults().count; + store[16] = 0x5A; + EXPECT_GT(PersistentMapTracker::FaultEpochForTest(), epochBefore) + << "the write did not fault, so this case proved nothing about the handler"; + EXPECT_EQ(PersistentMapTracker::DeclinedFaults().count, declinedBefore) + << "the handler chained its own protected page away as foreign - the ownership row " + "is comparing two different address spaces again"; + EXPECT_EQ(store[16], 0x5A); + PersistentMapTracker::UntrackForTest(kId); +} + +#endif // !_WIN32 + +int main(int argc, char** argv) { + // Before anything logs, for StagedTextureStoreTest's reason: the file sink caches its + // FILE* on the first write and gtest_discover_tests runs every case as its own process. + namespace fs = std::filesystem; + const fs::path path = + fs::temp_directory_path() / + ("mobilegl-persistentmapfault-test-" + std::to_string(static_cast(::getpid())) + ".log"); + std::error_code ec; + fs::remove(path, ec); + const std::string logPath = path.string(); +#if defined(_WIN32) + _putenv_s("MOBILEGL_LOG_FILE_PATH", logPath.c_str()); +#else + setenv("MOBILEGL_LOG_FILE_PATH", logPath.c_str(), 1); +#endif + // A SPLIT process: the tracker is inert on the monolith path and every case here is + // about the split arm. + MobileGL::MG_Config::Transport = MobileGL::MG_Config::TransportMode::InProcess; + ::testing::InitGoogleTest(&argc, argv); + const int rc = RUN_ALL_TESTS(); + fs::remove(path, ec); + return rc; +} diff --git a/docs/Disaggregated/notes/p7/x1-persistent-map-segv.md b/docs/Disaggregated/notes/p7/x1-persistent-map-segv.md new file mode 100644 index 00000000..a89d3374 --- /dev/null +++ b/docs/Disaggregated/notes/p7/x1-persistent-map-segv.md @@ -0,0 +1,169 @@ +# X1 — the client-thread SIGSEGV in a mapped buffer under inproc (device window #1) + +Wave 2, package X1. Base `a686131e`. Branch `p7/segv`. + +## 1. What the tombstone actually said + +Three CI split-subset traces died on the Redmi (`2f7cbe2e`, Adreno 830, APK `p7w1-d260f110`) +under `MOBILEGL_TRANSPORT=inproc` and passed under monolith on the same device and APK: +`improved-transparency-minecraft-26.3`, `minecraft-1.21.1-neoforge-create-instancing-in-world`, +`minecraft-1.21.1-neoforge-create-indirect-in-world`. Every one of them is +`SIGSEGV / SEGV_ACCERR` inside `__memcpy_aarch64_nt`, on the `MobileGLTraceRe` (GL/client) +thread, in the apitrace replayer's write into a pointer MobileGL had handed it. + +The deciding evidence is two lines of the tombstone that name **the same address twice, in two +different address spaces**: + +``` +x0 b4000073ac9a5000 x1 b4000073ab1a2000 x2 0000000001800000 +signal 11 (SIGSEGV), code 2 (SEGV_ACCERR), fault addr 0x00000073ac9a5000 +``` + +`x0` is the memcpy destination — the pointer the application is writing through. `0xb4` in its +top byte is bionic's `POINTER_TAG`: on a TBI-capable arm64 every heap pointer carries it +(`tagged_addr_ctrl: 0000000000000001 (PR_TAGGED_ADDR_ENABLE)`, same tombstone). `fault addr` is +`info->si_addr`, and the kernel reports it **untagged**. `x2` is the copy length, 24 MiB. The +fault is on the destination's *first* byte, and the destination is page-aligned — which is the +shadow allocator's signature (`PipeResource.h:77`, `::operator new(…, align_val_t{4096})`). + +All four crash sites have exactly this shape: + +| case | x0 (tagged dst) | si_addr | x2 (length) | +|---|---|---|---| +| 26.3, DirectVulkan (E0a) | `b4000073ac9a5000` | `0x73ac9a5000` | `0x1800000` | +| 26.3, DirectGLES (E4a) | `b4000073b1d39000` | `0x73b1d39000` | `0x1800000` | +| 26.3, DirectGLES, block=0 (E4b) | `b4000073ab788000` | `0x73ab788000` | `0x1800000` | +| create-instancing (E0a) | `b40000739de43000` | `0x739de43000` | `0x5e10` | +| create-indirect (E0a) | `b40000739f5e8000` | `0x739f5e8000` | `0x7000` | + +(The logcat tombstone carries registers and backtrace but not the `memory map` section — +that lives only in `/data/tombstones/`. The registers were enough.) + +## 2. The mechanism + +`MG_Remote/Client/PersistentMapTracker.cpp`, two rows that never lived in the same address +space: + +* **Registration** (`TrackWriteMap`, formerly `:348`) took `shadowBase = + (uintptr_t)buffer.MappedData()` — a **tagged** heap pointer — derived `base`/`end` from it + (`& kPageMask` clears the low 12 bits and preserves the top byte), `mprotect`ed + `[base, end)` `PROT_READ`, and published the **tagged** `base`/`end` into the slot. +* **The handler** (`PersistentWriteFaultHandler`, formerly `:218/:231/:234`) took + `address = (uintptr_t)info->si_addr` — **untagged** — and tested + `if (base <= kSlotSettingUp || address < base) continue;`. + +`0x73ac9a5000 < 0xb4000073ac9a5000` is true, for every fault, forever. Ownership test (2) never +matched, `matched` stayed `false`, and every fault on a page the tracker had itself protected +fell through to `ChainToPreviousSegvHandler` → bionic's debuggerd → tombstone. + +`mprotect` itself was fine: `do_mprotect_pkey` calls `untagged_addr()` on its argument, so the +right pages really were protected. Only the *recognition* was broken. + +The other half of the defect is the ordering that made the handler load-bearing at all: +`BufferObject::AcquireMemoryRange` sets `m_isMapped`, calls `NotePersistentMapStateChanged()` +(which lands in `TrackWriteMap` and protected the range), and only *then* returns +`m_resource.Bytes() + range.start` (`BufferObject.cpp:913-965`). So `glMapBufferRange` with +`GL_MAP_WRITE_BIT | GL_MAP_PERSISTENT_BIT` handed the application a **read-only** pointer whose +writability depended entirely on a SIGSEGV handler reaching the writing thread. + +### Why every fact fits + +* **E1 (`RUN_AHEAD=0`) still crashes** — nothing here touches run-ahead. +* **E4a (DirectGLES) still crashes** — the tracker is backend-independent; it sits in the + client's `MG_State` map path. +* **E4b (`PERSISTENT_BLOCK_KB=0`) still crashes** — the knob gates `PushBlocksFor` + (`:849`, the `blockBytes == 0` early return), *not* `NoteMapStateChanged`/`TrackWriteMap`. + The mprotect arm is armed with the push off. +* **No tracker warnings in the client log** — registration succeeded; nothing on the + successful path logged anything. +* **Crashes early** (client log ends right after `CapsMirror generation 3 adopted`) — the + first persistent write map of the trace is the first and last one. +* **WSL/lavapipe green** — x86-64 has no top-byte tag, so `base` and `si_addr` agree and the + handler works. This is why every host lane has always been green. +* **Monolith green on the same device** — `NotePersistentMapStateChanged` returns immediately + when `PushIsArmed()` is false (`BufferObject.cpp:489`), so nothing is ever protected. + +### Ruled out + +ART's `libsigchain` is not implicated: the tombstone's `F libc : Fatal signal 11` line is +bionic's `debuggerd_signal_handler` output, which proves the signal *was* delivered to a +userspace handler chain and that our handler ran and chained. A blocked SIGSEGV would have had +the kernel force `SIG_DFL` and produced no such line. The 16 KiB-page refusal (`:274-278`) is +not reached (the device is a 4 KiB kernel — the protection demonstrably took). The +`RefaultOfANonWriteAccess` decline (`:156`) is not reached either: `matched` is false long +before it is consulted. P5d's `SHADOW_ALLOCATION_ALIGNMENT` / zero-page registration are +correct and are in fact what makes the destination page-aligned in the tombstone. + +## 3. The fix + +`MG_Remote/Client/PersistentMapTracker.cpp`: + +1. **`UntagAddress()`** — one normalisation, applied to the shadow base at registration + (`TrackWriteMap`), to the shadow base in `PushBlocksFor` (so `trackedBase - shadowBase` + stays in one space), and to `si_addr` in the handler. Written **without** an + `#if defined(__aarch64__)`: masking bits 56-63 is the identity for every valid userspace + address off arm64 (Linux/x86-64 caps `TASK_SIZE_MAX` below 2^56), and an arch guard would + have put the only gate that can catch this on the one platform CI never runs — which is how + it reached the device. Pointer *dereferences* keep the tagged pointer they came from + (`PushBlocksFor`'s two edge hashes now read through `shadowBytes`), because under a future + real-MTE tagging level the tag is load-bearing for the access itself. +2. **`SlotOwnsFault()`** — ownership test (2) extracted into one row the handler and the unit + test both read. +3. **Registration arms nothing.** The `mprotect(base, end - base, PROT_READ)` at the end of + `TrackWriteMap`'s slot setup is gone. Nothing is lost: every page bit is already SET at + registration, so the first push ships the whole interior regardless, and it is that push + (`PushBlocksFor`, `:775`) that arms the pages — which is where the dirty tracking has its + first real question to ask. Arming at registration could only ever produce faults whose + answer was already known, one per page: 6144 of them for the 24 MB arena that crashed. + The invariant the file rests on — *bit set ⇔ page writable* — is now true from registration + onward, where before it was knowingly false for the whole pre-first-push window. + +### The diagnostic (independent of the fix) + +* **One line per process at the first registration** (`MGLOG_I_ONCE`), naming the mapping the + way the device run needed and did not have: allocator, the pointer the application will be + handed, **its tag**, the extent that pointer owns, the mapped range, the published span, the + page count and which alignment arm produced it. +* **A decline record in the handler**, written with relaxed atomic stores only (async-signal- + safe), readable from the GL thread through `PersistentMapTracker::DeclinedFaults()`, plus one + async-signal-safe `write(2)` line to fd 2 the first time a decline happens while any slot is + live. A non-zero decline count whose `si_addr` lies inside `nearest-tracked` **once the top + byte is masked off** is the signature of this defect and of nothing else. + +## 4. Red-once (two, orthogonal) + +`MG_Test/Wire/PersistentMapFaultTest.cpp`, six cases, registered in +`MG_Test/Wire/CMakeLists.txt`'s `wiretest` loop. + +| revert | red | green | +|---|---|---| +| drop the two `UntagAddress` calls in `SlotOwnsFault` | `AKernelsUntaggedFaultAddressIsOwnedByATaggedShadowsSlot` | the other 5 | +| restore `mprotect(base, end - base, PROT_READ)` at the end of `TrackWriteMap` | `AMapHandsBackARangeWritableWithoutTheFaultHandler` (child dies by SIGSEGV), `RegistrationLeavesEveryPageOfTheRangeReadWrite` | the other 4 | + +Neither revert can be hidden by the other. `AnArmedPageIsStillAnsweredByTheHandlerAndMadeWritable` +is the anti-vacuity case: it protects a page by hand the way the push does and asserts the +handler answers it, so the two writability cases cannot stay green with the whole mprotect arm +deleted. + +The first case runs the **device's own two addresses** (`0xb4000073ac9a5000` against +`0x73ac9a5000`) through the real predicate, on an x86-64 host. That is the gate this defect +should have had. + +## 5. What the integrator must still decide + +1. **The device run.** The fix is blind with respect to the phone — see the experiment in the + hand-off. One `--matrix` pass over the three cases under inproc is the whole test. +2. **P5d's device numbers are suspect.** If registration always protected and the handler always + declined on arm64, then on Android the mprotect arm has never answered a single fault: every + persistent write map either killed the process or was never written through. Any + P5d measurement taken on the Redmi that attributes a win to the mprotect arm needs re-taking. +3. **Residual: a writer thread with SIGSEGV blocked.** After the first push the pages are armed + again while the map is live, so a write from a thread that blocks every signal is still a + process kill. Registration no longer creates that window, but the steady state does. If the + integrator wants it closed outright, the arm has to become opt-in per buffer (or the push has + to fall back to the hash arm for maps the application writes off the GL thread) — that is a + design change, not a bug fix, and it is not in X1. +4. **Residual: `RefaultOfANonWriteAccess`** can still decline a legitimate repeat write at the + same `(tid, pc, address)` when the page's bit is set. Deferring the arm removes the one + window that made "bit set while page read-only" routine, but the row is still reachable by a + sibling thread mid-answer. Not observed; recorded.