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 39d0640..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,17 +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}" - ), - })?; + 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 3ec0d60..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. @@ -590,7 +590,7 @@ fn create_io_request( port: u32, size: u32, ) -> Option { - let addr = heap.alloc(size).ok()?; + 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 0030d58..2c7756b 100644 --- a/crates/volamos-core/src/execmem.rs +++ b/crates/volamos-core/src/execmem.rs @@ -307,7 +307,10 @@ 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) => { let cleared = requirements & MEMF_CLEAR != 0; if cleared { @@ -419,7 +422,7 @@ 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) => { // Header: total block size (u32) followed by 4 bytes of // reserved padding (see the module docs). @@ -445,12 +448,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 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); } Err(_) => { @@ -530,7 +543,7 @@ 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.write_u32(pool, requirements); ctx.cpu.set_data_register(DataRegister(0), pool); @@ -588,7 +601,10 @@ 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) => { let cleared = requirements & MEMF_CLEAR != 0; if cleared { @@ -969,6 +985,111 @@ 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 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 392aa79..c8703b3 100644 --- a/crates/volamos-core/src/memory.rs +++ b/crates/volamos-core/src/memory.rs @@ -50,6 +50,23 @@ 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 (issue #95). + /// + /// **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) {} + /// 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 +241,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/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 7698dfd..e64acf4 100644 --- a/crates/volamos-core/src/utility.rs +++ b/crates/volamos-core/src/utility.rs @@ -274,7 +274,7 @@ 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) => { for i in 0..byte_size { ctx.mem.write_u8(addr.wrapping_add(i), 0);