From 5e5854016200dd87a7eb340b834c6461a2e9f907 Mon Sep 17 00:00:00 2001 From: Philip Craig Date: Wed, 3 Nov 2021 13:50:39 +1000 Subject: [PATCH] Avoid use of unsafe for Vec storage --- src/read/cfi.rs | 61 ++++++++++++-- src/read/op.rs | 39 +++++++-- src/read/util.rs | 211 +++++++++++++++++++++++++---------------------- 3 files changed, 197 insertions(+), 114 deletions(-) diff --git a/src/read/cfi.rs b/src/read/cfi.rs index b6a81f3f..58af3abc 100644 --- a/src/read/cfi.rs +++ b/src/read/cfi.rs @@ -7,7 +7,7 @@ use core::iter::FromIterator; use core::mem; use core::num::Wrapping; -use super::util::{ArrayLike, ArrayVec}; +use super::util::{ArrayLike, ArrayLikeSealed, ArrayLikeStorage}; use crate::common::{DebugFrameOffset, EhFrameOffset, Encoding, Format, Register, SectionId}; use crate::constants::{self, DwEhPe}; use crate::endianity::Endianity; @@ -1821,12 +1821,12 @@ impl UnwindContextStorage for StoreOnHeap { /// # unreachable!() /// # } /// ``` -#[derive(Clone, PartialEq, Eq)] +#[derive(Clone)] pub struct UnwindContext = StoreOnHeap> { // Stack of rows. The last row is the row currently being built by the // program. There is always at least one row. The vast majority of CFI // programs will only ever have one row on the stack. - stack: ArrayVec, + stack: ::Storage, // If we are evaluating an FDE's instructions, then `is_initialized` will be // `true`. If `initial_rule` is `Some`, then the initial register rules are either @@ -1841,7 +1841,10 @@ pub struct UnwindContext = StoreOnHeap> { is_initialized: bool, } -impl> Debug for UnwindContext { +impl> Debug for UnwindContext +where + ::Storage: fmt::Debug, +{ fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { f.debug_struct("UnwindContext") .field("stack", &self.stack) @@ -1851,6 +1854,25 @@ impl> Debug for UnwindContext { } } +impl> PartialEq for UnwindContext +where + ::Storage: PartialEq, + R: PartialEq, +{ + fn eq(&self, other: &Self) -> bool { + self.stack == other.stack + && self.initial_rule == other.initial_rule + && self.is_initialized == other.is_initialized + } +} + +impl> Eq for UnwindContext +where + ::Storage: Eq, + R: Eq, +{ +} + impl> Default for UnwindContext { fn default() -> Self { Self::new_in() @@ -2044,7 +2066,6 @@ impl> UnwindContext { /// > the ones above them. The whole table can be represented quite compactly by /// > recording just the differences starting at the beginning address of each /// > subroutine in the program. -#[derive(Debug)] pub struct UnwindTable<'a, 'ctx, R: Reader, A: UnwindContextStorage = StoreOnHeap> { code_alignment_factor: Wrapping, data_alignment_factor: Wrapping, @@ -2056,6 +2077,24 @@ pub struct UnwindTable<'a, 'ctx, R: Reader, A: UnwindContextStorage = StoreOn ctx: &'ctx mut UnwindContext, } +impl<'a, 'ctx, R: Reader, S: UnwindContextStorage> Debug for UnwindTable<'a, 'ctx, R, S> +where + ::Storage: fmt::Debug, +{ + fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + f.debug_struct("UnwindTable") + .field("code_alignment_factor", &self.code_alignment_factor) + .field("data_alignment_factor", &self.data_alignment_factor) + .field("next_start_address", &self.next_start_address) + .field("last_end_address", &self.last_end_address) + .field("returned_last_row", &self.returned_last_row) + .field("current_row_valid", &self.current_row_valid) + .field("instructions", &self.instructions) + .field("ctx", &self.ctx) + .finish() + } +} + /// # Signal Safe Methods /// /// These methods are guaranteed not to allocate, acquire locks, or perform any @@ -2357,10 +2396,13 @@ impl<'a, 'ctx, R: Reader, A: UnwindContextStorage> UnwindTable<'a, 'ctx, R, A // - https://github.com/libunwind/libunwind/blob/11fd461095ea98f4b3e3a361f5a8a558519363fa/include/tdep-arm/dwarf-config.h#L31 // - https://github.com/libunwind/libunwind/blob/11fd461095ea98f4b3e3a361f5a8a558519363fa/include/tdep-mips/dwarf-config.h#L31 struct RegisterRuleMap = StoreOnHeap> { - rules: ArrayVec, + rules: ::Storage, } -impl> Debug for RegisterRuleMap { +impl> Debug for RegisterRuleMap +where + ::Storage: Debug, +{ fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { f.debug_struct("RegisterRuleMap") .field("rules", &self.rules) @@ -2507,7 +2549,10 @@ pub struct UnwindTableRow = StoreOnHeap> { registers: RegisterRuleMap, } -impl> Debug for UnwindTableRow { +impl> Debug for UnwindTableRow +where + ::Storage: Debug, +{ fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { f.debug_struct("UnwindTableRow") .field("start_address", &self.start_address) diff --git a/src/read/op.rs b/src/read/op.rs index 2ca6247b..aa10d6b3 100644 --- a/src/read/op.rs +++ b/src/read/op.rs @@ -2,9 +2,10 @@ #[cfg(feature = "read")] use alloc::vec::Vec; +use core::fmt; use core::mem; -use super::util::{ArrayLike, ArrayVec}; +use super::util::{ArrayLike, ArrayLikeSealed, ArrayLikeStorage}; use crate::common::{DebugAddrIndex, DebugInfoOffset, Encoding, Register}; use crate::constants; use crate::read::{Error, Reader, ReaderOffset, Result, StoreOnHeap, UnitOffset, Value, ValueType}; @@ -1089,7 +1090,6 @@ impl EvaluationStorage for StoreOnHeap { /// let result = eval.result(); /// println!("{:?}", result); /// ``` -#[derive(Debug)] pub struct Evaluation = StoreOnHeap> { bytecode: R, encoding: Encoding, @@ -1104,20 +1104,43 @@ pub struct Evaluation = StoreOnHeap> { addr_mask: u64, // The stack. - stack: ArrayVec, + stack: ::Storage, // The next operation to decode and evaluate. pc: R, // If we see a DW_OP_call* operation, the previous PC and bytecode // is stored here while evaluating the subroutine. - expression_stack: ArrayVec, + expression_stack: ::Storage, + + result: ::Storage, +} - result: ArrayVec, +impl> fmt::Debug for Evaluation +where + ::Storage: fmt::Debug, + ::Storage: fmt::Debug, + ::Storage: fmt::Debug, +{ + fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + f.debug_struct("Evaluation") + .field("bytecode", &self.bytecode) + .field("encoding", &self.encoding) + .field("object_address", &self.object_address) + .field("max_iterations", &self.max_iterations) + .field("iteration", &self.iteration) + .field("state", &self.state) + .field("addr_mask", &self.addr_mask) + .field("stack", &self.stack) + .field("pc", &self.pc) + .field("expression_stack", &self.expression_stack) + .field("result", &self.result) + .finish() + } } #[cfg(feature = "read")] -impl Evaluation { +impl Evaluation { /// Create a new DWARF expression evaluator. /// /// The new evaluator is created without an initial value, without @@ -1132,7 +1155,7 @@ impl Evaluation { /// Panics if this `Evaluation` has not been driven to completion. pub fn result(self) -> Vec> { match self.state { - EvaluationState::Complete => self.result.into_vec(), + EvaluationState::Complete => self.result, _ => { panic!("Called `Evaluation::result` on an `Evaluation` that has not been completed") } @@ -1258,7 +1281,7 @@ impl> Evaluation { if index >= len { return Err(Error::NotEnoughStackItems); } - let value = self.stack[len - index - 1]; + let value = *self.stack.get(len - index - 1).unwrap(); self.push(value)?; } Operation::Swap => { diff --git a/src/read/util.rs b/src/read/util.rs index dd2af818..0ad4d323 100644 --- a/src/read/util.rs +++ b/src/read/util.rs @@ -1,122 +1,151 @@ #[cfg(feature = "read")] -use alloc::boxed::Box; -#[cfg(feature = "read")] use alloc::vec::Vec; use core::fmt; use core::mem::MaybeUninit; -use core::ops; +use core::ops::{Deref, DerefMut}; use core::ptr; use core::slice; -mod sealed { - // SAFETY: Implementer must not modify the content in storage. - pub unsafe trait Sealed { - type Storage; - - fn new_storage() -> Self::Storage; - - fn grow(_storage: &mut Self::Storage, _additional: usize) -> Result<(), CapacityFull> { - Err(CapacityFull) - } - } - - #[derive(Clone, Copy, Debug)] - pub struct CapacityFull; -} - -use sealed::*; - /// Marker trait for types that can be used as backing storage when a growable array type is needed. /// /// This trait is sealed and cannot be implemented for types outside this crate. -pub trait ArrayLike: Sealed { - /// Type of the elements being stored. - type Item; - - #[doc(hidden)] - fn as_slice(storage: &Self::Storage) -> &[MaybeUninit]; - - #[doc(hidden)] - fn as_mut_slice(storage: &mut Self::Storage) -> &mut [MaybeUninit]; -} +pub trait ArrayLike: ArrayLikeSealed {} // Use macro since const generics can't be used due to MSRV. macro_rules! impl_array { () => {}; ($n:literal $($rest:tt)*) => { // SAFETY: does not modify the content in storage. - unsafe impl Sealed for [T; $n] { - type Storage = [MaybeUninit; $n]; + unsafe impl Array for [MaybeUninit; $n] { + type Item = T; - fn new_storage() -> Self::Storage { + fn new_storage() -> Self { // SAFETY: An uninitialized `[MaybeUninit<_>; _]` is valid. unsafe { MaybeUninit::uninit().assume_init() } } - } - - impl ArrayLike for [T; $n] { - type Item = T; - fn as_slice(storage: &Self::Storage) -> &[MaybeUninit] { - storage + fn as_slice(&self) -> &[MaybeUninit] { + &self[..] } - fn as_mut_slice(storage: &mut Self::Storage) -> &mut [MaybeUninit] { - storage + fn as_mut_slice(&mut self) -> &mut [MaybeUninit] { + &mut self[..] } } + impl ArrayLike for [T; $n] {} + impl ArrayLikeSealed for [T; $n] { + type Item = T; + type Storage = ArrayVec<[MaybeUninit; $n]>; + } + impl_array!($($rest)*); } } impl_array!(0 1 2 3 4 8 16 32 64 128 192); -#[cfg(feature = "read")] -unsafe impl Sealed for Vec { - type Storage = Box<[MaybeUninit]>; +impl ArrayLike for Vec {} +impl ArrayLikeSealed for Vec { + type Item = T; + type Storage = Vec; +} + +impl ArrayLikeStorage for Vec { + fn len(&self) -> usize { + Vec::len(self) + } + + fn get(&self, index: usize) -> Option<&T> { + self.as_slice().get(index) + } - fn new_storage() -> Self::Storage { - Box::new([]) + fn clear(&mut self) { + Vec::clear(self) } - fn grow(storage: &mut Self::Storage, additional: usize) -> Result<(), CapacityFull> { - let mut vec: Vec<_> = core::mem::replace(storage, Box::new([])).into(); - vec.reserve(additional); - // SAFETY: This is a `Vec` of `MaybeUninit`. - unsafe { vec.set_len(vec.capacity()) }; - *storage = vec.into_boxed_slice(); + fn pop(&mut self) -> Option { + Vec::pop(self) + } + + fn try_push(&mut self, value: T) -> Result<(), CapacityFull> { + Vec::push(self, value); Ok(()) } + + fn try_insert(&mut self, index: usize, element: T) -> Result<(), CapacityFull> { + Vec::insert(self, index, element); + Ok(()) + } + + fn swap_remove(&mut self, index: usize) -> T { + Vec::swap_remove(self, index) + } } -#[cfg(feature = "read")] -impl ArrayLike for Vec { - type Item = T; +pub(crate) use sealed::*; +mod sealed { + use core::mem::MaybeUninit; + use core::ops::{Deref, DerefMut}; + + pub trait ArrayLikeSealed { + /// The type of item stored in the array. + type Item: Clone; - fn as_slice(storage: &Self::Storage) -> &[MaybeUninit] { - storage + /// The container for the array. + /// + /// This allows us keep `ArrayVec` as an implementation detail. + type Storage: ArrayLikeStorage; } - fn as_mut_slice(storage: &mut Self::Storage) -> &mut [MaybeUninit] { - storage + pub trait ArrayLikeStorage: Default + Clone + Deref + DerefMut { + fn len(&self) -> usize; + fn get(&self, index: usize) -> Option<&T>; + fn clear(&mut self); + fn pop(&mut self) -> Option; + fn try_push(&mut self, value: T) -> Result<(), CapacityFull>; + fn try_insert(&mut self, index: usize, element: T) -> Result<(), CapacityFull>; + fn swap_remove(&mut self, index: usize) -> T; + } + + /// A trait for types that can be used in `ArrayVec`. + /// + /// SAFETY: Implementer must not modify the content in storage. + pub unsafe trait Array { + type Item: Clone; + fn new_storage() -> Self; + fn as_slice(&self) -> &[MaybeUninit]; + fn as_mut_slice(&mut self) -> &mut [MaybeUninit]; } -} -pub(crate) struct ArrayVec { - storage: A::Storage, - len: usize, + #[derive(Clone, Copy, Debug)] + pub struct CapacityFull; + + pub struct ArrayVec { + pub(super) storage: A, + pub(super) len: usize, + } } -impl ArrayVec { - pub fn new() -> Self { +impl ArrayVec { + pub(crate) fn new() -> Self { Self { storage: A::new_storage(), len: 0, } } +} + +impl ArrayLikeStorage for ArrayVec { + fn len(&self) -> usize { + self.len + } - pub fn clear(&mut self) { + fn get(&self, index: usize) -> Option<&A::Item> { + self.deref().get(index) + } + + fn clear(&mut self) { let ptr: *mut [A::Item] = &mut **self; // Set length first so the type invariant is upheld even if `drop_in_place` panicks. self.len = 0; @@ -124,11 +153,10 @@ impl ArrayVec { unsafe { ptr::drop_in_place(ptr) }; } - pub fn try_push(&mut self, value: A::Item) -> Result<(), CapacityFull> { - let mut storage = A::as_mut_slice(&mut self.storage); + fn try_push(&mut self, value: A::Item) -> Result<(), CapacityFull> { + let storage = A::as_mut_slice(&mut self.storage); if self.len >= storage.len() { - A::grow(&mut self.storage, 1)?; - storage = A::as_mut_slice(&mut self.storage); + return Err(CapacityFull); } storage[self.len] = MaybeUninit::new(value); @@ -136,13 +164,12 @@ impl ArrayVec { Ok(()) } - pub fn try_insert(&mut self, index: usize, element: A::Item) -> Result<(), CapacityFull> { + fn try_insert(&mut self, index: usize, element: A::Item) -> Result<(), CapacityFull> { assert!(index <= self.len); - let mut storage = A::as_mut_slice(&mut self.storage); + let storage = A::as_mut_slice(&mut self.storage); if self.len >= storage.len() { - A::grow(&mut self.storage, 1)?; - storage = A::as_mut_slice(&mut self.storage); + return Err(CapacityFull); } // SAFETY: storage[index] is filled later. @@ -155,48 +182,36 @@ impl ArrayVec { Ok(()) } - pub fn pop(&mut self) -> Option { + fn pop(&mut self) -> Option { if self.len == 0 { None } else { self.len -= 1; // SAFETY: this element is valid and we "forget" it by setting the length. - Some(unsafe { A::as_slice(&mut self.storage)[self.len].as_ptr().read() }) + Some(unsafe { A::as_slice(&self.storage)[self.len].as_ptr().read() }) } } - pub fn swap_remove(&mut self, index: usize) -> A::Item { + fn swap_remove(&mut self, index: usize) -> A::Item { assert!(self.len > 0); A::as_mut_slice(&mut self.storage).swap(index, self.len - 1); self.pop().unwrap() } } -#[cfg(feature = "read")] -impl ArrayVec> { - pub fn into_vec(mut self) -> Vec { - let len = core::mem::replace(&mut self.len, 0); - let storage = core::mem::replace(&mut self.storage, Box::new([])); - let slice = Box::leak(storage); - debug_assert!(len <= slice.len()); - // SAFETY: valid elements. - unsafe { Vec::from_raw_parts(slice.as_ptr() as _, len, slice.len()) } - } -} - -impl Drop for ArrayVec { +impl Drop for ArrayVec { fn drop(&mut self) { self.clear(); } } -impl Default for ArrayVec { +impl Default for ArrayVec { fn default() -> Self { Self::new() } } -impl ops::Deref for ArrayVec { +impl Deref for ArrayVec { type Target = [A::Item]; fn deref(&self) -> &[A::Item] { @@ -207,7 +222,7 @@ impl ops::Deref for ArrayVec { } } -impl ops::DerefMut for ArrayVec { +impl DerefMut for ArrayVec { fn deref_mut(&mut self) -> &mut [A::Item] { let slice = &mut A::as_mut_slice(&mut self.storage); debug_assert!(self.len <= slice.len()); @@ -216,7 +231,7 @@ impl ops::DerefMut for ArrayVec { } } -impl Clone for ArrayVec +impl Clone for ArrayVec where A::Item: Clone, { @@ -229,7 +244,7 @@ where } } -impl PartialEq for ArrayVec +impl PartialEq for ArrayVec where A::Item: PartialEq, { @@ -238,9 +253,9 @@ where } } -impl Eq for ArrayVec where A::Item: Eq {} +impl Eq for ArrayVec where A::Item: Eq {} -impl fmt::Debug for ArrayVec +impl fmt::Debug for ArrayVec where A::Item: fmt::Debug, {