Poison the other allocators that carve from the guest heap

crates/volamos-core/src/dosfile.rs

@@ -1359,6 +1359,11 @@ fn alloc_dos_object_handler<C: Cpu>(ctx: &mut HandlerContext<'_, C>) -> Result<(
     for i in 0..struct_size {
         ctx.mem.write_u8(addr.wrapping_add(i), 0);
     }
+    // After the zeroing, never before -- those writes heal shadow
+    // bytes, so poisoning first would flag this handler's own
+    // initialisation. `false` because the block is zeroed, hence
+    // initialised. See `crate::execmem::poison_allocation_edges`.
+    crate::execmem::poison_allocation_edges(ctx, addr, false);
     ctx.cpu.set_data_register(DataRegister(0), addr);
     Ok(())
 }
@@ -1390,6 +1395,8 @@ fn free_dos_object_handler<C: Cpu>(ctx: &mut HandlerContext<'_, C>) -> Result<()
         });
     }
 
+    // Before the free, while the extent is still queryable.
+    crate::execmem::poison_freed_block(ctx, addr);
     ctx.heap
         .free(addr)
         .map_err(|e| DispatchError::HandlerFailed {

crates/volamos-core/src/execlist.rs

@@ -506,6 +506,11 @@ fn create_msg_port_handler<C: Cpu>(ctx: &mut HandlerContext<'_, C>) -> Result<()
         }
     };
     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
+    // setup. `false` because the block is initialised by the time we
+    // get here. See `crate::execmem::poison_allocation_edges`.
+    crate::execmem::poison_allocation_edges(ctx, port, false);
     ctx.cpu.set_data_register(DataRegister(0), port);
     Ok(())
 }
