Skip to content

Poison the other allocators that carve from the guest heap - #84

Merged
sidick merged 2 commits into
mainfrom
poison-all-allocators
Sep 16, 2026
Merged

sidick merged 2 commits into
mainfrom
poison-all-allocators

Conversation

@sidick

@sidick sidick commented Sep 16, 2026

Copy link
Copy Markdown
Owner

Tier 0 of #83. --sanitize's heap detectors only ever poisoned AllocMem/AllocVec/AllocPooled, but three other guest-visible APIs allocate from the same GuestHeap:

API Module
AllocateTagItems / FreeTagItems utility.rs
AllocDosObject / FreeDosObject dosfile.rs
CreateIORequest / DeleteIORequest, CreateMsgPort / DeleteMsgPort execlist.rs

Because they go through GuestHeap::alloc, redzone space was already being reserved for them whenever --sanitize was on — the guard bytes existed, nothing marked them poisoned. So a guest overrunning a FileInfoBlock, an RDArgs, a MsgPort or a TagItem array that volamos handed it went entirely unreported. Sharing execmem's existing helpers (now pub(crate)) is the whole fix.

Ordering, at every site

Poison after each handler's own zeroing and field initialisation, never before — those writes go through the checked path and heal shadow bytes, so poisoning first would make each handler report a violation against its own setup. Same rule established in #80. Each call also passes mark_data_uninit = false, because these blocks are initialised by the time they're handed over; marking them uninit would produce false reports under --sanitize-uninit.

CreateIORequest is poisoned in its handler rather than in the create_io_request free function, which takes heap/mem separately and has no HandlerContext to reach the shadow map through.

Testing

1002 tests green, clippy clean at -D warnings, fmt clean — all three verified by exit code this time, not through a pipe.

New tests cover both halves: redzones present either side of an AllocateTagItems block (with the block itself still reading as initialised), and a freed block reading as Unaddressable with PoisonReason::Freed specifically rather than as a redzone.

False-positive sweep: all fixtures, memtest clean and pLhA (102-file archive) stay silent; detection still fires on memtest overrun.

One thing to flag, not caused by this PR

The sweep surfaced a violation in PhxAss:

sanitizer: 1 site(s), 2 violation(s):
  invalid 2-byte read at 0x00036fe8 (heap redzone) from PC 0x0000483e (2 hits)

It reproduces on main without this change, and PhxAss uses none of the four APIs above — I bisected it to #82 (7eac2f1 and 88f0fc9 are both clean). So it's out of scope here, and I'll raise it separately: either a genuine PhxAss 2-byte over-read newly exposed by a heap-layout shift, or a false positive introduced by that PR.

Still not covered

