diff --git a/node-graph/libraries/core-types/src/arena.rs b/node-graph/libraries/core-types/src/arena.rs index 9789090d73..e0d9a1e8dc 100644 --- a/node-graph/libraries/core-types/src/arena.rs +++ b/node-graph/libraries/core-types/src/arena.rs @@ -26,10 +26,11 @@ pub struct Arena { /// a park costs one pointer in the arena and owns its content outside it. retained_heap: AtomicUsize, /// Where [`Arena::move_park`] sent each moved park, as its offset here to - /// the header's address in the receiving arena, so a payload two records - /// share is moved once. Cleared by [`Arena::reset`], so a forwarding holds - /// for one generation. - forwarded: Mutex>, + /// the header's address in the receiving arena and the moved size, so a + /// payload two records share is moved once and a mistyped sharer is + /// refused. Cleared by [`Arena::reset`], so a forwarding holds for one + /// generation. + forwarded: Mutex>, } impl std::fmt::Debug for Arena { @@ -40,6 +41,9 @@ impl std::fmt::Debug for Arena { struct DropEntry { offset: usize, + /// The parked payload's own size, so [`Arena::move_park`] refuses a + /// reference into a park of another type that shares the offset. + size: usize, drop_fn: unsafe fn(*mut u8), /// The park glue's estimate of the heap this payload owns, 0 where the /// glue cannot measure it, so the counter is a lower bound. @@ -221,7 +225,7 @@ impl Arena { unsafe fn glue(p: *mut u8) { unsafe { p.cast::().drop_in_place() } } - self.drops.lock().unwrap().push(DropEntry { offset, drop_fn: glue::, retained }); + self.drops.lock().unwrap().push(DropEntry { offset, size: size_of::(), drop_fn: glue::, retained }); self.retained_heap.fetch_add(retained, Ordering::Relaxed); } // SAFETY: initialized above; insert-only, so no `&mut` to it can exist. @@ -246,23 +250,27 @@ impl Arena { /// have credited it, and this arena is debited what its own park recorded, /// so neither counter reads worse than it did before the move. /// - /// `None` where `src` is not this arena's park, where `T` is zero-sized - /// (whose parks share an offset and so cannot be told apart), or where - /// `dst` refused the header. + /// `None` where `src` is not this arena's park of `T`'s own size, where + /// `T` is zero-sized (whose parks share an offset and so cannot be told + /// apart), or where `dst` refused the header. /// /// # Safety - /// `src` must address a live `T` this arena parked, and `T` must own all of - /// its content: the moved header may reference no storage of this arena. + /// `src` must address a live `T`, and a park of this arena at that address + /// and size must be the `T` itself, not another type's park the address + /// coincides with. `T` must own all of its content: the moved header may + /// reference no storage of this arena. pub unsafe fn move_park(&self, src: *const u8, dst: &Arena, retained: usize) -> Option<*const T> { (size_of::() != 0).then_some(())?; let offset = (src as usize).checked_sub(self.base() as usize)?; (offset < self.buf.len()).then_some(())?; let mut forwarded = self.forwarded.lock().unwrap(); - if let Some(&moved) = forwarded.get(&offset) { + if let Some(&(moved, size)) = forwarded.get(&offset) { + (size == size_of::()).then_some(())?; return Some(moved as *const T); } let mut entries = self.drops.lock().unwrap(); let entry = entry_at(&entries, offset)?; + (entries[entry].size == size_of::()).then_some(())?; let slot = dst.alloc_scratch::(1)?; let target = slot.as_mut_ptr().cast::(); // SAFETY: the caller's contract on `src`, into a freshly reserved, @@ -276,12 +284,13 @@ impl Arena { } dst.drops.lock().unwrap().push(DropEntry { offset: target as usize - dst.base() as usize, + size: size_of::(), drop_fn: glue::, retained, }); dst.retained_heap.fetch_add(retained, Ordering::Relaxed); let _ = self.retained_heap.fetch_update(Ordering::Relaxed, Ordering::Relaxed, |current| Some(current.saturating_sub(parked))); - forwarded.insert(offset, target as usize); + forwarded.insert(offset, (target as usize, size_of::())); Some(target.cast_const()) } @@ -640,6 +649,26 @@ mod tests { assert_eq!(DROPS.load(Ordering::Relaxed), 1, "the flush that owns the obligation frees it exactly once"); } + #[test] + fn a_move_refuses_a_mistyped_park_and_its_forwarding() { + let _guard = COUNTER_GUARD.lock().unwrap_or_else(PoisonError::into_inner); + struct Owner { + _first: String, + _tail: u64, + } + let mut transient = Arena::new(1024).unwrap(); + let mut persistent = Arena::new(1024).unwrap(); + + let (parked, _) = transient.alloc(Owner { _first: String::from("a first member"), _tail: 0 }).unwrap(); + let src = std::ptr::from_ref(parked).cast::(); + assert!(unsafe { transient.move_park::(src, &persistent, 0) }.is_none(), "a park of another size is refused"); + unsafe { transient.move_park::(src, &persistent, 0) }.unwrap(); + assert!(unsafe { transient.move_park::(src, &persistent, 0) }.is_none(), "the forwarding refuses the same mistype"); + + transient.reset(); + persistent.reset(); + } + #[test] fn the_forwarding_map_lives_one_generation() { let _guard = COUNTER_GUARD.lock().unwrap_or_else(PoisonError::into_inner); diff --git a/node-graph/libraries/core-types/src/record.rs b/node-graph/libraries/core-types/src/record.rs index 4705b785f1..be4f841040 100644 --- a/node-graph/libraries/core-types/src/record.rs +++ b/node-graph/libraries/core-types/src/record.rs @@ -1855,8 +1855,8 @@ impl<'a> Promotion<'a> { /// Moves a transient payload's header into the persistent region instead of /// cloning the heap it owns: the heap travels with the drop obligation and /// is freed at the persistent flush, never at the transient reset. `None` - /// where the header is not the transient arena's own park or the region - /// refused it, which leaves the caller its clone path. + /// where the header is not the transient arena's own park of `T`'s size or + /// the region refused it, which leaves the caller its clone path. /// /// The forwarding is the evaluation's, so a payload two records share moves /// once and both reach the one persistent header. The source header stays @@ -1864,9 +1864,9 @@ impl<'a> Promotion<'a> { /// the move keeps sound: no read of it may outlive the persistent flush. /// /// # Safety - /// `parked` must address a live `T` the transient arena parked, and `T` - /// must own all of its content, a persistent header being allowed to - /// reference no transient storage. + /// `parked` must address a live `T`, and a transient park at that address + /// and size must be the `T` itself. `T` must own all of its content, a + /// persistent header being allowed to reference no transient storage. pub unsafe fn move_park(&self, parked: *const u8, retained: usize) -> Option<*const T> { // SAFETY: the caller's contract, forwarded to the parking arena. unsafe { self.transient.move_park::(parked, self.persistent, retained) }