@@ -540,6 +545,7 @@ fn delete_msg_port_handler<C: Cpu>(ctx: &mut HandlerContext<'_, C>) -> Result<()
     if port == 0 {
         return Ok(());
     }
+    crate::execmem::poison_freed_block(ctx, port);
     ctx.heap
         .free(port)
         .map_err(|e| DispatchError::HandlerFailed {
@@ -565,6 +571,13 @@ fn create_io_request_handler<C: Cpu>(ctx: &mut HandlerContext<'_, C>) -> Result<
     let port = ctx.cpu.address_register(AddressRegister(0));
     let size = ctx.cpu.data_register(DataRegister(0));
     let addr = create_io_request(ctx.heap, ctx.mem, port, size).unwrap_or(0);
+    if addr != 0 {
+        // Poisoned here rather than inside `create_io_request`, which
+        // takes `heap`/`mem` separately and so has no `HandlerContext`
+        // to reach the shadow map through. After its zeroing and field
+        // writes, per `poison_allocation_edges`' ordering rule.
+        crate::execmem::poison_allocation_edges(ctx, addr, false);
+    }
     ctx.cpu.set_data_register(DataRegister(0), addr);
     Ok(())
 }
@@ -595,6 +608,7 @@ fn delete_io_request_handler<C: Cpu>(ctx: &mut HandlerContext<'_, C>) -> Result<
     if addr == 0 {
         return Ok(());
     }
+    crate::execmem::poison_freed_block(ctx, addr);
     ctx.heap
         .free(addr)
         .map_err(|e| DispatchError::HandlerFailed {

crates/volamos-core/src/execmem.rs

@@ -204,6 +204,17 @@ fn apply_dirty_fill<C: Cpu>(ctx: &mut HandlerContext<'_, C>, addr: u32, len: u32
 /// at `addr` (issue #65). A no-op unless both a shadow map is installed
 /// (`--sanitize`) and the allocation actually exists in the heap.
 ///
+/// `pub(crate)` because `AllocMem`/`AllocVec`/`AllocPooled` are not the
+/// only handlers that carve guest-visible blocks out of
+/// [`crate::guestmem::GuestHeap`]: `utility.library`'s
+/// `AllocateTagItems`, `dos.library`'s `AllocDosObject` and
+/// `exec.library`'s `CreateIORequest`/`CreateMsgPort` all do too, and
+/// every one of them hands the guest a block it can overrun. Since they
+/// allocate from the same heap, redzone *space* is already being
+/// reserved for them whenever `--sanitize` is on -- only the shadow
+/// marking was missing, which is why sharing this one helper is the
+/// whole fix rather than a per-module reimplementation.
+///
 /// Marks the leading and trailing redzones and the alignment slack
 /// unaddressable, so any guest access that runs off either end of the
 /// block -- including into the padding between the size it asked for
@@ -224,7 +235,7 @@ fn apply_dirty_fill<C: Cpu>(ctx: &mut HandlerContext<'_, C>, addr: u32, len: u32
 /// default -- see `crate::sanitize`). Pass `false` when the handler has
 /// already zeroed the block for `MEMF_CLEAR`, since those bytes are
 /// then genuinely initialized.
-fn poison_allocation_edges<C: Cpu>(
+pub(crate) fn poison_allocation_edges<C: Cpu>(
     ctx: &mut HandlerContext<'_, C>,
     addr: u32,
     mark_data_uninit: bool,
@@ -262,7 +273,7 @@ fn poison_allocation_edges<C: Cpu>(
 /// the other half is `crate::guestmem`'s free quarantine holding the
 /// address out of circulation, so a subsequent allocation doesn't
 /// immediately re-mark these same bytes valid and hide the bug.
-fn poison_freed_block<C: Cpu>(ctx: &mut HandlerContext<'_, C>, addr: u32) {
+pub(crate) fn poison_freed_block<C: Cpu>(ctx: &mut HandlerContext<'_, C>, addr: u32) {
     let Some(extent) = ctx.heap.extent_of_live_alloc(addr) else {
         return;
     };

crates/volamos-core/src/utility.rs

@@ -279,6 +279,13 @@ fn allocate_tag_items_handler<C: Cpu>(
             for i in 0..byte_size {
                 ctx.mem.write_u8(addr.wrapping_add(i), 0);
             }
+            // After the zeroing, never before -- the writes above heal
+            // shadow bytes, so poisoning first would make this handler
+            // report a violation against its own initialisation. Same
+            // ordering rule as `crate::execmem`'s `MEMF_CLEAR` path;
+            // see `poison_allocation_edges`' doc. `false` because the
+            // block *is* initialised: it was just zeroed.
+            crate::execmem::poison_allocation_edges(ctx, addr, false);
             ctx.cpu.set_data_register(DataRegister(0), addr);
         }
         Err(_) => {
@@ -325,6 +332,8 @@ fn free_tag_items_handler<C: Cpu>(ctx: &mut HandlerContext<'_, C>) -> Result<(),
     }
 
     let byte_size = count.saturating_mul(TAG_ITEM_SIZE);
+    // Before the free, while the extent is still queryable.
+    crate::execmem::poison_freed_block(ctx, list);
     ctx.heap
         .free(list)
         .map_err(|e| DispatchError::HandlerFailed {
@@ -958,6 +967,91 @@ mod tests {
         0x2040 | (n << 9)
     }
 
+    /// `move.l Dsrc,Ddst` -- MOVE.L with both operands in data-register
+    /// direct mode (`0010 ddd 000 000 sss`), for stashing an address
+    /// across a library call that clobbers D0.
+    fn move_d_to_d(dst: u16, src: u16) -> u16 {
+        0x2000 | (dst << 9) | src
+    }
+
+    #[test]
+    fn freed_tag_items_block_is_poisoned_for_use_after_free() {
+        // The other half: `FreeTagItems` now marks the block freed, so
+        // a guest still holding the pointer is caught reading it.
+        let mut words = Vec::new();
+        words.push(move_imm_to_d(0));
+        words.push(0);
+        words.push(3);
+        words.extend_from_slice(&jsr_disp16_a6(-66)); // AllocateTagItems -> D0
+        words.push(move_d0_to_a(0)); // A0 = the array
+        words.push(move_d_to_d(2, 0)); // stash the address in D2
+        words.extend_from_slice(&jsr_disp16_a6(-78)); // FreeTagItems(A0)
+        words.push(move_d_to_d(0, 2)); // D0 = the now-freed address
+        words.push(RTS);
+
+        let mut rt = program(&words);
+        rt.memory_mut().enable_sanitizer();
+        rt.enable_heap_sanitizer();
+        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, "expected the freed block's address back in D0");
+
+        let shadow = rt.memory().shadow().expect("sanitizer enabled above");
+        assert_eq!(
+            shadow.state(addr),
+            crate::sanitize::ShadowState::Unaddressable,
+            "a freed TagItem array should read as unaddressable"
+        );
+        assert_eq!(
+            shadow.poison_reason(addr),
+            Some(crate::sanitize::PoisonReason::Freed),
+            "and specifically as freed, not as a redzone"
+        );
+    }
+
+    #[test]
+    fn allocate_tag_items_block_gets_redzones_under_the_sanitizer() {
+        // `AllocateTagItems` carves from the same `GuestHeap` as
+        // `AllocMem`, so redzone *space* was already being reserved for
+        // it whenever the sanitizer was on -- only the shadow marking
+        // was missing, which meant a guest overrunning a TagItem array
+        // volamos handed it went unreported. This asserts the guard
+        // bytes either side of the block are now actually poisoned.
+        let mut words = Vec::new();
+        words.push(move_imm_to_d(0)); // D0 = numTags
+        words.push(0);
+        words.push(3);
+        words.extend_from_slice(&jsr_disp16_a6(-66)); // AllocateTagItems
+        words.push(RTS);
+
+        let mut rt = program(&words);
+        rt.memory_mut().enable_sanitizer();
+        rt.enable_heap_sanitizer();
+        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);
+
+        let shadow = rt.memory().shadow().expect("sanitizer enabled above");
+        let size = 3 * TAG_ITEM_SIZE;
+        assert_eq!(
+            shadow.state(addr.wrapping_sub(1)),
+            crate::sanitize::ShadowState::Unaddressable,
+            "the byte before the block should be a poisoned redzone"
+        );
+        assert_eq!(
+            shadow.state(addr + size),
+            crate::sanitize::ShadowState::Unaddressable,
+            "the byte just past the block should be a poisoned redzone"
+        );
+        assert_eq!(
+            shadow.state(addr),
+            crate::sanitize::ShadowState::Valid,
+            "the block itself was zeroed by the handler, so it is initialised"
+        );
+    }
+
     #[test]
     fn allocate_tag_items_returns_a_zeroed_block() {
         let mut words = Vec::new();