diff --git a/mpcore/src/main/cpp/emu/guest_heap.cpp b/mpcore/src/main/cpp/emu/guest_heap.cpp index 2be48d2..43dec8b 100644 --- a/mpcore/src/main/cpp/emu/guest_heap.cpp +++ b/mpcore/src/main/cpp/emu/guest_heap.cpp @@ -44,7 +44,40 @@ void GuestHeap::PushFree(GuestAddr dataAddr, uint32_t size) { uint32_t GuestHeap::BlockSize(GuestAddr addr) const { if (!IsValidLiveBlock(addr)) return 0; - return HeaderAt(addr)->size; + // The USER size, not the footprint - callers (Shim_realloc) want to know + // how many bytes they were actually given, not how much the guard adds. + return HeaderAt(addr)->user_size; +} + +void GuestHeap::WriteRedZone(GuestAddr dataAddr, uint32_t userSize) { + if (!kHeapDebugChecks) return; + std::memset(host_base_ + dataAddr + userSize, kRedZoneFill, kRedZoneBytes); +} + +bool GuestHeap::CheckRedZone(GuestAddr dataAddr, uint32_t userSize) const { + if (!kHeapDebugChecks) return true; + const uint8_t* guard = host_base_ + dataAddr + userSize; + for (uint32_t i = 0; i < kRedZoneBytes; i++) { + if (guard[i] == kRedZoneFill) continue; + // Loud and specific: this identifies the block that was overflowed + // AND how far past its end the write reached, which is the whole + // point - the corruption is reported at the offending block rather + // than wherever it happens to surface later. + Log("GuestHeap: RED ZONE VIOLATION - block at 0x%x (user_size=%u) was written past its " + "end: guard byte %u of %u is 0x%02x, expected 0x%02x. Something overflowed THIS " + "block; see guest_heap.h's kHeapDebugChecks comment.", + dataAddr, userSize, i, kRedZoneBytes, guard[i], kRedZoneFill); + return false; + } + return true; +} + +void GuestHeap::PoisonPayload(GuestAddr dataAddr, uint32_t userSize) { + if (!kHeapDebugChecks) return; + // Makes a use-after-free read obviously-wrong data. Matters more since + // Alloc stopped zeroing (2026-09-19): without this, a freed block keeps + // its old, plausible contents and a stale pointer keeps "working". + std::memset(host_base_ + dataAddr, kFreePoisonFill, userSize); } GuestAddr GuestHeap::Alloc(uint32_t size) { @@ -52,7 +85,11 @@ GuestAddr GuestHeap::Alloc(uint32_t size) { if (!host_base_ || size == 0) return 0; uint64_t callNo = g_allocCalls2.fetch_add(1, std::memory_order_relaxed) + 1; uint64_t t0 = NowNs2(); - uint32_t payload = AlignUp(size, kAlign); + const uint32_t userSize = AlignUp(size, kAlign); + // Everything below allocates in terms of the FOOTPRINT (user bytes plus + // the trailing guard). kRedZoneBytes is 0 unless checks are on, so this + // is identical to the previous arithmetic in normal builds. + const uint32_t payload = userSize + kRedZoneBytes; auto logIfSampled = [&](const char* how) { if (callNo <= 5 || callNo % 20000 == 0) { Log("GuestHeap::Alloc: call #%llu (%s), took %lluns", @@ -109,9 +146,14 @@ GuestAddr GuestHeap::Alloc(uint32_t size) { th->magic = kMagic; th->size = remainder - sizeof(BlockHeader); th->free = 1; - th->pad = 0; + th->user_size = th->size; PushFree(tailHeader + sizeof(BlockHeader), th->size); } + // The guard goes right after the USER bytes, so an overflow of even + // one byte past what the caller asked for is caught - even when the + // block reused was larger and did not get split. + h->user_size = userSize; + WriteRedZone(blockAddr, userSize); live_bytes_ += h->size; if (live_bytes_ > peak_live_bytes_) peak_live_bytes_ = live_bytes_; logIfSampled("reuse"); @@ -133,9 +175,10 @@ GuestAddr GuestHeap::Alloc(uint32_t size) { h->magic = kMagic; h->size = payload; h->free = 0; - h->pad = 0; + h->user_size = userSize; GuestAddr dataAddr = free_cursor_ + sizeof(BlockHeader); free_cursor_ += needed; + WriteRedZone(dataAddr, userSize); live_bytes_ += payload; if (live_bytes_ > peak_live_bytes_) peak_live_bytes_ = live_bytes_; logIfSampled("bump-new"); @@ -164,6 +207,13 @@ void GuestHeap::Free(GuestAddr addr) { return; } BlockHeader* h = HeaderAt(addr); + // Verify the guard BEFORE the block goes back on a free list: this is the + // last moment the overflow can still be attributed to this block rather + // than to whoever allocates the memory next. Deliberately not fatal - the + // point is a loud, precise report, and aborting here would make the + // engine less debuggable than the corruption it is reporting. + CheckRedZone(addr, h->user_size); + PoisonPayload(addr, h->user_size); h->free = 1; live_bytes_ -= h->size; // Every block carries its exact payload size, so it goes straight onto diff --git a/mpcore/src/main/cpp/emu/guest_heap.h b/mpcore/src/main/cpp/emu/guest_heap.h index 2b450de..17da8f0 100644 --- a/mpcore/src/main/cpp/emu/guest_heap.h +++ b/mpcore/src/main/cpp/emu/guest_heap.h @@ -93,11 +93,43 @@ private: // payloads are multiples of kAlign), and makes the split arithmetic below // exact. Costs 4 bytes per block, trivial next to the ~4x this file's // 2026-09-19 rework removes. + // ---- memory-error detection (2026-09-19, task #45) ---- + // OFF by default. The existing magic check on Free catches a wrong or + // stale POINTER, but not the more insidious failure this layer is prone + // to: a shim writing a few bytes past the end of a block and silently + // corrupting whatever live object follows. That has already happened + // once in this project's history (the guest JNIEnv structure was + // overwritten that way - see this class's own comment) and it surfaces + // subsystems later, looking like an unrelated crash. + // + // With checks on, each block gets a trailing guard filled with a known + // pattern, verified when the block is freed, and freed payloads are + // poisoned so a use-after-free reads obviously-wrong data instead of + // plausible stale values. The latter matters more since 2026-09-19, + // when Alloc stopped zeroing. + // + // Both cost memory and time, so this follows the same opt-in discipline + // as every other diagnostic here (see kTraceHeapAllocations in + // import_shims.cpp): flip to true while hunting a corruption bug, never + // leave it on. With it false, kRedZoneBytes is 0 and every guard-related + // branch folds away at compile time. + static constexpr bool kHeapDebugChecks = false; + static constexpr uint32_t kRedZoneBytes = kHeapDebugChecks ? 16 : 0; + static constexpr uint8_t kRedZoneFill = 0xBE; // "guard" + static constexpr uint8_t kFreePoisonFill = 0xDF; // "dead free" + struct BlockHeader { uint32_t magic; // kMagic if this is a real header GuestHeap itself wrote - see class comment - uint32_t size; // payload size, not including this header + // Total payload footprint this block occupies, INCLUDING the trailing + // guard when checks are on. Free lists, splitting and IsValidLiveBlock + // all work in these terms. + uint32_t size; uint32_t free; // 1 = free, 0 = in use - uint32_t pad; // alignment only - see above + // What the caller actually asked for (aligned up) - i.e. size minus + // the guard. Was pure alignment padding before; it is maintained + // unconditionally so BlockSize() can report the user-visible size and + // realloc can copy the right amount, whether or not checks are on. + uint32_t user_size; }; static constexpr uint32_t kMagic = 0x47484B21; // "GuestHeap blocK!" - arbitrary but distinctive static constexpr uint32_t kAlign = 8; @@ -175,6 +207,16 @@ private: // Pushes a free block onto the intrusive list for its exact size. void PushFree(GuestAddr dataAddr, uint32_t size); + // Guard handling - all no-ops when kHeapDebugChecks is false. + // `dataAddr` is a payload address, `userSize` what the caller asked for; + // the guard lives immediately after the user payload so that a one-byte + // overflow is caught, rather than at the very end of a possibly-larger + // block where slack would hide it. + void WriteRedZone(GuestAddr dataAddr, uint32_t userSize); + // Logs and returns false if the guard was damaged. + bool CheckRedZone(GuestAddr dataAddr, uint32_t userSize) const; + void PoisonPayload(GuestAddr dataAddr, uint32_t userSize); + BlockHeader* HeaderAt(GuestAddr addr) const { return reinterpret_cast(host_base_ + addr - sizeof(BlockHeader)); }