Repository navigation
sanitize: extend heap detectors to exec.library Allocate/Deallocate (#83, Tier 1) - #166
Merged
Merged
Conversation
, Tier 1) A successful Allocate now marks its returned block Uninit in the sanitizer shadow map, and Deallocate marks the freed block Unaddressable with PoisonReason::Freed -- the same states execmem's poison_allocation_edges/ poison_freed_block already apply to AllocMem/AllocVec. This matters because a C program's malloc (libnix and newlib alike) takes a few big blocks via AllocVec and then sub-allocates from its own pool with Allocate, so most of a real C program's heap traffic was previously invisible to --sanitize. Unlike AllocMem's free list (host-side, invisible to the shadow map), Allocate/Deallocate's free list lives in guest memory and is read/written through the same checked AddressSpace path a guest access uses. Poisoning a freed chunk's entire extent naively would poison its own live mc_Next/mc_Bytes header too, and the very next Allocate/Deallocate call that walks past it would then trip a violation against the allocator's own bookkeeping -- not a guest bug. Fixed by always keeping a MemChunk header that remains (or becomes) part of the live free list shadow-Valid, via a new exempt_chunk_header helper, called before any checked write to such a header (a checked write to an already-poisoned byte is recorded as a violation, not healed). A dedicated test (malloc_pattern_allocate_free_then_reallocate_smaller_causes_no_false_positive) exercises exactly this and caught a real ordering bug during development (exemption was originally applied after the write instead of before). Tier 2 (redzones around the returned block, to catch overruns) is explicitly not attempted here -- it would change the guest-visible pool layout, a separate, bigger decision per the issue. No real m68k-amigaos-gcc/sc toolchain was available in this environment, so the C-fixture and false-positive sweep against real gcc-built/pLhA/ PhxAss/sc binaries from the issue's checklist were not run; the new trap-dispatch end-to-end tests are the substitute coverage here. 1109 tests pass workspace-wide (was 1102 on main). Co-Authored-By: Claude Sonnet 5 <[email protected]> Claude-Session: https://claude.ai/code/session_01Godbk9UDW5obqYsdkXkyRR
Co-Authored-By: Claude Sonnet 5 <[email protected]> Claude-Session: https://claude.ai/code/session_01Godbk9UDW5obqYsdkXkyRR
5 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes the biggest gap flagged in #83:
--sanitize's heap detectors coveredAllocMem/AllocVec/AllocPooledbut notexec.library'sAllocate/Deallocate-- which is where a C program'smallocactually lives (libnix and newlib both take a few big blocks viaAllocVec, then sub-allocate from their own pool withAllocate). This is Tier 1 only, exactly as #83 scoped it:Tier 2 (redzones, overrun detection) is explicitly not attempted here. This PR does not close #83 -- only its Tier 1 checklist items.
Confirmed against the NDK
exec.docautodoc entry forAllocate: it takes onlymemHeader/byteSize(norequirements/MEMF_*flag at all), and itsRESULTnever describes the returned block as cleared -- so unlikeAllocMemthere's noMEMF_CLEARcase to special-case; every successfulAllocateisUninit.The wrinkle, and how it's handled
AllocMem's free list lives entirely in host memory (GuestHeap), invisible to the shadow map.Allocate/Deallocate's free list is the opposite:mc_Next/mc_Byteslive in guest memory, read/written through the exact same checkedAddressSpacepath a guest access uses. Naively poisoning a freed chunk's whole extent therefore poisons its own live 8-byte header too -- and since a poisoned byte never heals on a checked write (onlyUninitdoes), the very nextAllocate/Deallocatecall that walks past that chunk (to read its size, or rewrite its header while splitting/coalescing it) would report a violation against the allocator's own bookkeeping, not a guest bug. This is an everyday pattern (malloc(n); free(p); malloc(m)withm < n), not a corner case -- I caught this for real during development via a dedicated regression test that failed on the first attempt.Fixed while staying within Tier 1's "no layout change" scope: a
MemChunkheader that remains (or becomes) part of the live free list is always kept shadow-Valid, via a newexempt_chunk_headerhelper, called before any checked write to such a header. Seecrates/volamos-core/src/execchunk.rs's module doc ("Sanitizer interaction" section) for the full reasoning, including the resulting documented scope limitation: a freed block that stays a standalone free-list node keeps its own first 8 bytesValidrather thanFreeduntil it's merged or reallocated (a UAF into exactly those 8 bytes in that window goes undetected). The tests exercise UAF/uninit-read well inside a block's body, which is where this fix's real value is.Item 4 from the task brief: the fragmentation-leftover-chunk-header question
Verified against the actual code, and the original suspicion ("no special treatment needed") turned out to be wrong for one case:
allocate_handler's leftover-chunk split writes a new header into memory that can land inside a region an earlierDeallocatepoisoned wholesale. Fixed by havingexempt_chunk_headerrun before that write (not after -- a checked write to poisoned bytes is recorded as a violation, never healed). This is exactly the false positive themalloc_pattern_allocate_free_then_reallocate_smaller_causes_no_false_positivetest below was built to catch, and it did, on the first implementation attempt (the exemption call was originally placed after the write).Testing
allocate_marks_the_returned_block_uninit_when_sanitized/allocate_does_not_touch_the_shadow_map_when_sanitize_is_offdeallocate_marks_the_freed_blocks_body_unaddressable_as_freeddeallocate_leaves_a_standalone_chunks_own_header_valid(documents the Tier 1 limitation above)a_real_use_after_free_through_allocate_is_reported-- real trap dispatch,Allocate->Deallocate-> guest read through the dangling pointer, assertsViolationKind::InvalidRead/PoisonReason::Freedan_uninitialized_read_through_allocate_is_reported_when_enabled-- real trap dispatch, freshAllocate-> guest read before any write, withShadowMap::report_uniniton, assertsViolationKind::UninitReadmalloc_pattern_allocate_free_then_reallocate_smaller_causes_no_false_positive-- the regression test described aboveexecchunk.rs's existing tests broke (allocate_exact_fit_unlinks_the_only_chunkand siblings all still pass; they don't enable the sanitizer, so the new code is a no-op for them either way)cargo fmt --all --check,cargo clippy --workspace --all-targets -- -D warnings, andcargo test --workspaceare all clean. 1109 tests pass (was 1102 onmainbefore this change -- +7 new).Deliberately not attempted (documented gap for the human reviewer)
No real
m68k-amigaos-gcc/sctoolchain is available in this sandboxed environment, so two of #83's checklist items were not run:mallocuse-after-free/uninitialized-read via a real compiled binary.sc.The real-trap-dispatch end-to-end tests above substitute for proving the mechanism works, but someone with the real toolchain available should still run the gcc/pLhA/PhxAss/
scsweep before this is considered fully verified in the wild.Docs
Updated
userdocs/CLI-Reference.md's--sanitizesection and added auserdocs/Changelog.mdentry noting the new coverage and the no-redzones-yet limitation.Not merging -- leaving this for review per #83's own multi-item checklist (Tier 2, the real-toolchain sweep, and the "which allocator does
--sanitizecover" doc wording are all still open).