From 2338f94c99e3206be212db5725ab4ae7593473ab Mon Sep 17 00:00:00 2001 From: Adrien Burgun Date: Fri, 17 Jun 2022 12:56:20 +0200 Subject: [PATCH] :bug: :sparkles: transmit_all, fix UB if *Context::position was modified --- stackline/src/context.rs | 70 +++++++++++++++++----- stackline/src/lib.rs | 77 ++---------------------- stackline/src/pane.rs | 116 +++++++++++++++++++++++++++++++++++++ stackline/src/signal.rs | 28 ++++++++- stackline/src/tile/mod.rs | 93 ++++++++++++++++++++++++++++- stackline/src/tile/wire.rs | 70 +++++++++++++++++++++- 6 files changed, 362 insertions(+), 92 deletions(-) create mode 100644 stackline/src/pane.rs diff --git a/stackline/src/context.rs b/stackline/src/context.rs index fe444cd..ff39fea 100644 --- a/stackline/src/context.rs +++ b/stackline/src/context.rs @@ -16,7 +16,7 @@ use super::*; // SAFETY: `pane[position].cell` is borrow mutably, while a pointer to the original Pane is kept; // thus, no other reference to `pane[position].cell` may be done pub struct UpdateContext<'a> { - pub position: (usize, usize), + position: (usize, usize), pane: *const Pane, phantom: PhantomData<&'a Pane>, @@ -38,6 +38,11 @@ impl<'a> UpdateContext<'a> { Some((res, tile)) } + #[inline] + pub fn position(&self) -> (usize, usize) { + self.position + } + #[inline] pub fn signal<'b>(&'b self) -> Option<&'b Rc> where 'a: 'b { let pane = unsafe { self.pane() }; @@ -87,7 +92,7 @@ impl<'a> UpdateContext<'a> { // SAFETY: this structures ensures that it has exlusive, mutable access to `∀x, pane[x].signal` and `pane.signals`. // Other parts of `pane` may be accessed and returned immutably. pub struct TransmitContext<'a> { - pub position: (usize, usize), + position: (usize, usize), pane: *mut Pane, phantom: PhantomData<&'a mut Pane>, @@ -112,6 +117,11 @@ impl<'a> TransmitContext<'a> { Some((res, tile, signal)) } + #[inline] + pub fn position(&self) -> (usize, usize) { + self.position + } + /// Returns an immutable reference to the [tile](AnyTile) at `pos` in the current [Pane]. /// Returns `None` if that tile does not exist. #[inline] @@ -124,22 +134,12 @@ impl<'a> TransmitContext<'a> { /// Sends a signal to be stored in a cell (may be the current one), the signal overrides that of the other cell /// Returns true if the signal was stored in a cell, false otherwise - pub fn send<'b>(&'b self, pos: (usize, usize), signal: Signal) -> bool where 'a: 'b { + pub fn send<'b>(&'b self, pos: (usize, usize), signal: Signal) -> Option> where 'a: 'b { // SAFETY: we do not return any reference to any data borrowed in this function // SAFETY: we only access `pane[pos].signal` and `pane.signals` let pane = unsafe { self.pane_mut() }; - match pane.get_mut(pos) { - Some(ref mut tile) => { - if let Some(weak) = tile.set_signal(signal) { - pane.signals.push(weak); - true - } else { - false - } - } - _ => false - } + pane.set_signal(pos, signal) } /// Returns `Some((position.x + Δx, position.y + Δy))` iff `(x + Δx, y + Δy)` is inside the pane @@ -165,3 +165,45 @@ impl<'a> TransmitContext<'a> { &mut *self.pane } } + +#[cfg(test)] +mod test { + use super::*; + + #[test] + fn test_update_exclusivity() { + // Check that UpdateContext does not allow any other reference to `tiles[position].cell` to be made + let mut pane = Pane::empty(4, 4).unwrap(); + let mut tile = FullTile::from(Wire::new(Orientation::Any)); + tile.set_signal(Signal::empty((1, 2), Direction::Up)).unwrap(); + *pane.get_mut((1, 2)).unwrap() = tile; + + let (ctx, _tile) = UpdateContext::new(&mut pane, (1, 2)).unwrap(); + + assert_eq!(ctx.position(), (1, 2)); + + let tile_self: Option<&FullTile> = ctx.get((1, 2)); // The FullTile may not be read + assert!(tile_self.is_none()); + + assert!(ctx.signal().is_some()); // Our Signal may be read, though + } + + #[test] + fn test_transmit() { + let mut pane = Pane::empty(4, 4).unwrap(); + let mut tile = FullTile::from(Wire::new(Orientation::Any)); + tile.set_signal(Signal::empty((1, 2), Direction::Up)).unwrap(); + *pane.get_mut((1, 2)).unwrap() = tile; + + let (ctx, _tile, _signal) = TransmitContext::new(&mut pane, (1, 2)).unwrap(); + + assert_eq!(ctx.position(), (1, 2)); + + let tile_self: Option<&AnyTile> = ctx.get((1, 2)); + assert!(tile_self.is_some()); // We may read our AnyTile, as they are under an immutable borrow + + // Check that the signal was dropped + std::mem::drop(ctx); + assert!(pane.get((1, 2)).unwrap().signal().is_none()); + } +} diff --git a/stackline/src/lib.rs b/stackline/src/lib.rs index 85044c2..7297f44 100644 --- a/stackline/src/lib.rs +++ b/stackline/src/lib.rs @@ -1,86 +1,21 @@ use std::num::NonZeroUsize; use std::rc::Weak; +mod signal; +pub use signal::*; + +mod pane; +pub use pane::*; + pub mod utils; use utils::*; pub mod tile; use tile::*; -mod signal; -pub use signal::*; - pub mod context; use context::*; pub struct World { panes: Vec, } - -pub struct Pane { - tiles: Vec, - width: NonZeroUsize, - height: NonZeroUsize, - - signals: Vec>, -} - -impl Pane { - pub fn empty(width: usize, height: usize) -> Option { - // TODO: check that width * height is a valid usize - - Some(Self { - width: width.try_into().ok()?, - height: height.try_into().ok()?, - tiles: vec![FullTile::default(); width * height], - - signals: vec![], - }) - } - - /// Returns `Some((x + Δx, y + Δy))` iff `(x + Δx, y + Δy)` is inside the world - // SAFETY: this function may *not* access `self.signals`, `∀x, self.tiles[x].cell` or `∀x, self.tiles[x].signal` - #[inline] - pub fn offset(&self, position: (usize, usize), offset: (i8, i8)) -> Option<(usize, usize)> { - if offset.0 < 0 && (-offset.0) as usize > position.0 - || offset.1 < 0 && (-offset.1) as usize > position.1 - { - return None; - } - - // TODO: check that position and position + offset are valid isize values - let new_pos = ( - (position.0 as isize + offset.0 as isize) as usize, - (position.1 as isize + offset.1 as isize) as usize, - ); - - if new_pos.0 < self.width.get() && new_pos.1 < self.height.get() { - Some(new_pos) - } else { - None - } - } - - #[inline] - pub fn get<'b>(&'b self, position: (usize, usize)) -> Option<&'b FullTile> { - if !self.in_bounds(position) { - return None; - } - - self.tiles.get(position.1 * self.width.get() + position.0) - } - - #[inline] - pub fn get_mut<'b>(&'b mut self, position: (usize, usize)) -> Option<&'b mut FullTile> { - if !self.in_bounds(position) { - return None; - } - - self.tiles.get_mut(position.1 * self.width.get() + position.0) - } - - #[inline] - pub fn in_bounds(&self, position: (usize, usize)) -> bool { - position.0 < self.width.get() && position.1 < self.height.get() - } -} diff --git a/stackline/src/pane.rs b/stackline/src/pane.rs new file mode 100644 index 0000000..6aa4f6d --- /dev/null +++ b/stackline/src/pane.rs @@ -0,0 +1,116 @@ +use super::*; + +#[derive(Debug)] +pub struct Pane { + tiles: Vec, + width: NonZeroUsize, + height: NonZeroUsize, + + signals: Vec>, +} + +impl Pane { + pub fn empty(width: usize, height: usize) -> Option { + // TODO: check that width * height is a valid usize + let length = width.checked_mul(height)?; + + Some(Self { + width: width.try_into().ok()?, + height: height.try_into().ok()?, + tiles: vec![FullTile::default(); length], + + signals: vec![], + }) + } + + /// Returns `Some((x + Δx, y + Δy))` iff `(x + Δx, y + Δy)` is inside the world + // SAFETY: this function may *not* access `self.signals`, `∀x, self.tiles[x].cell` or `∀x, self.tiles[x].signal` + #[inline] + pub fn offset(&self, position: (usize, usize), offset: (i8, i8)) -> Option<(usize, usize)> { + if offset.0 < 0 && (-offset.0) as usize > position.0 + || offset.1 < 0 && (-offset.1) as usize > position.1 + { + return None; + } + + // TODO: check that position and position + offset are valid isize values + let new_pos = ( + (position.0 as isize + offset.0 as isize) as usize, + (position.1 as isize + offset.1 as isize) as usize, + ); + + if new_pos.0 < self.width.get() && new_pos.1 < self.height.get() { + Some(new_pos) + } else { + None + } + } + + // SAFETY: this function may not access `self.signals`, nor may it read the contents of `self.tiles[position]` + #[inline] + pub fn get<'b>(&'b self, position: (usize, usize)) -> Option<&'b FullTile> { + if !self.in_bounds(position) { + return None; + } + + self.tiles.get(position.1 * self.width.get() + position.0) + } + + #[inline] + pub fn get_mut<'b>(&'b mut self, position: (usize, usize)) -> Option<&'b mut FullTile> { + if !self.in_bounds(position) { + return None; + } + + self.tiles.get_mut(position.1 * self.width.get() + position.0) + } + + // SAFETY: may only access `self[pos].signal` and `self.signals` + #[inline] + pub fn set_signal(&mut self, position: (usize, usize), signal: Signal) -> Option> { + let signal = self.get_mut(position)?.set_signal(signal)?; + self.signals.push(signal.clone()); + Some(signal) + } + + #[inline] + pub fn in_bounds(&self, position: (usize, usize)) -> bool { + position.0 < self.width.get() && position.1 < self.height.get() + } + + #[inline] + pub fn update(&mut self, position: (usize, usize)) -> Option<()> { + let (ctx, tile) = UpdateContext::new(self, position)?; + + tile.update(ctx); + + Some(()) + } + + // TODO: update_all (requires FullTile::state) + + #[inline] + pub fn transmit(&mut self, position: (usize, usize)) -> Option<()> { + let (ctx, tile, signal) = TransmitContext::new(self, position)?; + + tile.transmit(signal, ctx); + + Some(()) + } + + pub fn transmit_all(&mut self) { + // TODO: store a second buffer and perform swap reads + for signal in std::mem::replace(&mut self.signals, vec![]) { + if let Some(upgraded) = signal.upgrade() { + let position = upgraded.position(); + let _ = self.transmit(position); // May return None if the signal was aliased + } + } + } + + /// Returns an iterator over the tiles and their coordinates + #[inline] + pub fn tiles<'b>(&'b self) -> impl Iterator + 'b { + self.tiles.iter().enumerate().map(move |(i, v)| (i % self.width, i / self.width, v)) + } +} diff --git a/stackline/src/signal.rs b/stackline/src/signal.rs index 4d8b533..d9a1524 100644 --- a/stackline/src/signal.rs +++ b/stackline/src/signal.rs @@ -2,13 +2,35 @@ use super::*; #[derive(Clone, Debug)] pub struct Signal { - pub direction: Direction, + direction: Direction, + position: (usize, usize), } impl Signal { - pub fn clone_with_dir(&self, direction: Direction) -> Self { + pub fn empty(position: (usize, usize), direction: Direction) -> Self { + Self { + direction, + position + } + } + + pub fn clone_move(&self, direction: Direction) -> Option { let mut res = self.clone(); res.direction = direction; - res + + let (dx, dy) = direction.into_offset(); + + res.position.0 = (res.position.0 as isize + dx as isize).try_into().ok()?; + res.position.1 = (res.position.1 as isize + dy as isize).try_into().ok()?; + + Some(res) + } + + pub fn direction(&self) -> Direction { + self.direction + } + + pub fn position(&self) -> (usize, usize) { + self.position } } diff --git a/stackline/src/tile/mod.rs b/stackline/src/tile/mod.rs index 2fae950..3f2718f 100644 --- a/stackline/src/tile/mod.rs +++ b/stackline/src/tile/mod.rs @@ -47,39 +47,60 @@ impl FullTile { } /// Returns the internal state of this full tile + #[inline] pub fn get<'b>(&'b self) -> Option<&'b AnyTile> { self.cell.as_ref() } /// Returns the signal of this tile + #[inline] pub fn signal<'b>(&'b self) -> Option<&'b Rc> { self.signal.as_ref() } + #[inline] pub(crate) fn take_signal(&mut self) -> Option> { std::mem::take(&mut self.signal) } + #[inline] pub(crate) fn get_mut<'b>(&'b mut self) -> Option<&'b mut AnyTile> { self.cell.as_mut() } } impl Default for FullTile { + #[inline] fn default() -> Self { Self::new(None) } } +impl From for FullTile { + #[inline] + fn from(tile: T) -> Self { + Self::new(Some(AnyTile::new(tile))) + } +} + +impl From<()> for FullTile { + #[inline] + fn from(_empty: ()) -> Self { + Self::new(None) + } +} + pub trait Tile: DynClone + std::fmt::Debug { /// Function to be called when the tile needs to update its internal state. /// During the "update" phase, the tile may access its signal and the other tiles immutably. + #[inline] fn update<'b>(&'b mut self, _context: UpdateContext<'b>) {} /// Function that will be called if the tile has a signal. fn transmit<'b>(&'b self, signal: Rc, context: TransmitContext<'b>); /// Should return true iff the tile accepts a signal travelling in `Direction` + #[inline] fn accepts_signal(&self, _direction: Direction) -> bool { true } @@ -89,17 +110,85 @@ pub trait Tile: DynClone + std::fmt::Debug { pub struct AnyTile(Box); impl AnyTile { - fn update<'b>(&'b mut self, ctx: UpdateContext<'b>) { + #[inline] + pub fn new(tile: T) -> Self { + Self(Box::new(tile)) + } + + #[inline] + pub fn update<'b>(&'b mut self, ctx: UpdateContext<'b>) { self.0.update(ctx) } - fn accepts_signal(&self, direction: Direction) -> bool { + #[inline] + pub fn transmit<'b>(&'b self, signal: Rc, context: TransmitContext<'b>) { + self.0.transmit(signal, context) + } + + #[inline] + pub fn accepts_signal(&self, direction: Direction) -> bool { self.0.accepts_signal(direction) } } impl Clone for AnyTile { + #[inline] fn clone(&self) -> Self { Self(clone_box(self.0.as_ref())) } } + +#[cfg(test)] +mod crate_macros { + #[macro_export] + macro_rules! test_tile_setup { + ( $width:expr, $height:expr, [ $( $x:expr ),* ] ) => {{ + assert!($width > 0); + assert!($height > 0); + let mut pane = Pane::empty($width, $height).unwrap(); + let mut index = 0; + + $( + { + let x = index % $width; + let y = index / $width; + *pane.get_mut((x, y)).unwrap() = FullTile::from($x); + index += 1; + } + )* + + assert!(index == $width * $height); + + pane + }} + } + + #[macro_export] + macro_rules! test_set_signal { + ( $pane:expr, $pos:expr, $dir:expr ) => { + $pane.set_signal($pos, Signal::empty($pos, $dir)).unwrap(); + } + } + + #[macro_export] + macro_rules! assert_signal { + ( $pane:expr, $pos:expr ) => {{ + let signal = $pane.get($pos).expect(&format!("Couldn't get tile at {:?}", $pos)).signal(); + assert!(signal.is_some()); + signal + }}; + + ( $pane:expr, $pos:expr, [ $( $data:expr ),* ] ) => {{ + let signal = assert_signal!($pane, $pos); + // TODO: check that signal.data == data + }} + } + + #[macro_export] + macro_rules! assert_no_signal { + ( $pane:expr, $pos:expr) => {{ + let signal = $pane.get($pos).expect(&format!("Couldn't get tile at {:?}", $pos)).signal(); + assert!(signal.is_none()); + }} + } +} diff --git a/stackline/src/tile/wire.rs b/stackline/src/tile/wire.rs index 569c3c8..1b95498 100644 --- a/stackline/src/tile/wire.rs +++ b/stackline/src/tile/wire.rs @@ -5,17 +5,23 @@ use super::*; #[derive(Clone, Debug)] pub struct Wire(Orientation); +impl Wire { + pub fn new(orientation: Orientation) -> Self { + Self(orientation) + } +} + impl Tile for Wire { fn transmit<'b>(&'b self, signal: Rc, context: TransmitContext<'b>) { for &direction in self.0.into_directions() { - if direction == signal.direction.opposite() { + if direction == signal.direction().opposite() { continue; } if let Some(new_pos) = context.offset(direction.into_offset()) { let tile = context.get(new_pos); if tile.map(|t| t.accepts_signal(direction)).unwrap_or(false) { - context.send(new_pos, (*signal).clone_with_dir(direction)); + context.send(new_pos, signal.clone_move(direction).unwrap()); } } } @@ -29,6 +35,12 @@ impl Tile for Wire { #[derive(Clone, Debug)] pub struct Diode(Direction); +impl Diode { + pub fn new(direction: Direction) -> Self { + Self(direction) + } +} + // impl Tile for Diode { // fn update(&mut self, world: &mut World, signal: &mut Option>, pos: (usize, usize)) { // if let Some(signal) = std::mem::take(signal) { @@ -41,3 +53,57 @@ pub struct Diode(Direction); // } // } // } + +#[cfg(test)] +mod test { + use super::*; + + #[test] + fn test_wire_transmit() { + use crate::Orientation::*; + + let mut pane = test_tile_setup!(3, 2, [ + Wire::new(Horizontal), Wire::new(Any), Wire::new(Horizontal), + (), Wire::new(Vertical), () + ]); + + // Test the signal going from left to right + test_set_signal!(pane, (0, 0), Direction::Right); + + pane.transmit_all(); + + assert_signal!(pane, (1, 0)); + assert_no_signal!(pane, (0, 0)); + assert_no_signal!(pane, (2, 0)); + assert_no_signal!(pane, (1, 1)); + + pane.transmit_all(); + + assert_signal!(pane, (2, 0)); + assert_signal!(pane, (1, 1)); + assert_no_signal!(pane, (0, 0)); + assert_no_signal!(pane, (1, 0)); + + pane.transmit_all(); + for (_, _, tile) in pane.tiles() { + assert!(tile.signal().is_none()); + } + + // Test the signal going from right to left + test_set_signal!(pane, (2, 0), Direction::Left); + + pane.transmit_all(); + + assert_signal!(pane, (1, 0)); + assert_no_signal!(pane, (0, 0)); + assert_no_signal!(pane, (2, 0)); + assert_no_signal!(pane, (1, 1)); + + pane.transmit_all(); + + assert_signal!(pane, (0, 0)); + assert_signal!(pane, (1, 1)); + assert_no_signal!(pane, (2, 0)); + assert_no_signal!(pane, (1, 0)); + } +}