From 1159eed4de1c30e508d1b546c882dcea461829b3 Mon Sep 17 00:00:00 2001 From: Bernie Innocenti Date: Mon, 28 Sep 2026 07:32:57 +0800 Subject: [PATCH 1/2] sanitize: clear the freed-block poison when a block is reallocated A block that FreeVec poisoned keeps that marking when the heap hands the same addresses back out, so the allocating handler's own header write or zeroing, and then every write the guest makes, are all reported as use-after-free. Writing over the block does not clear it either: check_write_byte heals Uninit bytes but records and keeps Unaddressable ones. Clear the shadow where the block is handed out, before the handler writes anything into it. AddressSpace::clear_fresh_block does it, so the allocators that only hold a &mut dyn AddressSpace can call it too. BenchWork's "backdrop lha-pack" under --sanitize went from 6 minutes and 691416 violations to 10 seconds and none. Fixes #95 Co-Authored-By: Claude Opus 5 (1M context) --- crates/volamos-core/src/dosfile.rs | 1 + crates/volamos-core/src/execlist.rs | 2 + crates/volamos-core/src/execmem.rs | 82 +++++++++++++++++++++++++++-- crates/volamos-core/src/memory.rs | 21 ++++++++ crates/volamos-core/src/utility.rs | 1 + 5 files changed, 102 insertions(+), 5 deletions(-) diff --git a/crates/volamos-core/src/dosfile.rs b/crates/volamos-core/src/dosfile.rs index 39d0640..cbacf3f 100644 --- a/crates/volamos-core/src/dosfile.rs +++ b/crates/volamos-core/src/dosfile.rs @@ -1369,6 +1369,7 @@ fn alloc_dos_object_handler(ctx: &mut HandlerContext<'_, C>) -> Result<( "AllocDosObject(type={object_type}): guest heap allocation failed: {e}" ), })?; + ctx.mem.clear_fresh_block(addr, struct_size); for i in 0..struct_size { ctx.mem.write_u8(addr.wrapping_add(i), 0); } diff --git a/crates/volamos-core/src/execlist.rs b/crates/volamos-core/src/execlist.rs index 3ec0d60..98ec33b 100644 --- a/crates/volamos-core/src/execlist.rs +++ b/crates/volamos-core/src/execlist.rs @@ -505,6 +505,7 @@ fn create_msg_port_handler(ctx: &mut HandlerContext<'_, C>) -> Result<() return Ok(()); } }; + ctx.mem.clear_fresh_block(port, MSGPORT_SIZE); init_msg_port_fields(ctx.mem, port, ctx.current_task); // After the field initialisation, never before: those writes heal // shadow bytes, so poisoning first would flag this handler's own @@ -591,6 +592,7 @@ fn create_io_request( size: u32, ) -> Option { let addr = heap.alloc(size).ok()?; + mem.clear_fresh_block(addr, size); for i in 0..size { mem.write_u8(addr.wrapping_add(i), 0); } diff --git a/crates/volamos-core/src/execmem.rs b/crates/volamos-core/src/execmem.rs index 0030d58..1fe9772 100644 --- a/crates/volamos-core/src/execmem.rs +++ b/crates/volamos-core/src/execmem.rs @@ -309,6 +309,7 @@ fn alloc_mem_handler(ctx: &mut HandlerContext<'_, C>) -> Result<(), Disp // knows the unrounded number. match ctx.heap.alloc_with_requested(rounded, byte_size) { Ok(addr) => { + ctx.mem.clear_fresh_block(addr, rounded); let cleared = requirements & MEMF_CLEAR != 0; if cleared { for i in 0..rounded { @@ -421,6 +422,7 @@ fn alloc_vec_handler(ctx: &mut HandlerContext<'_, C>) -> Result<(), Disp let total = user_rounded + ALLOCVEC_HEADER_SIZE; match ctx.heap.alloc(total) { Ok(block) => { + ctx.mem.clear_fresh_block(block, total); // Header: total block size (u32) followed by 4 bytes of // reserved padding (see the module docs). ctx.mem.write_u32(block, total); @@ -445,12 +447,22 @@ fn alloc_vec_handler(ctx: &mut HandlerContext<'_, C>) -> Result<(), Disp // included -- the header is ours, not the guest's, but it // sits inside the same heap block, so the redzones bracket // both). Done after the header write and the MEMF_CLEAR - // loop above, per `poison_allocation_edges`' doc. The data - // bytes are left un-marked rather than marked uninit: the - // block address the heap knows is `block`, not `user_ptr`, - // so an uninit range starting at `block` would wrongly - // cover the header this handler just wrote. + // loop above, per `poison_allocation_edges`' doc. `false` + // because the data range cannot be described from `block`: + // that is what the heap knows, so an uninit range starting + // there would cover the header this handler just wrote. poison_allocation_edges(ctx, block, false); + // Which leaves the guest's own range marked valid by the + // call above -- right for the header, but it loses the + // uninitialized state of the part the guest owns. Mark that + // from `user_ptr`, the one address that excludes the header. + if let Some(shadow) = ctx.mem.shadow_mut() { + if cleared { + shadow.mark_valid(user_ptr, user_rounded); + } else { + shadow.mark_uninit(user_ptr, user_rounded); + } + } ctx.cpu.set_data_register(DataRegister(0), user_ptr); } Err(_) => { @@ -532,6 +544,7 @@ fn create_pool_handler(ctx: &mut HandlerContext<'_, C>) -> Result<(), Di match ctx.heap.alloc(POOL_HEADER_SIZE) { Ok(pool) => { + ctx.mem.clear_fresh_block(pool, POOL_HEADER_SIZE); ctx.mem.write_u32(pool, requirements); ctx.cpu.set_data_register(DataRegister(0), pool); } @@ -590,6 +603,7 @@ fn alloc_pooled_handler(ctx: &mut HandlerContext<'_, C>) -> Result<(), D let rounded = round_up_8(byte_size); match ctx.heap.alloc_with_requested(rounded, byte_size) { Ok(addr) => { + ctx.mem.clear_fresh_block(addr, rounded); let cleared = requirements & MEMF_CLEAR != 0; if cleared { for i in 0..rounded { @@ -969,6 +983,64 @@ mod tests { ); } + #[test] + fn a_recycled_alloc_vec_block_is_no_longer_a_freed_block() { + // Issue #95: FreeVec poisons the block, and the heap hands the + // same addresses back out once the free quarantine releases + // them. If the allocation path leaves that poison in place, + // both the handler's own AllocVec header write and every write + // the guest makes into its brand-new buffer are reported as + // use-after-free -- and at one violation per byte the run slows + // to a crawl. + let mut words = Vec::new(); + words.push(move_imm_to_d(0)); // D0 = byteSize + words.push(0); + words.push(64); + words.push(move_imm_to_d(1)); // D1 = 0, no MEMF_CLEAR + words.push(0); + words.push(0); + words.extend_from_slice(&jsr_disp16_a6(-684)); // AllocVec + words.push(0x2240); // movea.l d0,a1 (A1 = the block to free) + words.extend_from_slice(&jsr_disp16_a6(-690)); // FreeVec + words.push(move_imm_to_d(0)); // same size again + words.push(0); + words.push(64); + words.push(move_imm_to_d(1)); + words.push(0); + words.push(0); + words.extend_from_slice(&jsr_disp16_a6(-684)); // AllocVec + words.push(0x2040); // movea.l d0,a0 + words.push(0x10bc); // move.b #1,(a0) -- the guest's first write + words.push(0x0001); + words.push(RTS); + + let mut rt = program(&words); + rt.memory_mut().enable_sanitizer(); + rt.enable_heap_sanitizer(); + // No quarantine, so the second AllocVec is handed the block the + // FreeVec just poisoned -- the whole point of the test. With the + // default budget a block this small is held back instead, and + // the run never recycles anything. + rt.heap_mut().set_quarantine_budget(0); + let mut out = Vec::new(); + let code = rt.run(&mut out, None).expect("run should succeed"); + let addr = code as u32; + + assert_ne!(addr, 0, "both allocations must succeed for this to test anything"); + let shadow = rt.memory().shadow().expect("sanitizer was enabled above"); + assert_eq!( + shadow.violation_count(), + 0, + "writing into a freshly allocated block must not be reported, \ + whatever that memory was used for before" + ); + assert_ne!( + shadow.state(addr), + crate::sanitize::ShadowState::Unaddressable, + "the recycled block must not still be marked as freed" + ); + } + #[test] fn alloc_mem_with_memf_clear_zeroes_and_alloc_free_round_trips() { let mut words = Vec::new(); diff --git a/crates/volamos-core/src/memory.rs b/crates/volamos-core/src/memory.rs index 392aa79..2817e7d 100644 --- a/crates/volamos-core/src/memory.rs +++ b/crates/volamos-core/src/memory.rs @@ -50,6 +50,21 @@ pub trait AddressSpace { /// Writes a single byte. Silently ignored if `addr` is out of range. fn write_u8(&mut self, addr: u32, value: u8); + /// Clears any sanitizer poison over `[addr, addr + len)`, a block the + /// guest heap has just handed out. + /// + /// Call this as soon as an allocation succeeds, *before* the handler + /// writes a header or zeroes the block: the heap recycles addresses, + /// so those bytes may still carry the `PoisonReason::Freed` marking + /// of the previous allocation, and writing over them does not clear + /// it -- the shadow map heals `Uninit` bytes but keeps + /// `Unaddressable` ones. Without this, the handler's own + /// initialization is reported as a use-after-free, and so is every + /// write the guest then makes. + /// + /// Default: nothing to do, for address spaces with no shadow map. + fn clear_fresh_block(&mut self, _addr: u32, _len: u32) {} + /// Reads a big-endian 16-bit value. Any byte that falls out of range /// reads as `0`. fn read_u16(&self, addr: u32) -> u16 { @@ -224,6 +239,12 @@ impl AddressSpace for FlatMemory { self.bytes.get(addr as usize).copied().unwrap_or(0) } + fn clear_fresh_block(&mut self, addr: u32, len: u32) { + if let Some(shadow) = &mut self.shadow { + shadow.mark_valid(addr, len); + } + } + fn write_u8(&mut self, addr: u32, value: u8) { if let Some(shadow) = &mut self.shadow { shadow.check_write(addr, 1); diff --git a/crates/volamos-core/src/utility.rs b/crates/volamos-core/src/utility.rs index 7698dfd..fe179a6 100644 --- a/crates/volamos-core/src/utility.rs +++ b/crates/volamos-core/src/utility.rs @@ -276,6 +276,7 @@ fn allocate_tag_items_handler( let byte_size = num_tags.saturating_mul(TAG_ITEM_SIZE); match ctx.heap.alloc(byte_size) { Ok(addr) => { + ctx.mem.clear_fresh_block(addr, byte_size); for i in 0..byte_size { ctx.mem.write_u8(addr.wrapping_add(i), 0); } From a8c62df57bfffde11e6ff04379295a9287b1e20b Mon Sep 17 00:00:00 2001 From: Simon Dick Date: Mon, 28 Sep 2026 06:30:58 +0100 Subject: [PATCH 2/2] sanitize: clear recycled-block poison in the heap, not per handler Completes the #95 fix. Two things were left over. **The clear reached only 8 of ~48 allocation sites.** Every one of them carves guest structures out of the same GuestHeap, so any of them can be handed a block FreeMem/FreeVec/FreePooled poisoned -- not just the exec allocators. A guest that frees a block larger than the 64 KiB quarantine budget (released immediately rather than held back) and then opens a file reproduces the original symptom against the FileHandle/MsgPort that dos Open carves out: 46 spurious "invalid write (freed block)" reports, one per byte, attributed to the guest's PC. So the clear moves to GuestHeap::alloc/alloc_with_requested, the one point where a block leaves the heap's control, and they now take the address space as a parameter -- which makes the invariant a compile error to forget rather than a convention to remember. The whole block is cleared, redzones included; callers that want redzone protection re-poison via poison_allocation_edges afterwards, as they already had to. The allocator's own placement/coalescing tests keep a module-private `alloc_bare` shorthand, since they have no address space in play. **The wall-clock half of #95 was not the dedup hash.** check_read and check_write detected "did that byte report?" by comparing violation_count() before and after -- and violation_count *sums* hits across the whole log, so once the log filled, every byte of every violating access walked 1000 entries twice. The byte checkers now return that answer directly. On a 400,000-byte use-after-free loop: 1.33s before, 0.03s after, with byte-identical output. Verified: the #95 repro pair, the Open-after-free case above, and the whole fixture corpus are clean; sanitizer output is byte-identical to main for PhxAss (--sanitize, --sanitize-uninit, with and without --cpu 68020) and for a real SAS/C `sc hello.c`; a genuine use-after-free is still reported (new regression test, alongside one for the recycled block at the heap level). cargo test 1010 passed, clippy and fmt clean. Co-Authored-By: Claude Opus 5 (1M context) --- crates/volamos-core/src/bsdsocket.rs | 108 ++++++------- crates/volamos-core/src/dispatch.rs | 20 +-- crates/volamos-core/src/dosanchor.rs | 4 +- crates/volamos-core/src/dosargs.rs | 16 +- crates/volamos-core/src/dosdevlist.rs | 6 +- crates/volamos-core/src/dosdevproc.rs | 2 +- crates/volamos-core/src/dosfile.rs | 27 ++-- crates/volamos-core/src/doslock.rs | 2 +- crates/volamos-core/src/dosseg.rs | 8 +- crates/volamos-core/src/execlib.rs | 4 +- crates/volamos-core/src/execlist.rs | 6 +- crates/volamos-core/src/execmem.rs | 87 ++++++++--- crates/volamos-core/src/exectask.rs | 8 +- crates/volamos-core/src/graphics.rs | 4 +- crates/volamos-core/src/guestmem.rs | 214 ++++++++++++++++++++------ crates/volamos-core/src/intuition.rs | 10 +- crates/volamos-core/src/locale.rs | 2 +- crates/volamos-core/src/memory.rs | 20 +-- crates/volamos-core/src/sanitize.rs | 72 ++++++--- crates/volamos-core/src/utility.rs | 3 +- 20 files changed, 408 insertions(+), 215 deletions(-) diff --git a/crates/volamos-core/src/bsdsocket.rs b/crates/volamos-core/src/bsdsocket.rs index e409c0e..5339c8d 100644 --- a/crates/volamos-core/src/bsdsocket.rs +++ b/crates/volamos-core/src/bsdsocket.rs @@ -1951,15 +1951,15 @@ fn inet_ntoa_handler(ctx: &mut HandlerContext<'_, C>) -> Result<(), Disp None => { // "###.###.###.###\0" -- 16 bytes is enough for any IPv4 // dotted-decimal string plus its NUL terminator. - let addr = ctx - .heap - .alloc(16) - .map_err(|e| DispatchError::HandlerFailed { - library: "bsdsocket.library".to_string(), - lvo: -174, - handler_name: "Inet_NtoA".to_string(), - message: format!("Inet_NtoA: guest heap allocation failed: {e}"), - })?; + let addr = + ctx.heap + .alloc(&mut *ctx.mem, 16) + .map_err(|e| DispatchError::HandlerFailed { + library: "bsdsocket.library".to_string(), + lvo: -174, + handler_name: "Inet_NtoA".to_string(), + message: format!("Inet_NtoA: guest heap allocation failed: {e}"), + })?; ctx.bsdsocket.ntoa_buf = Some(addr); addr } @@ -2097,15 +2097,14 @@ fn build_hostent( let mut allocs = Vec::new(); let mut alloc = |ctx: &mut HandlerContext<'_, C>, size: u32| -> Result { - let addr = ctx - .heap - .alloc(size.max(4)) - .map_err(|e| DispatchError::HandlerFailed { + let addr = ctx.heap.alloc(&mut *ctx.mem, size.max(4)).map_err(|e| { + DispatchError::HandlerFailed { library: "bsdsocket.library".to_string(), lvo, handler_name: handler_name.to_string(), message: format!("{handler_name}: guest heap allocation failed: {e}"), - })?; + } + })?; allocs.push(addr); Ok(addr) }; @@ -2325,15 +2324,14 @@ fn build_servent( } let mut allocs = Vec::new(); let mut alloc = |ctx: &mut HandlerContext<'_, C>, size: u32| -> Result { - let addr = ctx - .heap - .alloc(size.max(4)) - .map_err(|e| DispatchError::HandlerFailed { + let addr = ctx.heap.alloc(&mut *ctx.mem, size.max(4)).map_err(|e| { + DispatchError::HandlerFailed { library: "bsdsocket.library".to_string(), lvo, handler_name: handler_name.to_string(), message: format!("{handler_name}: guest heap allocation failed: {e}"), - })?; + } + })?; allocs.push(addr); Ok(addr) }; @@ -2459,15 +2457,14 @@ fn build_protoent( } let mut allocs = Vec::new(); let mut alloc = |ctx: &mut HandlerContext<'_, C>, size: u32| -> Result { - let addr = ctx - .heap - .alloc(size.max(4)) - .map_err(|e| DispatchError::HandlerFailed { + let addr = ctx.heap.alloc(&mut *ctx.mem, size.max(4)).map_err(|e| { + DispatchError::HandlerFailed { library: "bsdsocket.library".to_string(), lvo, handler_name: handler_name.to_string(), message: format!("{handler_name}: guest heap allocation failed: {e}"), - })?; + } + })?; allocs.push(addr); Ok(addr) }; @@ -2701,17 +2698,16 @@ fn socket_base_tag_list_handler( let ptr = match ctx.bsdsocket.errno_ptr { Some(ptr) => ptr, None => { - let addr = - ctx.heap - .alloc(4) - .map_err(|e| DispatchError::HandlerFailed { - library: "bsdsocket.library".to_string(), - lvo: -294, - handler_name: "SocketBaseTagList".to_string(), - message: format!( - "SocketBaseTagList: guest heap allocation failed: {e}" - ), - })?; + let addr = ctx.heap.alloc(&mut *ctx.mem, 4).map_err(|e| { + DispatchError::HandlerFailed { + library: "bsdsocket.library".to_string(), + lvo: -294, + handler_name: "SocketBaseTagList".to_string(), + message: format!( + "SocketBaseTagList: guest heap allocation failed: {e}" + ), + } + })?; ctx.mem.write_u32(addr, ctx.bsdsocket.last_errno as u32); ctx.bsdsocket.errno_ptr = Some(addr); addr @@ -2733,17 +2729,16 @@ fn socket_base_tag_list_handler( let ptr = match ctx.bsdsocket.herrno_ptr { Some(ptr) => ptr, None => { - let addr = - ctx.heap - .alloc(4) - .map_err(|e| DispatchError::HandlerFailed { - library: "bsdsocket.library".to_string(), - lvo: -294, - handler_name: "SocketBaseTagList".to_string(), - message: format!( - "SocketBaseTagList: guest heap allocation failed: {e}" - ), - })?; + let addr = ctx.heap.alloc(&mut *ctx.mem, 4).map_err(|e| { + DispatchError::HandlerFailed { + library: "bsdsocket.library".to_string(), + lvo: -294, + handler_name: "SocketBaseTagList".to_string(), + message: format!( + "SocketBaseTagList: guest heap allocation failed: {e}" + ), + } + })?; ctx.mem.write_u32(addr, ctx.bsdsocket.last_herrno as u32); ctx.bsdsocket.herrno_ptr = Some(addr); addr @@ -2795,17 +2790,16 @@ fn socket_base_tag_list_handler( let buf = match ctx.bsdsocket.release_str_buf { Some(addr) => addr, None => { - let addr = - ctx.heap - .alloc(32) - .map_err(|e| DispatchError::HandlerFailed { - library: "bsdsocket.library".to_string(), - lvo: -294, - handler_name: "SocketBaseTagList".to_string(), - message: format!( - "SocketBaseTagList: guest heap allocation failed: {e}" - ), - })?; + let addr = ctx.heap.alloc(&mut *ctx.mem, 32).map_err(|e| { + DispatchError::HandlerFailed { + library: "bsdsocket.library".to_string(), + lvo: -294, + handler_name: "SocketBaseTagList".to_string(), + message: format!( + "SocketBaseTagList: guest heap allocation failed: {e}" + ), + } + })?; crate::guestmem::write_c_string( ctx.mem, addr, diff --git a/crates/volamos-core/src/dispatch.rs b/crates/volamos-core/src/dispatch.rs index f8cf4f9..5f8cba9 100644 --- a/crates/volamos-core/src/dispatch.rs +++ b/crates/volamos-core/src/dispatch.rs @@ -1568,15 +1568,15 @@ fn open_library_common( } let size = FAKE_LIB_JUMP_TABLE_SIZE; - let block = - ctx.heap - .alloc(size + LIB_STRUCT_SIZE) - .map_err(|e| DispatchError::HandlerFailed { - library: "exec.library".to_string(), - lvo: -552, - handler_name: "OpenLibrary".to_string(), - message: format!("couldn't auto-create fake library {name:?}: {e}"), - })?; + let block = ctx + .heap + .alloc(&mut *ctx.mem, size + LIB_STRUCT_SIZE) + .map_err(|e| DispatchError::HandlerFailed { + library: "exec.library".to_string(), + lvo: -552, + handler_name: "OpenLibrary".to_string(), + message: format!("couldn't auto-create fake library {name:?}: {e}"), + })?; let base = block.wrapping_add(size); let mut addr = block; @@ -2330,7 +2330,7 @@ impl Runtime { }; let line_len = line.len() as u32; let args_addr = heap - .alloc(line_len + 1) + .alloc(&mut mem, line_len + 1) .expect("guest heap has room for the command-line buffer"); { let mut a = args_addr; diff --git a/crates/volamos-core/src/dosanchor.rs b/crates/volamos-core/src/dosanchor.rs index c52c4bb..206159d 100644 --- a/crates/volamos-core/src/dosanchor.rs +++ b/crates/volamos-core/src/dosanchor.rs @@ -248,7 +248,7 @@ fn set_flag_bit(mem: &mut dyn AddressSpace, ap_addr: u32, bit: u8, set: bool) { fn alloc_achain(heap: &mut GuestHeap, mem: &mut dyn AddressSpace) -> Result { let addr = heap - .alloc(ACHAIN_SIZE) + .alloc(&mut *mem, ACHAIN_SIZE) .map_err(|_| crate::dosfile::ERROR_NO_FREE_STORE)?; for i in 0..ACHAIN_SIZE { mem.write_u8(addr + i, 0); @@ -825,7 +825,7 @@ mod tests { /// `ap_Buf` tail, setting `ap_Strlen` accordingly. fn alloc_ap(heap: &mut GuestHeap, mem: &mut FlatMemory, strlen: u16) -> u32 { let size = AP_BUF_OFFSET + u32::from(strlen); - let addr = heap.alloc(size).unwrap(); + let addr = heap.alloc(&mut *mem, size).unwrap(); for i in 0..size { mem.write_u8(addr + i, 0); } diff --git a/crates/volamos-core/src/dosargs.rs b/crates/volamos-core/src/dosargs.rs index 552ddf5..dfe8c0e 100644 --- a/crates/volamos-core/src/dosargs.rs +++ b/crates/volamos-core/src/dosargs.rs @@ -483,7 +483,7 @@ fn alloc_c_string( allocations: &mut Vec, bytes: &[u8], ) -> Result { - let addr = heap.alloc(bytes.len() as u32 + 1)?; + let addr = heap.alloc(&mut *mem, bytes.len() as u32 + 1)?; allocations.push(addr); write_c_string(mem, addr, bytes); Ok(addr) @@ -508,7 +508,7 @@ fn materialize( ArgKind::Switch | ArgKind::Toggle => u32::from(slot.bool_val), ArgKind::Number => match slot.number { Some(n) => { - let addr = heap.alloc(4)?; + let addr = heap.alloc(&mut *mem, 4)?; allocations.push(addr); mem.write_u32(addr, n as u32); addr @@ -528,7 +528,7 @@ fn materialize( ptrs.push(alloc_c_string(heap, mem, &mut allocations, item)?); } ptrs.push(0); - let table_addr = heap.alloc(ptrs.len() as u32 * 4)?; + let table_addr = heap.alloc(&mut *mem, ptrs.len() as u32 * 4)?; allocations.push(table_addr); for (j, ptr) in ptrs.iter().enumerate() { mem.write_u32(table_addr.wrapping_add((j as u32) * 4), *ptr); @@ -583,7 +583,7 @@ fn read_args( let (anchor_addr, owns_anchor) = if rdargs_ptr != 0 { (rdargs_ptr, false) } else { - let addr = heap.alloc(RDARGS_ANCHOR_SIZE).map_err(|_| { + let addr = heap.alloc(&mut *mem, RDARGS_ANCHOR_SIZE).map_err(|_| { for a in &allocations { let _ = heap.free(*a); } @@ -736,7 +736,7 @@ mod tests { let mut heap = GuestHeap::new(0x100, 0x3000); let mut line = cmdline.join(" ").into_bytes(); line.push(b'\n'); - let addr = heap.alloc(line.len() as u32).unwrap(); + let addr = heap.alloc(&mut mem, line.len() as u32).unwrap(); for (i, &b) in line.iter().enumerate() { mem.write_u8(addr + i as u32, b); } @@ -754,7 +754,7 @@ mod tests { template: &str, slot_count: u32, ) -> (u32, u32) { - let array_addr = heap.alloc(slot_count * 4).unwrap(); + let array_addr = heap.alloc(&mut *mem, slot_count * 4).unwrap(); for i in 0..slot_count { mem.write_u32(array_addr + i * 4, 0); } @@ -772,7 +772,7 @@ mod tests { template: &str, slot_count: u32, ) -> i32 { - let array_addr = heap.alloc(slot_count * 4).unwrap(); + let array_addr = heap.alloc(&mut *mem, slot_count * 4).unwrap(); for i in 0..slot_count { mem.write_u32(array_addr + i * 4, 0); } @@ -945,7 +945,7 @@ mod tests { #[test] fn caller_supplied_rdargs_anchor_is_not_freed() { let (mut heap, mut mem, mut dos) = setup(&["value"]); - let array_addr = heap.alloc(4).unwrap(); + let array_addr = heap.alloc(&mut mem, 4).unwrap(); mem.write_u32(array_addr, 0); let caller_rdargs = 0x2000; // not a heap allocation; FreeArgs must not touch it let anchor = read_args( diff --git a/crates/volamos-core/src/dosdevlist.rs b/crates/volamos-core/src/dosdevlist.rs index 678e211..49d3852 100644 --- a/crates/volamos-core/src/dosdevlist.rs +++ b/crates/volamos-core/src/dosdevlist.rs @@ -129,7 +129,7 @@ fn lock_dos_list( ) -> u32 { let mut allocated = Vec::new(); let header = heap - .alloc(DOSLIST_SIZE) + .alloc(&mut *mem, DOSLIST_SIZE) .expect("LockDosList: heap exhausted allocating header"); zero_doslist(mem, header); // DLT_PRIVATE, not a real entry type: this node is a scan starting @@ -148,7 +148,7 @@ fn lock_dos_list( .unwrap_or_default(); for name in volumes { let addr = heap - .alloc(DOSLIST_SIZE) + .alloc(&mut *mem, DOSLIST_SIZE) .expect("LockDosList: heap exhausted allocating a DosList entry"); zero_doslist(mem, addr); mem.write_u32(addr + DOL_TYPE_OFFSET, DLT_VOLUME as u32); @@ -163,7 +163,7 @@ fn lock_dos_list( let name_bytes = name.as_bytes(); // +1 length byte, +1 NUL courtesy byte (see module docs). let name_addr = heap - .alloc(name_bytes.len() as u32 + 2) + .alloc(&mut *mem, name_bytes.len() as u32 + 2) .expect("LockDosList: heap exhausted allocating dol_Name"); let written = write_bstr(mem, name_addr, name_bytes); mem.write_u8(name_addr + 1 + written as u32, 0); diff --git a/crates/volamos-core/src/dosdevproc.rs b/crates/volamos-core/src/dosdevproc.rs index a2873ef..a1b81f0 100644 --- a/crates/volamos-core/src/dosdevproc.rs +++ b/crates/volamos-core/src/dosdevproc.rs @@ -97,7 +97,7 @@ fn get_device_proc( }; let lock_bptr = dos.lock(heap, mem, &dir_part, SHARED_LOCK)?; - match heap.alloc(DEVPROC_SIZE) { + match heap.alloc(&mut *mem, DEVPROC_SIZE) { Ok(addr) => { mem.write_u32(addr + DVP_PORT_OFFSET, 0); mem.write_u32(addr + DVP_LOCK_OFFSET, lock_bptr); diff --git a/crates/volamos-core/src/dosfile.rs b/crates/volamos-core/src/dosfile.rs index cbacf3f..03c74b9 100644 --- a/crates/volamos-core/src/dosfile.rs +++ b/crates/volamos-core/src/dosfile.rs @@ -528,7 +528,7 @@ impl DosState { return Ok(addr); } let addr = heap - .alloc(crate::execlist::MSGPORT_SIZE) + .alloc(&mut *mem, crate::execlist::MSGPORT_SIZE) .map_err(|_| ERROR_NO_FREE_STORE)?; for i in 0..crate::execlist::MSGPORT_SIZE { mem.write_u8(addr.wrapping_add(i), 0); @@ -562,7 +562,7 @@ impl DosState { self.fs_port_addr(heap, mem)? }; let addr = heap - .alloc(FILE_HANDLE_SIZE) + .alloc(&mut *mem, FILE_HANDLE_SIZE) .map_err(|_| ERROR_NO_FREE_STORE)?; for i in 0..FILE_HANDLE_SIZE { mem.write_u8(addr.wrapping_add(i), 0); @@ -1358,18 +1358,17 @@ fn alloc_dos_object_handler(ctx: &mut HandlerContext<'_, C>) -> Result<( } }; - let addr = ctx - .heap - .alloc(struct_size) - .map_err(|e| DispatchError::HandlerFailed { - library: "dos.library".to_string(), - lvo: -228, - handler_name: "AllocDosObject".to_string(), - message: format!( - "AllocDosObject(type={object_type}): guest heap allocation failed: {e}" - ), - })?; - ctx.mem.clear_fresh_block(addr, struct_size); + let addr = + ctx.heap + .alloc(&mut *ctx.mem, struct_size) + .map_err(|e| DispatchError::HandlerFailed { + library: "dos.library".to_string(), + lvo: -228, + handler_name: "AllocDosObject".to_string(), + message: format!( + "AllocDosObject(type={object_type}): guest heap allocation failed: {e}" + ), + })?; for i in 0..struct_size { ctx.mem.write_u8(addr.wrapping_add(i), 0); } diff --git a/crates/volamos-core/src/doslock.rs b/crates/volamos-core/src/doslock.rs index 4a4d9ca..0cd2350 100644 --- a/crates/volamos-core/src/doslock.rs +++ b/crates/volamos-core/src/doslock.rs @@ -623,7 +623,7 @@ fn alloc_lock_struct( key: u32, access: i32, ) -> Result { - let addr = heap.alloc(FILE_LOCK_SIZE)?; + let addr = heap.alloc(&mut *mem, FILE_LOCK_SIZE)?; for i in 0..FILE_LOCK_SIZE { mem.write_u8(addr.wrapping_add(i), 0); } diff --git a/crates/volamos-core/src/dosseg.rs b/crates/volamos-core/src/dosseg.rs index e9af6be..5d07af0 100644 --- a/crates/volamos-core/src/dosseg.rs +++ b/crates/volamos-core/src/dosseg.rs @@ -291,7 +291,7 @@ pub(crate) fn build_seglist( let mut alloc_addrs: Vec = Vec::with_capacity(file.hunks.len()); for hunk in &file.hunks { let total = SEG_HEADER_SIZE.wrapping_add(hunk.reserved_size as u32); - match heap.alloc(total) { + match heap.alloc(&mut *mem, total) { Ok(addr) => alloc_addrs.push(addr), Err(e) => { // Unwind: free everything allocated so far for this @@ -432,7 +432,7 @@ fn build_overlay_root_seglist( // (not a BPTR -- the manager indexes into it with byte offsets // directly, per the disassembly-confirmed convention). let ovtab_bytes = overlay.table.raw.len() as u32 * 4; - let ovtab_addr = match heap.alloc(ovtab_bytes.max(4)) { + let ovtab_addr = match heap.alloc(&mut *mem, ovtab_bytes.max(4)) { Ok(a) => a, Err(e) => { for &addr in &seglist.alloc_addrs { @@ -448,7 +448,7 @@ fn build_overlay_root_seglist( // oh_Segments: one BPTR slot per hunk in the whole tree, zeroed for // not-yet-loaded overlay-node hunks, filled in for the root's own. let segments_bytes = overlay.total_hunks as u32 * 4; - let segments_addr = match heap.alloc(segments_bytes.max(4)) { + let segments_addr = match heap.alloc(&mut *mem, segments_bytes.max(4)) { Ok(a) => a, Err(e) => { let _ = heap.free(ovtab_addr); @@ -694,7 +694,7 @@ impl DosState { let mut alloc_addrs: Vec = Vec::with_capacity(node.hunks.len()); for hunk in &node.hunks { let total = SEG_HEADER_SIZE.wrapping_add(hunk.reserved_size as u32); - match heap.alloc(total) { + match heap.alloc(&mut *mem, total) { Ok(addr) => alloc_addrs.push(addr), Err(_) => { for &addr in &alloc_addrs { diff --git a/crates/volamos-core/src/execlib.rs b/crates/volamos-core/src/execlib.rs index c79433f..0be1250 100644 --- a/crates/volamos-core/src/execlib.rs +++ b/crates/volamos-core/src/execlib.rs @@ -460,7 +460,9 @@ pub fn make_library( let negsize = round_up_longword(vectors.len() as u32 * LIB_VECTSIZE); let total = negsize.saturating_add(d_size); - let alloc_addr = heap.alloc(total).map_err(MakeLibraryError::Heap)?; + let alloc_addr = heap + .alloc(&mut *mem, total) + .map_err(MakeLibraryError::Heap)?; let base = alloc_addr.wrapping_add(negsize); for i in 0..d_size { diff --git a/crates/volamos-core/src/execlist.rs b/crates/volamos-core/src/execlist.rs index 98ec33b..46dd2e9 100644 --- a/crates/volamos-core/src/execlist.rs +++ b/crates/volamos-core/src/execlist.rs @@ -495,7 +495,7 @@ fn reply_msg_handler(ctx: &mut HandlerContext<'_, C>) -> Result<(), Disp /// heap is exhausted. See the module docs for the `mp_Flags`/`mp_SigBit`/ /// `mp_SigTask` simplifications. fn create_msg_port_handler(ctx: &mut HandlerContext<'_, C>) -> Result<(), DispatchError> { - let port = match ctx.heap.alloc(MSGPORT_SIZE) { + let port = match ctx.heap.alloc(&mut *ctx.mem, MSGPORT_SIZE) { Ok(addr) => addr, Err(_) => { // Real CreateMsgPort returns NULL on failure (e.g. @@ -505,7 +505,6 @@ fn create_msg_port_handler(ctx: &mut HandlerContext<'_, C>) -> Result<() return Ok(()); } }; - ctx.mem.clear_fresh_block(port, MSGPORT_SIZE); init_msg_port_fields(ctx.mem, port, ctx.current_task); // After the field initialisation, never before: those writes heal // shadow bytes, so poisoning first would flag this handler's own @@ -591,8 +590,7 @@ fn create_io_request( port: u32, size: u32, ) -> Option { - let addr = heap.alloc(size).ok()?; - mem.clear_fresh_block(addr, size); + let addr = heap.alloc(&mut *mem, size).ok()?; for i in 0..size { mem.write_u8(addr.wrapping_add(i), 0); } diff --git a/crates/volamos-core/src/execmem.rs b/crates/volamos-core/src/execmem.rs index 1fe9772..2c7756b 100644 --- a/crates/volamos-core/src/execmem.rs +++ b/crates/volamos-core/src/execmem.rs @@ -307,9 +307,11 @@ fn alloc_mem_handler(ctx: &mut HandlerContext<'_, C>) -> Result<(), Disp // reservation: the difference is alignment slack the guest has no // business touching, and this handler is the last place that still // knows the unrounded number. - match ctx.heap.alloc_with_requested(rounded, byte_size) { + match ctx + .heap + .alloc_with_requested(&mut *ctx.mem, rounded, byte_size) + { Ok(addr) => { - ctx.mem.clear_fresh_block(addr, rounded); let cleared = requirements & MEMF_CLEAR != 0; if cleared { for i in 0..rounded { @@ -420,9 +422,8 @@ fn alloc_vec_handler(ctx: &mut HandlerContext<'_, C>) -> Result<(), Disp let user_rounded = round_up_8(byte_size); let total = user_rounded + ALLOCVEC_HEADER_SIZE; - match ctx.heap.alloc(total) { + match ctx.heap.alloc(&mut *ctx.mem, total) { Ok(block) => { - ctx.mem.clear_fresh_block(block, total); // Header: total block size (u32) followed by 4 bytes of // reserved padding (see the module docs). ctx.mem.write_u32(block, total); @@ -452,16 +453,16 @@ fn alloc_vec_handler(ctx: &mut HandlerContext<'_, C>) -> Result<(), Disp // that is what the heap knows, so an uninit range starting // there would cover the header this handler just wrote. poison_allocation_edges(ctx, block, false); - // Which leaves the guest's own range marked valid by the - // call above -- right for the header, but it loses the - // uninitialized state of the part the guest owns. Mark that - // from `user_ptr`, the one address that excludes the header. - if let Some(shadow) = ctx.mem.shadow_mut() { - if cleared { - shadow.mark_valid(user_ptr, user_rounded); - } else { - shadow.mark_uninit(user_ptr, user_rounded); - } + // Which leaves the guest's own range valid -- `GuestHeap:: + // alloc` cleared the whole block on the way out, which is + // right for the header but loses the uninitialized state of + // the part the guest owns. Mark that from `user_ptr`, the + // one address that excludes the header. (With `MEMF_CLEAR` + // the block genuinely *is* initialized, so valid -- already + // the case -- is the correct state and there is nothing to + // do.) + if !cleared && let Some(shadow) = ctx.mem.shadow_mut() { + shadow.mark_uninit(user_ptr, user_rounded); } ctx.cpu.set_data_register(DataRegister(0), user_ptr); } @@ -542,9 +543,8 @@ fn create_pool_handler(ctx: &mut HandlerContext<'_, C>) -> Result<(), Di // D1 (puddleSize) and D2 (threshSize) are intentionally unused -- // see this handler's doc comment. - match ctx.heap.alloc(POOL_HEADER_SIZE) { + match ctx.heap.alloc(&mut *ctx.mem, POOL_HEADER_SIZE) { Ok(pool) => { - ctx.mem.clear_fresh_block(pool, POOL_HEADER_SIZE); ctx.mem.write_u32(pool, requirements); ctx.cpu.set_data_register(DataRegister(0), pool); } @@ -601,9 +601,11 @@ fn alloc_pooled_handler(ctx: &mut HandlerContext<'_, C>) -> Result<(), D let requirements = ctx.mem.read_u32(pool); let rounded = round_up_8(byte_size); - match ctx.heap.alloc_with_requested(rounded, byte_size) { + match ctx + .heap + .alloc_with_requested(&mut *ctx.mem, rounded, byte_size) + { Ok(addr) => { - ctx.mem.clear_fresh_block(addr, rounded); let cleared = requirements & MEMF_CLEAR != 0; if cleared { for i in 0..rounded { @@ -1026,7 +1028,10 @@ mod tests { let code = rt.run(&mut out, None).expect("run should succeed"); let addr = code as u32; - assert_ne!(addr, 0, "both allocations must succeed for this to test anything"); + assert_ne!( + addr, 0, + "both allocations must succeed for this to test anything" + ); let shadow = rt.memory().shadow().expect("sanitizer was enabled above"); assert_eq!( shadow.violation_count(), @@ -1041,6 +1046,50 @@ mod tests { ); } + #[test] + fn a_genuine_use_after_free_is_still_reported() { + // The guard rail on the issue #95 fix: `GuestHeap::alloc` clears + // the whole block it hands out, so it must be the *handing out* + // that clears the poison and not, say, the free-list bookkeeping + // -- a block that has been freed and not yet reallocated has to + // stay poisoned, or `--sanitize` would have stopped detecting + // use-after-free altogether. 64 bytes is far below the default + // quarantine budget, so this block is held back rather than + // recycled and the dangling pointer really does point at poison. + let mut words = Vec::new(); + words.push(move_imm_to_d(0)); // D0 = byteSize + words.push(0); + words.push(64); + words.push(move_imm_to_d(1)); // D1 = 0, no MEMF_CLEAR + words.push(0); + words.push(0); + words.extend_from_slice(&jsr_disp16_a6(-684)); // AllocVec + words.push(move_d0_to_d(2)); // keep the pointer in D2 + words.push(move_d0_to_a(1)); // A1 = the block to free + words.extend_from_slice(&jsr_disp16_a6(-690)); // FreeVec + words.push(0x2242); // movea.l d2,a1 -- the now-dangling pointer + words.push(0x12bc); // move.b #1,(a1) -- use after free + words.push(0x0001); + words.push(move_d0_to_d(0)); + words.push(RTS); + + let mut rt = program(&words); + rt.memory_mut().enable_sanitizer(); + rt.enable_heap_sanitizer(); + let mut out = Vec::new(); + rt.run(&mut out, None).expect("run should succeed"); + + let shadow = rt.memory().shadow().expect("sanitizer was enabled above"); + assert_eq!( + shadow.violation_count(), + 1, + "writing through a pointer whose block was freed must still be reported" + ); + let violation = &shadow.violations()[0]; + assert_eq!(violation.kind, crate::sanitize::ViolationKind::InvalidWrite); + assert_eq!(violation.reason, Some(crate::sanitize::PoisonReason::Freed)); + } + #[test] fn alloc_mem_with_memf_clear_zeroes_and_alloc_free_round_trips() { let mut words = Vec::new(); diff --git a/crates/volamos-core/src/exectask.rs b/crates/volamos-core/src/exectask.rs index ce37633..2762262 100644 --- a/crates/volamos-core/src/exectask.rs +++ b/crates/volamos-core/src/exectask.rs @@ -505,7 +505,7 @@ pub fn create_current_task( input_addr: u32, ) -> u32 { let task = heap - .alloc(PROCESS_STRUCT_SIZE) + .alloc(&mut *mem, PROCESS_STRUCT_SIZE) .expect("guest heap has room for the fake current task struct"); // Zero the whole struct first -- every field this module doesn't @@ -525,7 +525,7 @@ pub fn create_current_task( // here). mem.write_u8(task + LN_TYPE, NT_TASK); let name_addr = heap - .alloc(PROCESS_NAME.len() as u32 + 1) + .alloc(&mut *mem, PROCESS_NAME.len() as u32 + 1) .expect("guest heap has room for the process name string"); write_c_string(mem, name_addr, PROCESS_NAME); mem.write_u32(task + LN_NAME, name_addr); @@ -547,7 +547,7 @@ pub fn create_current_task( // CommandLineInterface, written as a BPTR -- see PR_CLI_OFFSET's doc // for why this must be non-NULL. let cli_addr = heap - .alloc(CLI_STRUCT_SIZE) + .alloc(&mut *mem, CLI_STRUCT_SIZE) .expect("guest heap has room for the fake CLI struct"); for i in 0..CLI_STRUCT_SIZE { mem.write_u8(cli_addr.wrapping_add(i), 0); @@ -560,7 +560,7 @@ pub fn create_current_task( // data to read instead of NUL/zero bytes. let name_bytes = program_name.as_bytes(); let cli_command_name_addr = heap - .alloc(1 + (name_bytes.len().min(255) as u32)) + .alloc(&mut *mem, 1 + (name_bytes.len().min(255) as u32)) .expect("guest heap has room for the fake cli_CommandName BSTR"); write_bstr(mem, cli_command_name_addr, name_bytes); mem.write_u32( diff --git a/crates/volamos-core/src/graphics.rs b/crates/volamos-core/src/graphics.rs index 752a1af..32d9314 100644 --- a/crates/volamos-core/src/graphics.rs +++ b/crates/volamos-core/src/graphics.rs @@ -148,7 +148,7 @@ fn open_font_handler(ctx: &mut HandlerContext<'_, C>) -> Result<(), Disp return Ok(()); } - let Ok(font) = ctx.heap.alloc(TEXTFONT_SIZE) else { + let Ok(font) = ctx.heap.alloc(&mut *ctx.mem, TEXTFONT_SIZE) else { ctx.cpu.set_data_register(crate::cpu::DataRegister(0), 0); return Ok(()); }; @@ -196,7 +196,7 @@ fn open_font_handler(ctx: &mut HandlerContext<'_, C>) -> Result<(), Disp /// the font's own identity -- a fresh copy per `OpenFont` call, freed /// by [`close_font_handler`] alongside the rest. fn alloc_font_name(heap: &mut GuestHeap, mem: &mut M) -> Option { - let addr = heap.alloc(TOPAZ_NAME.len() as u32 + 1).ok()?; + let addr = heap.alloc(&mut *mem, TOPAZ_NAME.len() as u32 + 1).ok()?; crate::guestmem::write_c_string(mem, addr, TOPAZ_NAME); Some(addr) } diff --git a/crates/volamos-core/src/guestmem.rs b/crates/volamos-core/src/guestmem.rs index 4bcdd62..5c05771 100644 --- a/crates/volamos-core/src/guestmem.rs +++ b/crates/volamos-core/src/guestmem.rs @@ -450,8 +450,13 @@ impl GuestHeap { /// start) stays aligned; a request for `0` bytes still consumes a /// (minimal, 0-sized) accounted block so a subsequent `free` on it is /// well-defined. - pub fn alloc(&mut self, size: u32) -> Result { - self.alloc_with_requested(size, size) + /// + /// `mem` is the guest address space the block is being handed out + /// *in*, and is required rather than optional: see + /// [`Self::alloc_with_requested`]'s doc for why every allocation has + /// to touch it. + pub fn alloc(&mut self, mem: &mut dyn AddressSpace, size: u32) -> Result { + self.alloc_with_requested(mem, size, size) } /// Like [`GuestHeap::alloc`], but additionally records the caller's @@ -470,11 +475,63 @@ impl GuestHeap { /// exactly `alloc_with_requested(size, size)`, i.e. "no distinct /// requested size, treat the two rounding steps this module already /// does as the only slack there is." + /// + /// # Why this takes the address space (issue #95) + /// + /// The heap recycles addresses, and under `--sanitize` the block it + /// is about to hand out may still carry the previous tenant's + /// [`crate::sanitize::PoisonReason::Freed`] marking -- writing over + /// a byte does not clear that (the shadow map heals + /// [`crate::sanitize::ShadowState::Uninit`] bytes but records and + /// keeps [`crate::sanitize::ShadowState::Unaddressable`] ones). Left + /// in place, the allocating handler's own header write or + /// `MEMF_CLEAR` zeroing is reported as a use-after-free, and so is + /// every subsequent write the guest makes into its brand-new buffer + /// -- one violation per byte, which is slow enough to look like a + /// hang. + /// + /// Clearing it belongs *here*, at the one point where a block leaves + /// the heap's control, rather than in each caller: `AllocMem`/ + /// `AllocVec`/`AllocPooled` are only 3 of some four dozen sites + /// across `dosfile`, `doslock`, `dosargs`, `dosseg`, `dosanchor`, + /// `bsdsocket`, `intuition`, `graphics`, `locale` and more that + /// carve guest structures out of this heap, *any* of which can be + /// handed a block that `FreeMem`/`FreeVec`/`FreePooled` poisoned. + /// Taking `mem` as a parameter is what makes that impossible to + /// forget: a new allocation site cannot compile without supplying + /// it. The whole *block* is cleared, redzones included -- nothing + /// about the previous tenant applies to these addresses any more -- + /// and callers that want redzone/alignment-slack protection re-poison + /// afterwards via `execmem::poison_allocation_edges`, which has to + /// run after the handler's own initializing writes anyway. pub fn alloc_with_requested( &mut self, + mem: &mut dyn AddressSpace, size: u32, requested: u32, ) -> Result { + let (user_start, block_start, block_end) = self.alloc_untracked(size, requested)?; + mem.clear_fresh_block(block_start, block_end - block_start); + Ok(user_start) + } + + /// The allocator proper: the bookkeeping half of + /// [`Self::alloc_with_requested`], with no sanitizer involvement. + /// Returns `(user_start, block_start, block_end)` so its caller can + /// clear the shadow map over the whole block without a follow-up + /// [`Self::extent_of_live_alloc`] lookup. + /// + /// Private, and deliberately so: every allocation a guest can ever + /// see has to go through [`Self::alloc_with_requested`] to get its + /// shadow bytes cleared, and a caller that could reach this directly + /// could skip that. Within the module, the test-only `alloc_bare` + /// shorthands use it for the placement/coalescing tests, which have + /// no address space in play. + fn alloc_untracked( + &mut self, + size: u32, + requested: u32, + ) -> Result<(u32, u32, u32), GuestHeapError> { let aligned_size = align_up(size); // Clamp `requested` to what was actually reserved. A caller that @@ -541,7 +598,7 @@ impl GuestHeap { block_end, }, ); - Ok(user_start) + Ok((user_start, block_start, block_end)) } /// Frees a block previously returned by [`GuestHeap::alloc`]. @@ -831,19 +888,41 @@ pub fn write_bstr(mem: &mut dyn AddressSpace, addr: u32, bytes: &[u8]) -> u8 { len } +/// Test-only shorthands for [`GuestHeap::alloc`]/ +/// [`GuestHeap::alloc_with_requested`] without an address space. +/// +/// The allocator's own unit tests are about placement, rounding, +/// coalescing and the quarantine -- none of which involve the sanitizer. +/// The shadow-clearing half that `alloc` adds on top is covered where it +/// can be observed, against a real `FlatMemory`: see +/// `alloc_clears_a_recycled_blocks_freed_poison` below and +/// `execmem::tests::a_recycled_alloc_vec_block_is_no_longer_a_freed_block`. +#[cfg(test)] +impl GuestHeap { + fn alloc_bare(&mut self, size: u32) -> Result { + self.alloc_bare_req(size, size) + } + + fn alloc_bare_req(&mut self, size: u32, requested: u32) -> Result { + self.alloc_untracked(size, requested) + .map(|(addr, _, _)| addr) + } +} + #[cfg(test)] mod tests { use super::*; use crate::memory::FlatMemory; + use crate::sanitize::{PoisonReason, ShadowState}; #[test] fn alloc_returns_4_byte_aligned_addresses() { let mut heap = GuestHeap::new(0x1001, 0x2000); // start rounds up to 0x1004. - let a = heap.alloc(3).unwrap(); + let a = heap.alloc_bare(3).unwrap(); assert_eq!(a % 4, 0); assert_eq!(a, 0x1004); - let b = heap.alloc(1).unwrap(); + let b = heap.alloc_bare(1).unwrap(); assert_eq!(b % 4, 0); // 3 rounds up to 4, so b should be right after a's 4-byte block. assert_eq!(b, 0x1008); @@ -852,32 +931,73 @@ mod tests { #[test] fn alloc_free_realloc_reuses_freed_block() { let mut heap = GuestHeap::new(0x1000, 0x2000); - let a = heap.alloc(16).unwrap(); + let a = heap.alloc_bare(16).unwrap(); heap.free(a).unwrap(); - let b = heap.alloc(16).unwrap(); + let b = heap.alloc_bare(16).unwrap(); assert_eq!(a, b, "freed block should be reused by a same-size alloc"); } + #[test] + fn alloc_clears_a_recycled_blocks_freed_poison() { + // Issue #95, at the one place the fix lives. Every guest-visible + // allocation in the runtime -- `AllocMem` and `AllocVec`, but + // equally the `FileHandle` a dos `Open` carves out, the + // `FileLock` a `Lock` carves out, a `ReadArgs` argument buffer + // and three dozen more -- comes out of here, and any of them can + // be handed a block that `FreeMem`/`FreeVec` poisoned. Clearing + // it here is what makes that true for all of them at once + // (`alloc` cannot even be called without an address space to + // clear), so this test works the heap directly rather than + // through any one handler. + let mut mem = FlatMemory::new(0x2000); + mem.enable_sanitizer(); + let mut heap = GuestHeap::new(0x1000, 0x2000).with_redzone_size(DEFAULT_REDZONE_SIZE); + + let a = heap.alloc(&mut mem, 16).unwrap(); + // Exactly what `execmem::poison_freed_block` does before a + // `FreeVec`, then release the block for immediate reuse (the + // quarantine is off, so `free` puts it straight back). + let extent = heap.extent_of_live_alloc(a).unwrap(); + mem.shadow_mut().unwrap().mark_unaddressable( + extent.block_start, + extent.block_end - extent.block_start, + PoisonReason::Freed, + ); + heap.free(a).unwrap(); + + let b = heap.alloc(&mut mem, 16).unwrap(); + assert_eq!(a, b, "the test needs the block to actually be recycled"); + let shadow = mem.shadow().unwrap(); + for addr in extent.block_start..extent.block_end { + assert_eq!( + shadow.state(addr), + ShadowState::Valid, + "{addr:#x}: nothing about the previous tenant of a recycled \ + block applies to it any more, redzone bytes included" + ); + } + } + #[test] fn alloc_free_coalesces_adjacent_blocks() { let mut heap = GuestHeap::new(0x1000, 0x2000); - let a = heap.alloc(16).unwrap(); - let b = heap.alloc(16).unwrap(); - let c = heap.alloc(16).unwrap(); + let a = heap.alloc_bare(16).unwrap(); + let b = heap.alloc_bare(16).unwrap(); + let c = heap.alloc_bare(16).unwrap(); heap.free(a).unwrap(); heap.free(c).unwrap(); heap.free(b).unwrap(); // Everything freed and coalesced back into one block: a single // alloc of the whole managed region should now succeed. - let big = heap.alloc(0x1000 - 16 * 3).unwrap(); + let big = heap.alloc_bare(0x1000 - 16 * 3).unwrap(); assert_eq!(big, a); } #[test] fn alloc_exhaustion_returns_out_of_memory_err() { let mut heap = GuestHeap::new(0x1000, 0x1010); // 16 bytes total - heap.alloc(16).unwrap(); - let err = heap.alloc(4).unwrap_err(); + heap.alloc_bare(16).unwrap(); + let err = heap.alloc_bare(4).unwrap_err(); match err { GuestHeapError::OutOfMemory { requested, .. } => assert_eq!(requested, 4), other => panic!("expected OutOfMemory, got {other:?}"), @@ -887,7 +1007,7 @@ mod tests { #[test] fn double_free_is_detected_as_an_error() { let mut heap = GuestHeap::new(0x1000, 0x2000); - let a = heap.alloc(16).unwrap(); + let a = heap.alloc_bare(16).unwrap(); heap.free(a).unwrap(); let err = heap.free(a).unwrap_err(); assert_eq!(err, GuestHeapError::DoubleOrInvalidFree { addr: a }); @@ -903,7 +1023,7 @@ mod tests { #[test] fn empty_heap_range_always_out_of_memory() { let mut heap = GuestHeap::new(0x2000, 0x1000); // end <= start - assert!(heap.alloc(1).is_err()); + assert!(heap.alloc_bare(1).is_err()); } #[test] @@ -955,9 +1075,9 @@ mod tests { fn total_free_sums_disjoint_free_blocks() { let mut heap = GuestHeap::new(0x1000, 0x1000 + 48); assert_eq!(heap.total_free(), 48); - let a = heap.alloc(16).unwrap(); - let _b = heap.alloc(16).unwrap(); - let _c = heap.alloc(16).unwrap(); + let a = heap.alloc_bare(16).unwrap(); + let _b = heap.alloc_bare(16).unwrap(); + let _c = heap.alloc_bare(16).unwrap(); assert_eq!(heap.total_free(), 0); heap.free(a).unwrap(); assert_eq!(heap.total_free(), 16); @@ -966,9 +1086,9 @@ mod tests { #[test] fn largest_free_finds_the_biggest_block_even_when_fragmented() { let mut heap = GuestHeap::new(0x1000, 0x1000 + 48); - let a = heap.alloc(16).unwrap(); - let _b = heap.alloc(16).unwrap(); - let _c = heap.alloc(16).unwrap(); + let a = heap.alloc_bare(16).unwrap(); + let _b = heap.alloc_bare(16).unwrap(); + let _c = heap.alloc_bare(16).unwrap(); assert_eq!(heap.largest_free(), 0); // Free the first and third blocks (non-adjacent to each other, // so they don't coalesce into one bigger block): two 16-byte @@ -989,7 +1109,7 @@ mod tests { #[test] fn size_of_live_alloc_reports_the_rounded_size_and_none_when_unknown() { let mut heap = GuestHeap::new(0x1000, 0x2000); - let a = heap.alloc(13).unwrap(); // rounds up to 16 + let a = heap.alloc_bare(13).unwrap(); // rounds up to 16 assert_eq!(heap.size_of_live_alloc(a), Some(16)); assert_eq!(heap.size_of_live_alloc(0x1234), None); heap.free(a).unwrap(); @@ -1015,13 +1135,13 @@ mod tests { let mut heap = GuestHeap::new(0x1000, 0x2000); assert_eq!(heap.redzone_size(), 0); assert_eq!(heap.quarantine_budget(), 0); - let a = heap.alloc(16).unwrap(); + let a = heap.alloc_bare(16).unwrap(); assert_eq!(a, 0x1000); assert_eq!(heap.size_of_live_alloc(a), Some(16)); - let b = heap.alloc(16).unwrap(); + let b = heap.alloc_bare(16).unwrap(); assert_eq!(b, 0x1010, "no redzone gap between consecutive allocs"); heap.free(a).unwrap(); - let c = heap.alloc(16).unwrap(); + let c = heap.alloc_bare(16).unwrap(); assert_eq!( c, a, "freed block reused immediately, as before quarantine existed" @@ -1031,7 +1151,7 @@ mod tests { #[test] fn redzones_on_return_user_address_past_leading_redzone_with_plain_reported_size() { let mut heap = GuestHeap::new(0x1000, 0x3000).with_redzone_size(32); - let a = heap.alloc(16).unwrap(); + let a = heap.alloc_bare(16).unwrap(); // The whole block reserved is redzone(32) + user(16) + redzone(32), // starting at the heap base 0x1000, so the user address sits 32 // bytes in. @@ -1044,8 +1164,8 @@ mod tests { #[test] fn consecutive_allocations_with_redzones_are_separated_by_at_least_two_redzones() { let mut heap = GuestHeap::new(0x1000, 0x4000).with_redzone_size(32); - let a = heap.alloc(16).unwrap(); - let b = heap.alloc(16).unwrap(); + let a = heap.alloc_bare(16).unwrap(); + let b = heap.alloc_bare(16).unwrap(); assert!(b > a, "b should be allocated after a"); // Between the end of a's user bytes and the start of b's user // bytes there must be at least a's trailing redzone plus b's @@ -1063,7 +1183,7 @@ mod tests { // 32-byte redzone on each side it needs 80 bytes and must fail // cleanly rather than handing out an unguarded block. let mut heap = GuestHeap::new(0x1000, 0x1010).with_redzone_size(32); - let err = heap.alloc(16).unwrap_err(); + let err = heap.alloc_bare(16).unwrap_err(); match err { GuestHeapError::OutOfMemory { requested, .. } => assert_eq!(requested, 16), other => panic!("expected OutOfMemory, got {other:?}"), @@ -1075,7 +1195,7 @@ mod tests { let mut heap = GuestHeap::new(0x1000, 0x2000).with_redzone_size(32); // Near-u32::MAX size plus two redzones would overflow u32 if // computed naively; it must fail cleanly instead. - let err = heap.alloc(u32::MAX - 8).unwrap_err(); + let err = heap.alloc_bare(u32::MAX - 8).unwrap_err(); match err { GuestHeapError::OutOfMemory { .. } => {} other => panic!("expected OutOfMemory, got {other:?}"), @@ -1085,12 +1205,12 @@ mod tests { #[test] fn quarantine_holds_a_freed_address_back_from_immediate_reuse() { let mut heap = GuestHeap::new(0x1000, 0x2000).with_quarantine_budget(1024); - let a = heap.alloc(16).unwrap(); + let a = heap.alloc_bare(16).unwrap(); heap.free(a).unwrap(); // The freed block is still within budget (16 bytes << 1024), so // it should not have been released back to the free list yet: a // same-size alloc must land somewhere else. - let b = heap.alloc(16).unwrap(); + let b = heap.alloc_bare(16).unwrap(); assert_ne!( a, b, "quarantined address must not be handed out immediately" @@ -1102,14 +1222,14 @@ mod tests { // Budget holds exactly one 16-byte block; a second free must // push the first back out to the free list (oldest first). let mut heap = GuestHeap::new(0x1000, 0x2000).with_quarantine_budget(16); - let a = heap.alloc(16).unwrap(); - let b = heap.alloc(16).unwrap(); + let a = heap.alloc_bare(16).unwrap(); + let b = heap.alloc_bare(16).unwrap(); heap.free(a).unwrap(); // quarantine now holds exactly 16 bytes (at budget, not over) heap.free(b).unwrap(); // pushes quarantine to 32 bytes, over budget: drains `a` first // `a`'s block should now be back in the free list and coalesced // with any adjacent space, so it's allocatable again. - let c = heap.alloc(16).unwrap(); + let c = heap.alloc_bare(16).unwrap(); assert_eq!( c, a, "drained block should be released in FIFO order and reusable" @@ -1119,23 +1239,23 @@ mod tests { #[test] fn zero_quarantine_budget_behaves_like_immediate_free() { let mut heap = GuestHeap::new(0x1000, 0x2000).with_quarantine_budget(0); - let a = heap.alloc(16).unwrap(); + let a = heap.alloc_bare(16).unwrap(); heap.free(a).unwrap(); - let b = heap.alloc(16).unwrap(); + let b = heap.alloc_bare(16).unwrap(); assert_eq!(a, b, "budget 0 disables quarantining entirely"); } #[test] fn lowering_quarantine_budget_drains_blocks_that_no_longer_fit() { let mut heap = GuestHeap::new(0x1000, 0x2000).with_quarantine_budget(1024); - let a = heap.alloc(16).unwrap(); + let a = heap.alloc_bare(16).unwrap(); heap.free(a).unwrap(); // Still well within the free list as "not free" (quarantined). assert_eq!(heap.total_free(), heap.free_bytes()); // Shrinking the budget below what's queued must drain it // immediately, not just on the next free. heap.set_quarantine_budget(0); - let b = heap.alloc(16).unwrap(); + let b = heap.alloc_bare(16).unwrap(); assert_eq!( a, b, "shrinking the budget should have drained the queued block" @@ -1145,7 +1265,7 @@ mod tests { #[test] fn recently_freed_info_answers_for_a_freed_address_and_not_for_a_stranger() { let mut heap = GuestHeap::new(0x1000, 0x2000); - let a = heap.alloc(16).unwrap(); + let a = heap.alloc_bare(16).unwrap(); assert_eq!( heap.recently_freed_info(a), None, @@ -1162,10 +1282,10 @@ mod tests { #[test] fn recently_freed_info_reports_the_most_recent_free_serial_order() { let mut heap = GuestHeap::new(0x1000, 0x2000); - let a = heap.alloc(16).unwrap(); + let a = heap.alloc_bare(16).unwrap(); heap.free(a).unwrap(); let first = heap.recently_freed_info(a).unwrap(); - let b = heap.alloc(16).unwrap(); + let b = heap.alloc_bare(16).unwrap(); assert_eq!(a, b, "immediately reusable: quarantine is off by default"); heap.free(b).unwrap(); let second = heap.recently_freed_info(a).unwrap(); @@ -1181,7 +1301,7 @@ mod tests { // Request 13 (true "requested" size) which aligns up to 16: 3 // bytes of alignment slack between the requested data and the // trailing redzone. - let a = heap.alloc_with_requested(13, 13).unwrap(); + let a = heap.alloc_bare_req(13, 13).unwrap(); let extent = heap.extent_of_live_alloc(a).expect("a is live"); assert_eq!(extent.user_start, a); assert_eq!(extent.user_size, 16); @@ -1216,7 +1336,7 @@ mod tests { // underflow. The guest can never legitimately touch more than // was reserved, so clamping is the only well-ordered answer. let mut heap = GuestHeap::new(0x1000, 0x3000).with_redzone_size(32); - let a = heap.alloc_with_requested(16, 999).unwrap(); + let a = heap.alloc_bare_req(16, 999).unwrap(); let extent = heap.extent_of_live_alloc(a).expect("a is live"); assert_eq!(extent.user_size, 16); assert_eq!(extent.requested_size, 16, "clamped down to the reservation"); @@ -1231,7 +1351,7 @@ mod tests { #[test] fn extent_of_live_alloc_has_empty_redzone_ranges_when_redzones_disabled() { let mut heap = GuestHeap::new(0x1000, 0x2000); - let a = heap.alloc(16).unwrap(); + let a = heap.alloc_bare(16).unwrap(); let extent = heap.extent_of_live_alloc(a).unwrap(); assert_eq!(extent.block_start, extent.user_start); assert_eq!(extent.block_end, extent.user_start + extent.user_size); @@ -1244,12 +1364,12 @@ mod tests { assert_eq!(plain.total_free(), total); let mut redzoned = GuestHeap::new(0x1000, 0x1000 + total).with_redzone_size(16); - let a = redzoned.alloc(16).unwrap(); + let a = redzoned.alloc_bare(16).unwrap(); // 16 (redzone) + 16 (user) + 16 (redzone) = 48 bytes consumed. assert_eq!(redzoned.total_free(), total - 48); let mut quarantined = GuestHeap::new(0x1000, 0x1000 + total).with_quarantine_budget(1024); - let b = quarantined.alloc(16).unwrap(); + let b = quarantined.alloc_bare(16).unwrap(); quarantined.free(b).unwrap(); assert_eq!( quarantined.total_free(), diff --git a/crates/volamos-core/src/intuition.rs b/crates/volamos-core/src/intuition.rs index 05afa5d..97ac3ba 100644 --- a/crates/volamos-core/src/intuition.rs +++ b/crates/volamos-core/src/intuition.rs @@ -283,12 +283,12 @@ fn ensure_default_screen( return addr; } let screen = heap - .alloc(SCREEN_SIZE) + .alloc(&mut *mem, SCREEN_SIZE) .expect("guest heap has room for the one default Screen"); let title_bytes = b"Workbench Screen"; let title_addr = heap - .alloc(title_bytes.len() as u32 + 1) + .alloc(&mut *mem, title_bytes.len() as u32 + 1) .expect("guest heap has room for the screen title string"); write_c_string(mem, title_addr, title_bytes); @@ -489,18 +489,18 @@ fn open_window_handler(ctx: &mut HandlerContext<'_, C>) -> Result<(), Di requested_screen }; - let Ok(window) = ctx.heap.alloc(WINDOW_SIZE) else { + let Ok(window) = ctx.heap.alloc(&mut *ctx.mem, WINDOW_SIZE) else { ctx.cpu.set_data_register(DataRegister(0), 0); return Ok(()); }; - let Ok(rport) = ctx.heap.alloc(RASTPORT_SIZE) else { + let Ok(rport) = ctx.heap.alloc(&mut *ctx.mem, RASTPORT_SIZE) else { ctx.heap .free(window) .expect("just-allocated Window is live"); ctx.cpu.set_data_register(DataRegister(0), 0); return Ok(()); }; - let Ok(user_port) = ctx.heap.alloc(MSGPORT_SIZE) else { + let Ok(user_port) = ctx.heap.alloc(&mut *ctx.mem, MSGPORT_SIZE) else { ctx.heap .free(rport) .expect("just-allocated RastPort is live"); diff --git a/crates/volamos-core/src/locale.rs b/crates/volamos-core/src/locale.rs index 33ca0df..f8b327e 100644 --- a/crates/volamos-core/src/locale.rs +++ b/crates/volamos-core/src/locale.rs @@ -106,7 +106,7 @@ const LOCALE_STRUCT_SIZE: u32 = 168; fn open_locale_handler(ctx: &mut HandlerContext<'_, C>) -> Result<(), DispatchError> { let addr = ctx .heap - .alloc(LOCALE_STRUCT_SIZE) + .alloc(&mut *ctx.mem, LOCALE_STRUCT_SIZE) .map_err(|e| DispatchError::HandlerFailed { library: "locale.library".to_string(), lvo: -156, diff --git a/crates/volamos-core/src/memory.rs b/crates/volamos-core/src/memory.rs index 2817e7d..c8703b3 100644 --- a/crates/volamos-core/src/memory.rs +++ b/crates/volamos-core/src/memory.rs @@ -51,16 +51,18 @@ pub trait AddressSpace { fn write_u8(&mut self, addr: u32, value: u8); /// Clears any sanitizer poison over `[addr, addr + len)`, a block the - /// guest heap has just handed out. + /// guest heap has just handed out (issue #95). /// - /// Call this as soon as an allocation succeeds, *before* the handler - /// writes a header or zeroes the block: the heap recycles addresses, - /// so those bytes may still carry the `PoisonReason::Freed` marking - /// of the previous allocation, and writing over them does not clear - /// it -- the shadow map heals `Uninit` bytes but keeps - /// `Unaddressable` ones. Without this, the handler's own - /// initialization is reported as a use-after-free, and so is every - /// write the guest then makes. + /// **Called by [`crate::guestmem::GuestHeap::alloc_with_requested`], + /// which is the only place that should:** the heap recycles + /// addresses, so a fresh block may still carry the + /// `PoisonReason::Freed` marking of the previous allocation, and + /// writing over it does not clear that (the shadow map heals + /// `Uninit` bytes but records and keeps `Unaddressable` ones). Doing + /// it at the one point where a block leaves the heap's control -- + /// rather than in each of the four dozen handlers that allocate -- + /// is what keeps a new allocation site from silently reintroducing + /// the bug; see that method's doc for the full reasoning. /// /// Default: nothing to do, for address spaces with no shadow map. fn clear_fresh_block(&mut self, _addr: u32, _len: u32) {} diff --git a/crates/volamos-core/src/sanitize.rs b/crates/volamos-core/src/sanitize.rs index ac7aaf8..be9ab41 100644 --- a/crates/volamos-core/src/sanitize.rs +++ b/crates/volamos-core/src/sanitize.rs @@ -952,10 +952,16 @@ impl ShadowMap { /// their existing `hits` even after the cap is hit, since that's /// O(1) and doesn't grow the log). Takes `&self` -- see this /// struct's "interior mutability" doc. - fn record(&self, addr: u32, size: u8, kind: ViolationKind, reason: Option) { + fn record( + &self, + addr: u32, + size: u8, + kind: ViolationKind, + reason: Option, + ) -> bool { let pc = self.current_pc; if self.ignored_pcs.contains(&pc) { - return; + return false; } let key: ViolationKey = (pc, addr, kind); self.push_violation( @@ -970,7 +976,7 @@ impl ShadowMap { actual_return_addr: None, hits: 1, }, - ); + ) } /// Records one [`ViolationKind::ReturnAddressCorrupted`] at stack @@ -1001,6 +1007,9 @@ impl ShadowMap { hits: 1, }, ); + // `push_violation`'s `bool` is for the per-byte access loops in + // `check_read`/`check_write`; a return-address check is one whole + // access already, so there is nothing here to short-circuit. } /// Shared dedup/cap logic for [`Self::record`]/[`Self:: @@ -1008,20 +1017,27 @@ impl ShadowMap { /// repeat of `key`, otherwise appends `violation` unless the log is /// already at [`MAX_VIOLATIONS`], in which case the miss is only /// counted (see [`Self::suppressed_count`]). - fn push_violation(&self, key: ViolationKey, violation: Violation) { + /// + /// Returns whether the violation made it into the log -- `true` for + /// both an append and a `hits` bump, `false` when it was suppressed + /// past the cap. [`Self::check_read`]/[`Self::check_write`] need that + /// answer per byte; see [`Self::check_write`] for why it is a return + /// value rather than something they observe for themselves. + fn push_violation(&self, key: ViolationKey, violation: Violation) -> bool { let mut index = self.index.borrow_mut(); let mut violations = self.violations.borrow_mut(); if let Some(&i) = index.get(&key) { violations[i].hits += 1; - return; + return true; } if violations.len() >= MAX_VIOLATIONS { self.suppressed.set(self.suppressed.get() + 1); - return; + return false; } let i = violations.len(); violations.push(violation); index.insert(key, i); + true } /// Checks one byte about to be read at `addr`, recording an @@ -1031,21 +1047,23 @@ impl ShadowMap { /// `size` is the size in bytes of the whole access this byte is /// part of (carried through purely for the [`Violation`]'s own /// `size` field/diagnostics, not used to decide anything here). - fn check_read_byte(&self, addr: u32, size: u8) { + /// Returns whether this byte's access was logged as a violation -- + /// see [`Self::check_read`] for what its caller does with that. + fn check_read_byte(&self, addr: u32, size: u8) -> bool { match self.state(addr) { ShadowState::Unaddressable => { let reason = self.poison_reason(addr); if reason == Some(PoisonReason::BelowStackPointer) && self.below_sp_violation_is_within_grace_band(addr) { - return; + return false; } - self.record(addr, size, ViolationKind::InvalidRead, reason); + self.record(addr, size, ViolationKind::InvalidRead, reason) } ShadowState::Uninit if self.report_uninit => { - self.record(addr, size, ViolationKind::UninitRead, None); + self.record(addr, size, ViolationKind::UninitRead, None) } - ShadowState::Uninit | ShadowState::Valid => {} + ShadowState::Uninit | ShadowState::Valid => false, } } @@ -1080,7 +1098,9 @@ impl ShadowMap { /// made turning `report_uninit` on for real binaries produce tens of /// thousands of false positives -- see this module's "turning /// `report_uninit` on for real" doc. - fn check_write_byte(&mut self, addr: u32, size: u8) { + /// Returns whether this byte's access was logged as a violation -- + /// see [`Self::check_write`] for what its caller does with that. + fn check_write_byte(&mut self, addr: u32, size: u8) -> bool { match self.state(addr) { ShadowState::Unaddressable => { let reason = self.poison_reason(addr); @@ -1090,16 +1110,17 @@ impl ShadowMap { if let Some(slot) = self.bytes.get_mut(addr as usize) { *slot = VALID_BYTE; } - return; + return false; } - self.record(addr, size, ViolationKind::InvalidWrite, reason); + self.record(addr, size, ViolationKind::InvalidWrite, reason) } ShadowState::Uninit => { if let Some(slot) = self.bytes.get_mut(addr as usize) { *slot = VALID_BYTE; } + false } - ShadowState::Valid => {} + ShadowState::Valid => false, } } @@ -1155,9 +1176,7 @@ impl ShadowMap { // wanted. Reporting the first offending byte keeps the address // in the message the one a reader can act on. for i in 0..u32::from(size) { - let before = self.violation_count(); - self.check_read_byte(addr.wrapping_add(i), size); - if self.violation_count() != before { + if self.check_read_byte(addr.wrapping_add(i), size) { return; } } @@ -1169,14 +1188,25 @@ impl ShadowMap { // As `check_read`: report at most once per access. Every byte is // still *visited*, because a write has to heal each `Uninit` // byte it covers even after one of them has reported. + // + // "Did that byte report?" comes back from `check_write_byte` + // itself (and `check_read_byte`, for the read path) rather than + // being inferred from a before/after `violation_count()`, which + // is what both loops used to do and is the other half of issue + // #95's 75x slowdown: `violation_count` *sums* `hits` across the + // whole log, so once the log filled up, every byte of every + // violating access walked 1000 entries twice. A program that + // trips a lot of violations pays for the log it has already + // built, quadratically, which is exactly when a sanitized run + // needs to stay usable. Measured on a 400,000-byte + // use-after-free loop: 1.33s before, 0.03s after, byte-identical + // output. let mut reported = false; for i in 0..u32::from(size) { - let before = self.violation_count(); if reported { self.heal_uninit_byte(addr.wrapping_add(i)); } else { - self.check_write_byte(addr.wrapping_add(i), size); - reported = self.violation_count() != before; + reported = self.check_write_byte(addr.wrapping_add(i), size); } } } diff --git a/crates/volamos-core/src/utility.rs b/crates/volamos-core/src/utility.rs index fe179a6..e64acf4 100644 --- a/crates/volamos-core/src/utility.rs +++ b/crates/volamos-core/src/utility.rs @@ -274,9 +274,8 @@ fn allocate_tag_items_handler( } let byte_size = num_tags.saturating_mul(TAG_ITEM_SIZE); - match ctx.heap.alloc(byte_size) { + match ctx.heap.alloc(&mut *ctx.mem, byte_size) { Ok(addr) => { - ctx.mem.clear_fresh_block(addr, byte_size); for i in 0..byte_size { ctx.mem.write_u8(addr.wrapping_add(i), 0); }