exec.library/Allocate (#83's main body), where the guest owns the MemHeader so guard space can't be reserved without touching its accounting — that's where C malloc lives. AllocAbs, AllocEntry and AllocVecPooled are in the LVO table with no handlers, so nothing is missed yet; noted that they should get poisoning when implemented.

🤖 Generated with Claude Code

https://claude.ai/code/session_01AKBJRT9j5APTyKyZtj8f23

sidick and others added 2 commits September 16, 2026 17:40
`--sanitize`'s heap detectors only ever poisoned AllocMem, AllocVec and
AllocPooled. But three other guest-visible APIs allocate from the very
same GuestHeap and hand the guest a block it can overrun:

  utility.library  AllocateTagItems / FreeTagItems
  dos.library      AllocDosObject / FreeDosObject
  exec.library     CreateIORequest / DeleteIORequest
                   CreateMsgPort / DeleteMsgPort

Because they go through GuestHeap::alloc, redzone *space* was already
being reserved for them whenever --sanitize was on -- the guard bytes
existed, nothing marked them poisoned. So a guest overrunning a
FileInfoBlock, an RDArgs, a MsgPort or a TagItem array that volamos
handed it went entirely unreported. Sharing execmem's existing helpers
(now pub(crate)) is the whole fix; no per-module reimplementation.

Ordering follows the rule established in #80 at every site: poison
*after* the handler's own zeroing and field initialisation, never
before, because those writes go through the checked path and heal
shadow bytes -- poisoning first would make each handler report a
violation against its own setup. Each call passes
`mark_data_uninit = false` for the same reason: these blocks are zeroed
or field-initialised by the time they are handed over, so they are
genuinely initialised, and marking them uninit would produce false
uninitialized-read reports under --sanitize-uninit.

CreateIORequest is poisoned in its handler rather than in the
create_io_request free function, which takes heap/mem separately and so
has no HandlerContext to reach the shadow map through.

Tests cover both halves: redzones present either side of an
AllocateTagItems block (and the block itself still reading as
initialised), and a freed block reading as Unaddressable with
PoisonReason::Freed specifically, rather than as a redzone.

Not covered, and tracked in #83: exec.library/Allocate, where the guest
owns the MemHeader, so guard space cannot be reserved without touching
the guest's own accounting. That is where C malloc lives. AllocAbs,
AllocEntry and AllocVecPooled are in the LVO table with no handlers, so
there is nothing to miss yet -- they should get poisoning when they get
implemented.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01AKBJRT9j5APTyKyZtj8f23
@sidick

sidick commented Sep 16, 2026

Copy link
Copy Markdown
Owner Author

Correction, and the sweep finding is a true positive

I mis-attributed the PhxAss violation in the description above — it is caused by this PR, not by #82. My bisect was wrong: the "does it reproduce on main" check piped the build output away and I never verified the rebuild, so I was testing a stale binary. Re-run properly as a matrix, it is unambiguous:

binary clean work dir cluttered work dir
main (0d37bc5) 0 0
this branch 1 1

Deterministic across repeated runs and independent of the work directory. Apologies for the noise; #82 is clean.

What the violation actually is

It is a genuine out-of-bounds read, which is the whole point of the feature — this PR made it visible by poisoning CreateIORequest blocks, which nothing did before.

Traced with temporary instrumentation (since removed):

CreateMsgPort:    user=0x36f5c req=34 aligned=36 block=[0x36f3c,0x36fa0)
CreateIORequest:  user=0x36fc0 req=40 aligned=40 block=[0x36fa0,0x37008)
violation:        InvalidRead 2B at 0x36fe8  pc=0x483e

0x36fe8 is exactly 0x36fc0 + 40 — the first byte of the trailing redzone of the IORequest PhxAss asked for. PhxAss requests 40 bytes, i.e. sizeof(struct timerequest) (a 32-byte IORequest plus an 8-byte timeval), and then something reads two bytes one past the end of it.

It is not volamos's own read. run_io_request receives ioreq=0x36fc0 and touches only +20 (io_Device), +28 (io_Command), +32/+36 (tv_secs/tv_micro) — every one inside the 40 bytes. The violation is recorded after DoIO returns, attributed to PC 0x483e, which is PhxAss's own code.

So PhxAss reads past its own timerequest. On real hardware that read lands in whatever follows the block on the heap and nobody notices, which is exactly the class of latent bug --sanitize exists to surface. It is benign in practice — PhxAss assembles correctly, and its output is byte-identical with and without --sanitize.

Consequence for this PR

None, beyond the correction: the detector is working as designed on a real binary. Worth noting for anyone running the corpus sweep in future, though, that PhxAss is no longer silent under --sanitize — and that this is a finding rather than a regression. --sanitize-ignore-pc 0x483e silences it for anyone who wants a clean baseline.

I have not chased down which PhxAss source construct produces the over-read; that would need disassembly around 0x483e (the instruction there decodes as a MOVEM.L (d16,A1)-shaped word, which does not obviously match a 2-byte access, so the exact instruction wants confirming rather than guessing).

@sidick
sidick merged commit 9e98ed3 into main Sep 16, 2026
9 checks passed
@sidick
sidick deleted the poison-all-allocators branch September 16, 2026 17:59
sidick added a commit that referenced this pull request Sep 16, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant