From 8c5cabb687cbfd81ccfd497a42ea9056b8f0eec0 Mon Sep 17 00:00:00 2001 From: Tei Roberts Date: Thu, 24 Aug 2023 01:13:08 +0200 Subject: [PATCH 01/11] fix(miri): box transmute and invalid references The general causes for the miri invalidation is the prevelant use of `Box` and its references to `ErrorImpl<()>`. `mem::transmute` does not preserve the tag stack for transmuting the boxes. Additionally, having references to `ErrorImpl<()>` which has a different layout than the allocation or `ErrorImpl` for some unknown `E`. This causes the new "untyped" reference to now have a provenance that includes the size of E and thus is outside the provenance. --- src/context.rs | 4 +- src/error.rs | 266 ++++++++++++++++++++++++++----------------- src/fmt.rs | 21 ++-- src/lib.rs | 4 +- src/ptr.rs | 196 +++++++++++++++++++++++++++++++ src/wrapper.rs | 3 + tests/compiletest.rs | 1 + 7 files changed, 379 insertions(+), 116 deletions(-) create mode 100644 src/ptr.rs diff --git a/src/context.rs b/src/context.rs index fef121f..843240f 100644 --- a/src/context.rs +++ b/src/context.rs @@ -1,4 +1,4 @@ -use crate::error::ContextError; +use crate::error::{ContextError, ErrorImpl}; use crate::{ContextCompat, Report, StdError, WrapErr}; use core::fmt::{self, Debug, Display, Write}; @@ -158,7 +158,7 @@ where D: Display, { fn source(&self) -> Option<&(dyn StdError + 'static)> { - Some(self.error.inner.error()) + Some(ErrorImpl::error(self.error.inner.as_ptr())) } } diff --git a/src/error.rs b/src/error.rs index d6a2433..4e07317 100644 --- a/src/error.rs +++ b/src/error.rs @@ -1,4 +1,5 @@ use crate::chain::Chain; +use crate::ptr::{MutPtr, OwnedPtr, RefPtr}; use crate::EyreHandler; use crate::{Report, StdError}; use core::any::TypeId; @@ -70,6 +71,7 @@ impl Report { } #[cfg_attr(track_caller, track_caller)] + /// Creates a new error from an implementor of [`std::error::Error`] pub(crate) fn from_std(error: E) -> Self where E: StdError + Send + Sync + 'static, @@ -191,20 +193,24 @@ impl Report { where E: StdError + Send + Sync + 'static, { - let inner = Box::new(ErrorImpl { + let inner = ErrorImpl { vtable, handler, _object: error, - }); + }; + // Erase the concrete type of E from the compile-time type system. This // is equivalent to the safe unsize coersion from Box> to // Box> except that the // result is a thin pointer. The necessary behavior for manipulating the // underlying ErrorImpl is preserved in the vtable provided by the // caller rather than a builtin fat pointer vtable. - let erased = mem::transmute::>, Box>>(inner); - let inner = ManuallyDrop::new(erased); - Report { inner } + // let erased = mem::transmute::>, Box>>(inner); + let ptr = OwnedPtr::>::new(inner); + + // Safety: the type + let ptr = ptr.cast::>(); + Report { inner: ptr } } /// Create a new error from an error message to wrap the existing error. @@ -264,7 +270,11 @@ impl Report { where D: Display + Send + Sync + 'static, { - let handler = self.inner.handler.take(); + // Safety: this access a `ErrorImpl` as a valid reference to a `ErrorImpl<()>` + // + // As the generic is at the end of the struct and the struct is `repr(C)` this reference + // will be within bounds of the original pointer, and the field will have the same offset + let handler = self.inner_mut().handler.take(); let error: ContextError = ContextError { msg, error: self }; let vtable = &ErrorVTable { @@ -280,6 +290,24 @@ impl Report { unsafe { Report::construct(error, vtable, handler) } } + /// Access the vtable for the current error object. + const fn vtable(&self) -> &'static ErrorVTable { + vtable(self.inner.ptr) + } + + // Safety: this access a `ErrorImpl` as a valid reference to a `ErrorImpl<()>` + // + // As the generic is at the end of the struct and the struct is `repr(C)` this reference + // will be within bounds of the original pointer, and the field will have the same offset + const fn inner_ref(&self) -> &ErrorImpl<()> { + unsafe { self.inner.as_ref() } + } + + /// See: [Self::inner_ref] + fn inner_mut(&mut self) -> &mut ErrorImpl<()> { + unsafe { self.inner.as_mut() } + } + /// An iterator of the chain of source errors contained by this Report. /// /// This iterator will visit every error in the cause chain of this error @@ -302,7 +330,7 @@ impl Report { /// } /// ``` pub fn chain(&self) -> Chain<'_> { - self.inner.chain() + ErrorImpl::chain(self.inner.as_ptr()) } /// The lowest level cause of this error — this error's cause's @@ -342,7 +370,7 @@ impl Report { unsafe { // Use vtable to find NonNull<()> which points to a value of type E // somewhere inside the data structure. - let addr = match (self.inner.vtable.object_downcast)(&self.inner, target) { + let addr = match (self.vtable().object_downcast)(self.inner.as_ptr(), target) { Some(addr) => addr, None => return Err(self), }; @@ -357,10 +385,9 @@ impl Report { // Read Box> from self. Can't move it out because // Report has a Drop impl which we want to not run. let inner = ptr::read(&outer.inner); - let erased = ManuallyDrop::into_inner(inner); // Drop rest of the data structure outside of E. - (erased.vtable.object_drop_rest)(erased, target); + (outer.vtable().object_drop_rest)(inner, target); Ok(error) } @@ -413,7 +440,7 @@ impl Report { unsafe { // Use vtable to find NonNull<()> which points to a value of type E // somewhere inside the data structure. - let addr = (self.inner.vtable.object_downcast)(&self.inner, target)?; + let addr = (self.vtable().object_downcast)(self.inner.as_ptr(), target)?; Some(&*addr.cast::().as_ptr()) } } @@ -427,31 +454,41 @@ impl Report { unsafe { // Use vtable to find NonNull<()> which points to a value of type E // somewhere inside the data structure. - let addr = (self.inner.vtable.object_downcast)(&self.inner, target)?; + let addr = (self.vtable().object_downcast)(self.inner.as_ptr(), target)?; Some(&mut *addr.cast::().as_ptr()) } } /// Get a reference to the Handler for this Report. pub fn handler(&self) -> &dyn EyreHandler { - self.inner.handler.as_ref().unwrap().as_ref() + // TODO: find better ways of doing this without creating a reference to ErrorImpl as it + // creates a valid reference to a pointer of a mismatched layout + self.inner_ref().handler.as_ref().unwrap().as_ref() } /// Get a mutable reference to the Handler for this Report. pub fn handler_mut(&mut self) -> &mut dyn EyreHandler { - self.inner.handler.as_mut().unwrap().as_mut() + unsafe { self.inner.as_mut() } + .handler + .as_mut() + .unwrap() + .as_mut() } /// Get a reference to the Handler for this Report. #[doc(hidden)] pub fn context(&self) -> &dyn EyreHandler { - self.inner.handler.as_ref().unwrap().as_ref() + self.inner_ref().handler.as_ref().unwrap().as_ref() } /// Get a mutable reference to the Handler for this Report. #[doc(hidden)] pub fn context_mut(&mut self) -> &mut dyn EyreHandler { - self.inner.handler.as_mut().unwrap().as_mut() + unsafe { self.inner.as_mut() } + .handler + .as_mut() + .unwrap() + .as_mut() } } @@ -469,25 +506,25 @@ impl Deref for Report { type Target = dyn StdError + Send + Sync + 'static; fn deref(&self) -> &Self::Target { - self.inner.error() + ErrorImpl::error(self.inner.as_ptr()) } } impl DerefMut for Report { fn deref_mut(&mut self) -> &mut Self::Target { - self.inner.error_mut() + ErrorImpl::error_mut(self.inner.as_mut_ptr()) } } impl Display for Report { fn fmt(&self, formatter: &mut fmt::Formatter<'_>) -> fmt::Result { - self.inner.display(formatter) + ErrorImpl::display(self.inner.as_ptr(), formatter) } } impl Debug for Report { fn fmt(&self, formatter: &mut fmt::Formatter<'_>) -> fmt::Result { - self.inner.debug(formatter) + ErrorImpl::debug(self.inner.as_ptr(), formatter) } } @@ -495,39 +532,40 @@ impl Drop for Report { fn drop(&mut self) { unsafe { // Read Box> from self. - let inner = ptr::read(&self.inner); - let erased = ManuallyDrop::into_inner(inner); - - // Invoke the vtable's drop behavior. - (erased.vtable.object_drop)(erased); + (self.vtable().object_drop)(self.inner); } } } struct ErrorVTable { - object_drop: unsafe fn(Box>), - object_ref: unsafe fn(&ErrorImpl<()>) -> &(dyn StdError + Send + Sync + 'static), - object_mut: unsafe fn(&mut ErrorImpl<()>) -> &mut (dyn StdError + Send + Sync + 'static), + object_drop: unsafe fn(OwnedPtr>), + object_ref: unsafe fn(RefPtr<'_, ErrorImpl<()>>) -> &(dyn StdError + Send + Sync + 'static), + object_mut: unsafe fn(MutPtr<'_, ErrorImpl<()>>) -> &mut (dyn StdError + Send + Sync + 'static), #[allow(clippy::type_complexity)] - object_boxed: unsafe fn(Box>) -> Box, - object_downcast: unsafe fn(&ErrorImpl<()>, TypeId) -> Option>, - object_drop_rest: unsafe fn(Box>, TypeId), + object_boxed: unsafe fn(OwnedPtr>) -> Box, + object_downcast: unsafe fn(RefPtr<'_, ErrorImpl<()>>, TypeId) -> Option>, + object_drop_rest: unsafe fn(OwnedPtr>, TypeId), } -// Safety: requires layout of *e to match ErrorImpl. -unsafe fn object_drop(e: Box>) { +/// # Safety +/// +/// Requires layout of *e to match ErrorImpl. +unsafe fn object_drop(e: OwnedPtr>) { // Cast back to ErrorImpl so that the allocator receives the correct // Layout to deallocate the Box's memory. // Note: This must not use `mem::transmute` because it tries to reborrow the `Unique` // contained in `Box`, which must not be done. In practice this probably won't make any // difference by now, but technically it's unsound. // see: https://github.com/rust-lang/unsafe-code-guidelines/blob/master/wip/stacked-borrows.md - let unerased: Box> = Box::from_raw(Box::into_raw(e).cast()); + eprintln!("Dropping"); + let unerased = e.cast::>().into_box(); drop(unerased); } -// Safety: requires layout of *e to match ErrorImpl. -unsafe fn object_drop_front(e: Box>, target: TypeId) { +/// # Safety +/// +/// Requires layout of *e to match ErrorImpl. +unsafe fn object_drop_front(e: OwnedPtr>, target: TypeId) { // Drop the fields of ErrorImpl other than E as well as the Box allocation, // without dropping E itself. This is used by downcast after doing a // ptr::read to take ownership of the E. @@ -536,74 +574,90 @@ unsafe fn object_drop_front(e: Box>, target: TypeId) { // contained in `Box`, which must not be done. In practice this probably won't make any // difference by now, but technically it's unsound. // see: https://github.com/rust-lang/unsafe-code-guidelines/blob/master/wip/stacked-borrows.m - let unerased: Box>> = Box::from_raw(Box::into_raw(e).cast()); - drop(unerased); + let unerased = e.cast::>().into_box(); + + mem::forget(unerased._object) } -// Safety: requires layout of *e to match ErrorImpl. -unsafe fn object_ref(e: &ErrorImpl<()>) -> &(dyn StdError + Send + Sync + 'static) +/// # Safety +/// +/// Requires layout of *e to match ErrorImpl. +unsafe fn object_ref(e: RefPtr<'_, ErrorImpl<()>>) -> &(dyn StdError + Send + Sync + 'static) where E: StdError + Send + Sync + 'static, { // Attach E's native StdError vtable onto a pointer to self._object. - &(*(e as *const ErrorImpl<()> as *const ErrorImpl))._object + &e.cast::>().as_ref()._object } -// Safety: requires layout of *e to match ErrorImpl. -unsafe fn object_mut(e: &mut ErrorImpl<()>) -> &mut (dyn StdError + Send + Sync + 'static) +/// # Safety +/// +/// Requires layout of *e to match ErrorImpl. +unsafe fn object_mut(e: MutPtr<'_, ErrorImpl<()>>) -> &mut (dyn StdError + Send + Sync + 'static) where E: StdError + Send + Sync + 'static, { // Attach E's native StdError vtable onto a pointer to self._object. - &mut (*(e as *mut ErrorImpl<()> as *mut ErrorImpl))._object + &mut e.cast::>().into_mut()._object } -// Safety: requires layout of *e to match ErrorImpl. -unsafe fn object_boxed(e: Box>) -> Box +/// # Safety +/// +/// Requires layout of *e to match ErrorImpl. +unsafe fn object_boxed(e: OwnedPtr>) -> Box where E: StdError + Send + Sync + 'static, { // Attach ErrorImpl's native StdError vtable. The StdError impl is below. - mem::transmute::>, Box>>(e) + e.cast::>().into_box() } -// Safety: requires layout of *e to match ErrorImpl. -unsafe fn object_downcast(e: &ErrorImpl<()>, target: TypeId) -> Option> +/// # Safety +/// +/// Requires layout of *e to match ErrorImpl. +unsafe fn object_downcast(e: RefPtr<'_, ErrorImpl<()>>, target: TypeId) -> Option> where E: 'static, { if TypeId::of::() == target { // Caller is looking for an E pointer and e is ErrorImpl, take a // pointer to its E field. - let unerased = e as *const ErrorImpl<()> as *const ErrorImpl; - let addr = &(*unerased)._object as *const E as *mut (); + let unerased = e.cast::>(); + let addr = &(unerased.as_ref()._object) as *const E as *mut (); Some(NonNull::new_unchecked(addr)) } else { None } } -// Safety: requires layout of *e to match ErrorImpl>. -unsafe fn context_downcast(e: &ErrorImpl<()>, target: TypeId) -> Option> +/// # Safety +/// +/// Requires layout of *e to match ErrorImpl>. +unsafe fn context_downcast( + e: RefPtr<'_, ErrorImpl<()>>, + target: TypeId, +) -> Option> where D: 'static, E: 'static, { if TypeId::of::() == target { - let unerased = e as *const ErrorImpl<()> as *const ErrorImpl>; - let addr = &(*unerased)._object.msg as *const D as *mut (); + let unerased = e.cast::>>(); + let addr = (&unerased.as_ref()._object.msg as *const D) as *mut (); Some(NonNull::new_unchecked(addr)) } else if TypeId::of::() == target { - let unerased = e as *const ErrorImpl<()> as *const ErrorImpl>; - let addr = &(*unerased)._object.error as *const E as *mut (); + let unerased = e.cast::>>(); + let addr = (&unerased.as_ref()._object.error as *const E) as *mut (); Some(NonNull::new_unchecked(addr)) } else { None } } -// Safety: requires layout of *e to match ErrorImpl>. -unsafe fn context_drop_rest(e: Box>, target: TypeId) +/// # Safety +/// +/// Requires layout of *e to match ErrorImpl>. +unsafe fn context_drop_rest(e: OwnedPtr>, target: TypeId) where D: 'static, E: 'static, @@ -611,67 +665,66 @@ where // Called after downcasting by value to either the D or the E and doing a // ptr::read to take ownership of that value. if TypeId::of::() == target { - let unerased = mem::transmute::< - Box>, - Box, E>>>, - >(e); - drop(unerased); + e.cast::, E>>>() + .into_box(); } else { - let unerased = mem::transmute::< - Box>, - Box>>>, - >(e); - drop(unerased); + debug_assert_eq!(TypeId::of::(), target); + e.cast::>>>() + .into_box(); } } -// Safety: requires layout of *e to match ErrorImpl>. -unsafe fn context_chain_downcast(e: &ErrorImpl<()>, target: TypeId) -> Option> +/// # Safety +/// +/// Requires layout of *e to match ErrorImpl>. +unsafe fn context_chain_downcast( + e: RefPtr<'_, ErrorImpl<()>>, + target: TypeId, +) -> Option> where D: 'static, { - let unerased = e as *const ErrorImpl<()> as *const ErrorImpl>; + let unerased = e.cast::>>(); if TypeId::of::() == target { - let addr = &(*unerased)._object.msg as *const D as *mut (); + let addr = &(unerased.as_ref()._object.msg) as *const D as *mut (); Some(NonNull::new_unchecked(addr)) } else { // Recurse down the context chain per the inner error's vtable. - let source = &(*unerased)._object.error; - (source.inner.vtable.object_downcast)(&source.inner, target) + let source = &unerased.as_ref()._object.error; + (source.vtable().object_downcast)(source.inner.as_ptr(), target) } } -// Safety: requires layout of *e to match ErrorImpl>. -unsafe fn context_chain_drop_rest(e: Box>, target: TypeId) +/// # Safety +/// +/// Requires layout of *e to match ErrorImpl>. +unsafe fn context_chain_drop_rest(e: OwnedPtr>, target: TypeId) where D: 'static, { // Called after downcasting by value to either the D or one of the causes // and doing a ptr::read to take ownership of that value. if TypeId::of::() == target { - let unerased = mem::transmute::< - Box>, - Box, Report>>>, - >(e); + let unerased = e + .cast::, Report>>>() + .into_box(); // Drop the entire rest of the data structure rooted in the next Report. drop(unerased); } else { - let unerased = mem::transmute::< - Box>, - Box>>>, - >(e); + let unerased = e + .cast::>>>() + .into_box(); // Read out a ManuallyDrop>> from the next error. - let inner = ptr::read(&unerased._object.error.inner); + let inner = ptr::read(&unerased.as_ref()._object.error.inner); drop(unerased); - let erased = ManuallyDrop::into_inner(inner); // Recursively drop the next error using the same target typeid. - (erased.vtable.object_drop_rest)(erased, target); + (inner.as_ref().vtable.object_drop_rest)(inner, target); } } // repr C to ensure that E remains in the final position. #[repr(C)] -pub(crate) struct ErrorImpl { +pub(crate) struct ErrorImpl { vtable: &'static ErrorVTable, pub(crate) handler: Option>, // NOTE: Don't use directly. Use only through vtable. Erased type may have @@ -688,29 +741,37 @@ pub(crate) struct ContextError { } impl ErrorImpl { - fn erase(&self) -> &ErrorImpl<()> { + /// Returns a type erased Error + fn erase(&self) -> RefPtr<'_, ErrorImpl<()>> { // Erase the concrete type of E but preserve the vtable in self.vtable // for manipulating the resulting thin pointer. This is analogous to an // unsize coersion. - unsafe { &*(self as *const ErrorImpl as *const ErrorImpl<()>) } + RefPtr::new(self).cast() } } +// Reads the vtable out of `p`. This is the same as `p.as_ref().vtable`, but +// avoids converting `p` into a reference. +const fn vtable(p: NonNull>) -> &'static ErrorVTable { + // Safety: `ErrorVTable` is the first field of `ErrorImpl` and `ErrorImpl` is `repr(C)`. + unsafe { *(p.as_ptr() as *const &'static ErrorVTable) } +} + impl ErrorImpl<()> { - pub(crate) fn error(&self) -> &(dyn StdError + Send + Sync + 'static) { + pub(crate) fn error(this: RefPtr<'_, Self>) -> &(dyn StdError + Send + Sync + 'static) { // Use vtable to attach E's native StdError vtable for the right // original type E. - unsafe { &*(self.vtable.object_ref)(self) } + unsafe { (vtable(this.ptr).object_ref)(this) } } - pub(crate) fn error_mut(&mut self) -> &mut (dyn StdError + Send + Sync + 'static) { + pub(crate) fn error_mut(this: MutPtr<'_, Self>) -> &mut (dyn StdError + Send + Sync + 'static) { // Use vtable to attach E's native StdError vtable for the right // original type E. - unsafe { &mut *(self.vtable.object_mut)(self) } + unsafe { (vtable(this.ptr).object_mut)(this) } } - pub(crate) fn chain(&self) -> Chain<'_> { - Chain::new(self.error()) + pub(crate) fn chain(this: RefPtr<'_, Self>) -> Chain<'_> { + Chain::new(Self::error(this)) } } @@ -719,7 +780,7 @@ where E: StdError, { fn source(&self) -> Option<&(dyn StdError + 'static)> { - self.erase().error().source() + ErrorImpl::<()>::error(self.erase()).source() } } @@ -728,7 +789,7 @@ where E: Debug, { fn fmt(&self, formatter: &mut fmt::Formatter<'_>) -> fmt::Result { - self.erase().debug(formatter) + ErrorImpl::debug(self.erase(), formatter) } } @@ -737,7 +798,7 @@ where E: Display, { fn fmt(&self, formatter: &mut fmt::Formatter<'_>) -> fmt::Result { - Display::fmt(&self.erase().error(), formatter) + Display::fmt(ErrorImpl::error(self.erase()), formatter) } } @@ -747,12 +808,9 @@ impl From for Box { unsafe { // Read Box> from error. Can't move it out because // Report has a Drop impl which we want to not run. - let inner = ptr::read(&outer.inner); - let erased = ManuallyDrop::into_inner(inner); - // Use vtable to attach ErrorImpl's native StdError vtable for // the right original type E. - (erased.vtable.object_boxed)(erased) + (vtable(outer.inner.ptr).object_boxed)(outer.inner) } } } diff --git a/src/fmt.rs b/src/fmt.rs index b4e97a3..5ac1662 100644 --- a/src/fmt.rs +++ b/src/fmt.rs @@ -1,18 +1,21 @@ -use crate::error::ErrorImpl; +use crate::{error::ErrorImpl, ptr::RefPtr}; use core::fmt; impl ErrorImpl<()> { - pub(crate) fn display(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - self.handler + pub(crate) fn display(this: RefPtr<'_, Self>, f: &mut fmt::Formatter<'_>) -> fmt::Result { + unsafe { this.as_ref() } + .handler .as_ref() - .map(|handler| handler.display(self.error(), f)) - .unwrap_or_else(|| core::fmt::Display::fmt(self.error(), f)) + .map(|handler| handler.display(Self::error(this), f)) + .unwrap_or_else(|| core::fmt::Display::fmt(Self::error(this), f)) } - pub(crate) fn debug(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - self.handler + /// Debug formats the error using the captured handler + pub(crate) fn debug(this: RefPtr<'_, Self>, f: &mut fmt::Formatter<'_>) -> fmt::Result { + unsafe { this.as_ref() } + .handler .as_ref() - .map(|handler| handler.debug(self.error(), f)) - .unwrap_or_else(|| core::fmt::Debug::fmt(self.error(), f)) + .map(|handler| handler.debug(Self::error(this), f)) + .unwrap_or_else(|| core::fmt::Debug::fmt(Self::error(this), f)) } } diff --git a/src/lib.rs b/src/lib.rs index 37885b8..93c0afe 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -370,6 +370,7 @@ mod error; mod fmt; mod kind; mod macros; +mod ptr; mod wrapper; use crate::backtrace::Backtrace; @@ -383,6 +384,7 @@ pub use eyre as format_err; /// Compatibility re-export of `eyre` for interop with `anyhow` pub use eyre as anyhow; use once_cell::sync::OnceCell; +use ptr::OwnedPtr; #[doc(hidden)] pub use DefaultHandler as DefaultContext; #[doc(hidden)] @@ -469,7 +471,7 @@ pub use WrapErr as Context; /// [`hook`]: fn.set_hook.html #[must_use] pub struct Report { - inner: ManuallyDrop>>, + inner: OwnedPtr>, } type ErrorHook = diff --git a/src/ptr.rs b/src/ptr.rs new file mode 100644 index 0000000..5b057a7 --- /dev/null +++ b/src/ptr.rs @@ -0,0 +1,196 @@ +use std::{marker::PhantomData, ptr::NonNull}; + +/// An owned pointer +/// +/// **NOTE**: Does not deallocate when dropped +pub(crate) struct OwnedPtr { + pub(crate) ptr: NonNull, +} + +impl Copy for OwnedPtr {} + +impl Clone for OwnedPtr { + fn clone(&self) -> Self { + *self + } +} + +unsafe impl Send for OwnedPtr where T: Send {} +unsafe impl Sync for OwnedPtr where T: Send {} + +impl OwnedPtr { + pub(crate) fn new(value: T) -> Self { + Self::from_boxed(Box::new(value)) + } + + pub(crate) fn from_boxed(boxed: Box) -> Self { + // Safety: `Box::into_raw` is guaranteed to be non-null + Self { + ptr: unsafe { NonNull::new_unchecked(Box::into_raw(boxed)) }, + } + } + + /// Convert the pointer to another type + pub(crate) fn cast(self) -> OwnedPtr { + OwnedPtr { + ptr: self.ptr.cast(), + } + } + + /// Context the pointer into a Box + /// + /// # Safety + /// + /// Dropping the Box will deallocate a layout of `T` and run the destructor of `T`. + /// + /// A cast pointer must therefore be cast back to the original type before calling this method. + pub(crate) unsafe fn into_box(self) -> Box { + Box::from_raw(self.ptr.as_ptr()) + } + + /// Returns a shared reference to the owned value + /// + /// # Safety + /// + /// See: [`NonNull::as_ref`] + #[inline] + pub(crate) const unsafe fn as_ref(&self) -> &T { + self.ptr.as_ref() + } + + /// Returns a mutable reference to the owned value + /// + /// # Safety + /// + /// See: [`NonNull::as_mut`] + #[inline] + pub(crate) unsafe fn as_mut(&mut self) -> &mut T { + self.ptr.as_mut() + } + + pub(crate) const fn as_ptr<'a>(&'a self) -> RefPtr<'a, T> { + RefPtr { + ptr: self.ptr, + _marker: PhantomData, + } + } + + pub(crate) fn as_mut_ptr<'a>(&'a self) -> MutPtr<'a, T> { + MutPtr { + ptr: self.ptr, + _marker: PhantomData, + } + } +} + +/// Convenience lifetime annotated mutable pointer which facilitates returning an inferred lifetime +/// in a `fn` pointer. +pub(crate) struct RefPtr<'a, T: ?Sized> { + pub(crate) ptr: NonNull, + _marker: PhantomData<&'a T>, +} + +/// Safety: RefPtr indicates a shared reference to a value and as such exhibits the same Send + +/// Sync behavior of &'a T +unsafe impl<'a, T: ?Sized> Send for RefPtr<'a, T> where &'a T: Send {} +unsafe impl<'a, T: ?Sized> Sync for RefPtr<'a, T> where &'a T: Sync {} + +impl<'a, T: ?Sized> Copy for RefPtr<'a, T> {} +impl<'a, T: ?Sized> Clone for RefPtr<'a, T> { + fn clone(&self) -> Self { + *self + } +} + +impl<'a, T: ?Sized> RefPtr<'a, T> { + pub(crate) fn new(ptr: &'a T) -> Self { + Self { + ptr: NonNull::from(ptr), + _marker: PhantomData, + } + } + + /// Convert the pointer to another type + pub(crate) fn cast(self) -> RefPtr<'a, U> { + RefPtr { + ptr: self.ptr.cast(), + _marker: PhantomData, + } + } + + /// Returns a shared reference to the owned value + /// + /// # Safety + /// + /// See: [`NonNull::as_ref`] + #[inline] + pub(crate) unsafe fn as_ref(&self) -> &'a T { + self.ptr.as_ref() + } +} + +/// Convenience lifetime annotated mutable pointer which facilitates returning an inferred lifetime +/// in a `fn` pointer. +pub(crate) struct MutPtr<'a, T: ?Sized> { + pub(crate) ptr: NonNull, + _marker: PhantomData<&'a mut T>, +} + +/// Safety: RefPtr indicates an exclusive reference to a value and as such exhibits the same Send + +/// Sync behavior of &'a mut T +unsafe impl<'a, T: ?Sized> Send for MutPtr<'a, T> where &'a mut T: Send {} +unsafe impl<'a, T: ?Sized> Sync for MutPtr<'a, T> where &'a mut T: Sync {} + +impl<'a, T: ?Sized> Copy for MutPtr<'a, T> {} +impl<'a, T: ?Sized> Clone for MutPtr<'a, T> { + fn clone(&self) -> Self { + *self + } +} + +impl<'a, T: ?Sized> MutPtr<'a, T> { + pub(crate) fn new(ptr: &'a mut T) -> Self { + Self { + ptr: NonNull::from(ptr), + _marker: PhantomData, + } + } + + /// Convert the pointer to another type + pub(crate) fn cast(self) -> MutPtr<'a, U> { + MutPtr { + ptr: self.ptr.cast(), + _marker: PhantomData, + } + } + + /// Returns a shared reference to the owned value + /// + /// # Safety + /// + /// See: [`NonNull::as_ref`] + #[inline] + pub(crate) unsafe fn as_ref(&self) -> &'a T { + self.ptr.as_ref() + } + + /// Returns a mutable reference to the owned value + /// + /// # Safety + /// + /// See: [`NonNull::as_mut`] + #[inline] + pub(crate) unsafe fn as_mut(&'a mut self) -> &'a mut T { + self.ptr.as_mut() + } + + /// Returns a mutable reference to the owned value with the lifetime decoupled from self + /// + /// # Safety + /// + /// See: [`NonNull::as_mut`] + #[inline] + pub(crate) unsafe fn into_mut(mut self) -> &'a mut T { + self.ptr.as_mut() + } +} diff --git a/src/wrapper.rs b/src/wrapper.rs index f0334e5..b4da68a 100644 --- a/src/wrapper.rs +++ b/src/wrapper.rs @@ -5,6 +5,9 @@ use core::fmt::{self, Debug, Display}; pub(crate) struct DisplayError(pub(crate) M); #[repr(transparent)] +/// Wraps a Debug + Display type as an error. +/// +/// Its Debug and Display impls are the same as the wrapped type. pub(crate) struct MessageError(pub(crate) M); pub(crate) struct NoneError; diff --git a/tests/compiletest.rs b/tests/compiletest.rs index f9aea23..7974a62 100644 --- a/tests/compiletest.rs +++ b/tests/compiletest.rs @@ -1,4 +1,5 @@ #[rustversion::attr(not(nightly), ignore)] +#[cfg_attr(miri, ignore)] #[test] fn ui() { let t = trybuild::TestCases::new(); From 3640ba8f329d19e91a90cbbaf6d221e3fc093d69 Mon Sep 17 00:00:00 2001 From: Tei Roberts Date: Thu, 24 Aug 2023 01:46:50 +0200 Subject: [PATCH 02/11] fix(miri): downcast_mut using `&mut _ => *const _ => *mut` --- src/error.rs | 103 ++++++++++++++++++++++++++++++++++++++++++--------- src/ptr.rs | 4 +- 2 files changed, 88 insertions(+), 19 deletions(-) diff --git a/src/error.rs b/src/error.rs index 4e07317..8ddb24c 100644 --- a/src/error.rs +++ b/src/error.rs @@ -82,6 +82,7 @@ impl Report { object_mut: object_mut::, object_boxed: object_boxed::, object_downcast: object_downcast::, + object_downcast_mut: object_downcast_mut::, object_drop_rest: object_drop_front::, }; @@ -104,6 +105,7 @@ impl Report { object_mut: object_mut::>, object_boxed: object_boxed::>, object_downcast: object_downcast::, + object_downcast_mut: object_downcast_mut::, object_drop_rest: object_drop_front::, }; @@ -127,6 +129,7 @@ impl Report { object_mut: object_mut::>, object_boxed: object_boxed::>, object_downcast: object_downcast::, + object_downcast_mut: object_downcast_mut::, object_drop_rest: object_drop_front::, }; @@ -151,6 +154,7 @@ impl Report { object_mut: object_mut::>, object_boxed: object_boxed::>, object_downcast: context_downcast::, + object_downcast_mut: context_downcast_mut::, object_drop_rest: context_drop_rest::, }; @@ -172,6 +176,7 @@ impl Report { object_mut: object_mut::, object_boxed: object_boxed::, object_downcast: object_downcast::>, + object_downcast_mut: object_downcast_mut::>, object_drop_rest: object_drop_front::>, }; @@ -283,6 +288,7 @@ impl Report { object_mut: object_mut::>, object_boxed: object_boxed::>, object_downcast: context_chain_downcast::, + object_downcast_mut: context_chain_downcast_mut::, object_drop_rest: context_chain_drop_rest::, }; @@ -441,7 +447,7 @@ impl Report { // Use vtable to find NonNull<()> which points to a value of type E // somewhere inside the data structure. let addr = (self.vtable().object_downcast)(self.inner.as_ptr(), target)?; - Some(&*addr.cast::().as_ptr()) + Some(addr.cast::().as_ref()) } } @@ -454,8 +460,8 @@ impl Report { unsafe { // Use vtable to find NonNull<()> which points to a value of type E // somewhere inside the data structure. - let addr = (self.vtable().object_downcast)(self.inner.as_ptr(), target)?; - Some(&mut *addr.cast::().as_ptr()) + let addr = (self.vtable().object_downcast_mut)(self.inner.as_mut_ptr(), target)?; + Some(addr.cast::().as_mut()) } } @@ -544,6 +550,7 @@ struct ErrorVTable { #[allow(clippy::type_complexity)] object_boxed: unsafe fn(OwnedPtr>) -> Box, object_downcast: unsafe fn(RefPtr<'_, ErrorImpl<()>>, TypeId) -> Option>, + object_downcast_mut: unsafe fn(MutPtr<'_, ErrorImpl<()>>, TypeId) -> Option>, object_drop_rest: unsafe fn(OwnedPtr>, TypeId), } @@ -557,7 +564,6 @@ unsafe fn object_drop(e: OwnedPtr>) { // contained in `Box`, which must not be done. In practice this probably won't make any // difference by now, but technically it's unsound. // see: https://github.com/rust-lang/unsafe-code-guidelines/blob/master/wip/stacked-borrows.md - eprintln!("Dropping"); let unerased = e.cast::>().into_box(); drop(unerased); } @@ -622,9 +628,28 @@ where if TypeId::of::() == target { // Caller is looking for an E pointer and e is ErrorImpl, take a // pointer to its E field. - let unerased = e.cast::>(); - let addr = &(unerased.as_ref()._object) as *const E as *mut (); - Some(NonNull::new_unchecked(addr)) + let unerased = e.cast::>().as_ref(); + Some(NonNull::from(&(unerased._object)).cast::<()>()) + } else { + None + } +} + +/// # Safety +/// +/// Requires layout of *e to match ErrorImpl. +unsafe fn object_downcast_mut( + e: MutPtr<'_, ErrorImpl<()>>, + target: TypeId, +) -> Option> +where + E: 'static, +{ + if TypeId::of::() == target { + // Caller is looking for an E pointer and e is ErrorImpl, take a + // pointer to its E field. + let unerased = e.cast::>().into_mut(); + Some(NonNull::from(&mut (unerased._object)).cast::<()>()) } else { None } @@ -642,18 +667,41 @@ where E: 'static, { if TypeId::of::() == target { - let unerased = e.cast::>>(); - let addr = (&unerased.as_ref()._object.msg as *const D) as *mut (); - Some(NonNull::new_unchecked(addr)) + let unerased = e.cast::>>().as_ref(); + let addr = NonNull::from(&unerased._object.msg).cast::<()>(); + Some(addr) } else if TypeId::of::() == target { - let unerased = e.cast::>>(); - let addr = (&unerased.as_ref()._object.error as *const E) as *mut (); - Some(NonNull::new_unchecked(addr)) + let unerased = e.cast::>>().as_ref(); + let addr = NonNull::from(&unerased._object.error).cast::<()>(); + Some(addr) } else { None } } +/// # Safety +/// +/// Requires layout of *e to match ErrorImpl>. +unsafe fn context_downcast_mut( + e: MutPtr<'_, ErrorImpl<()>>, + target: TypeId, +) -> Option> +where + D: 'static, + E: 'static, +{ + if TypeId::of::() == target { + let unerased = e.cast::>>().into_mut(); + let addr = NonNull::from(&unerased._object.msg).cast::<()>(); + Some(addr) + } else if TypeId::of::() == target { + let unerased = e.cast::>>().into_mut(); + let addr = NonNull::from(&mut unerased._object.error).cast::<()>(); + Some(addr) + } else { + None + } +} /// # Safety /// /// Requires layout of *e to match ErrorImpl>. @@ -684,17 +732,38 @@ unsafe fn context_chain_downcast( where D: 'static, { - let unerased = e.cast::>>(); + let unerased = e.cast::>>().as_ref(); if TypeId::of::() == target { - let addr = &(unerased.as_ref()._object.msg) as *const D as *mut (); - Some(NonNull::new_unchecked(addr)) + let addr = NonNull::from(&unerased._object.msg).cast::<()>(); + Some(addr) } else { // Recurse down the context chain per the inner error's vtable. - let source = &unerased.as_ref()._object.error; + let source = &unerased._object.error; (source.vtable().object_downcast)(source.inner.as_ptr(), target) } } +/// # Safety +/// +/// Requires layout of *e to match ErrorImpl>. +unsafe fn context_chain_downcast_mut( + e: MutPtr<'_, ErrorImpl<()>>, + target: TypeId, +) -> Option> +where + D: 'static, +{ + let unerased = e.cast::>>().into_mut(); + if TypeId::of::() == target { + let addr = NonNull::from(&unerased._object.msg).cast::<()>(); + Some(addr) + } else { + // Recurse down the context chain per the inner error's vtable. + let source = &mut unerased._object.error; + (source.vtable().object_downcast_mut)(source.inner.as_mut_ptr(), target) + } +} + /// # Safety /// /// Requires layout of *e to match ErrorImpl>. diff --git a/src/ptr.rs b/src/ptr.rs index 5b057a7..0646007 100644 --- a/src/ptr.rs +++ b/src/ptr.rs @@ -68,14 +68,14 @@ impl OwnedPtr { self.ptr.as_mut() } - pub(crate) const fn as_ptr<'a>(&'a self) -> RefPtr<'a, T> { + pub(crate) const fn as_ptr(&self) -> RefPtr<'_, T> { RefPtr { ptr: self.ptr, _marker: PhantomData, } } - pub(crate) fn as_mut_ptr<'a>(&'a self) -> MutPtr<'a, T> { + pub(crate) fn as_mut_ptr(&mut self) -> MutPtr<'_, T> { MutPtr { ptr: self.ptr, _marker: PhantomData, From 4547d190d474bef4a06c5d766564edddb3bbb501 Mon Sep 17 00:00:00 2001 From: Tei Roberts Date: Thu, 24 Aug 2023 01:47:54 +0200 Subject: [PATCH 03/11] fix(miri): stub file reading --- tests/test_location.rs | 22 ++++++++++++++++++---- 1 file changed, 18 insertions(+), 4 deletions(-) diff --git a/tests/test_location.rs b/tests/test_location.rs index d851f5a..58b03b1 100644 --- a/tests/test_location.rs +++ b/tests/test_location.rs @@ -46,7 +46,7 @@ fn test_wrap_err() { Box::new(LocationHandler::new(expected_location)) })); - let err = std::fs::read_to_string("totally_fake_path") + let err = read_path("totally_fake_path") .wrap_err("oopsie") .unwrap_err(); @@ -54,6 +54,20 @@ fn test_wrap_err() { println!("{:?}", err); } +#[cfg(not(miri))] +fn read_path(path: &str) -> Result { + std::fs::read_to_string(path) +} + +#[cfg(miri)] +fn read_path(path: &str) -> Result { + // Miri doesn't support reading files, so we just return an error + Err(std::io::Error::new( + std::io::ErrorKind::Other, + "Miri doesn't support reading files", + )) +} + #[test] fn test_wrap_err_with() { let _ = eyre::set_hook(Box::new(|_e| { @@ -61,7 +75,7 @@ fn test_wrap_err_with() { Box::new(LocationHandler::new(expected_location)) })); - let err = std::fs::read_to_string("totally_fake_path") + let err = read_path("totally_fake_path") .wrap_err_with(|| "oopsie") .unwrap_err(); @@ -76,7 +90,7 @@ fn test_context() { Box::new(LocationHandler::new(expected_location)) })); - let err = std::fs::read_to_string("totally_fake_path") + let err = read_path("totally_fake_path") .context("oopsie") .unwrap_err(); @@ -91,7 +105,7 @@ fn test_with_context() { Box::new(LocationHandler::new(expected_location)) })); - let err = std::fs::read_to_string("totally_fake_path") + let err = read_path("totally_fake_path") .with_context(|| "oopsie") .unwrap_err(); From ae66514deee6a1ecd8e431739e8cb4e476075296 Mon Sep 17 00:00:00 2001 From: Tei Roberts Date: Sat, 26 Aug 2023 00:12:22 +0200 Subject: [PATCH 04/11] fix(miri): don't construct temp references of shunk provenance --- src/context.rs | 2 +- src/error.rs | 97 +++++++++++++++++++++++++++----------------------- src/fmt.rs | 4 +-- src/lib.rs | 1 - src/ptr.rs | 53 ++------------------------- 5 files changed, 58 insertions(+), 99 deletions(-) diff --git a/src/context.rs b/src/context.rs index 843240f..a15b41b 100644 --- a/src/context.rs +++ b/src/context.rs @@ -158,7 +158,7 @@ where D: Display, { fn source(&self) -> Option<&(dyn StdError + 'static)> { - Some(ErrorImpl::error(self.error.inner.as_ptr())) + Some(ErrorImpl::error(self.error.inner.as_ref())) } } diff --git a/src/error.rs b/src/error.rs index 8ddb24c..6d5ac98 100644 --- a/src/error.rs +++ b/src/error.rs @@ -199,8 +199,7 @@ impl Report { E: StdError + Send + Sync + 'static, { let inner = ErrorImpl { - vtable, - handler, + header: ErrorHeader { vtable, handler }, _object: error, }; @@ -279,7 +278,7 @@ impl Report { // // As the generic is at the end of the struct and the struct is `repr(C)` this reference // will be within bounds of the original pointer, and the field will have the same offset - let handler = self.inner_mut().handler.take(); + let handler = header_mut(self.inner.as_mut()).handler.take(); let error: ContextError = ContextError { msg, error: self }; let vtable = &ErrorVTable { @@ -297,21 +296,8 @@ impl Report { } /// Access the vtable for the current error object. - const fn vtable(&self) -> &'static ErrorVTable { - vtable(self.inner.ptr) - } - - // Safety: this access a `ErrorImpl` as a valid reference to a `ErrorImpl<()>` - // - // As the generic is at the end of the struct and the struct is `repr(C)` this reference - // will be within bounds of the original pointer, and the field will have the same offset - const fn inner_ref(&self) -> &ErrorImpl<()> { - unsafe { self.inner.as_ref() } - } - - /// See: [Self::inner_ref] - fn inner_mut(&mut self) -> &mut ErrorImpl<()> { - unsafe { self.inner.as_mut() } + fn vtable(&self) -> &'static ErrorVTable { + header(self.inner.as_ref()).vtable } /// An iterator of the chain of source errors contained by this Report. @@ -336,7 +322,7 @@ impl Report { /// } /// ``` pub fn chain(&self) -> Chain<'_> { - ErrorImpl::chain(self.inner.as_ptr()) + ErrorImpl::chain(self.inner.as_ref()) } /// The lowest level cause of this error — this error's cause's @@ -376,7 +362,7 @@ impl Report { unsafe { // Use vtable to find NonNull<()> which points to a value of type E // somewhere inside the data structure. - let addr = match (self.vtable().object_downcast)(self.inner.as_ptr(), target) { + let addr = match (self.vtable().object_downcast)(self.inner.as_ref(), target) { Some(addr) => addr, None => return Err(self), }; @@ -446,7 +432,7 @@ impl Report { unsafe { // Use vtable to find NonNull<()> which points to a value of type E // somewhere inside the data structure. - let addr = (self.vtable().object_downcast)(self.inner.as_ptr(), target)?; + let addr = (self.vtable().object_downcast)(self.inner.as_ref(), target)?; Some(addr.cast::().as_ref()) } } @@ -460,21 +446,23 @@ impl Report { unsafe { // Use vtable to find NonNull<()> which points to a value of type E // somewhere inside the data structure. - let addr = (self.vtable().object_downcast_mut)(self.inner.as_mut_ptr(), target)?; + let addr = (self.vtable().object_downcast_mut)(self.inner.as_mut(), target)?; Some(addr.cast::().as_mut()) } } /// Get a reference to the Handler for this Report. pub fn handler(&self) -> &dyn EyreHandler { - // TODO: find better ways of doing this without creating a reference to ErrorImpl as it - // creates a valid reference to a pointer of a mismatched layout - self.inner_ref().handler.as_ref().unwrap().as_ref() + header(self.inner.as_ref()) + .handler + .as_ref() + .unwrap() + .as_ref() } /// Get a mutable reference to the Handler for this Report. pub fn handler_mut(&mut self) -> &mut dyn EyreHandler { - unsafe { self.inner.as_mut() } + header_mut(self.inner.as_mut()) .handler .as_mut() .unwrap() @@ -484,13 +472,17 @@ impl Report { /// Get a reference to the Handler for this Report. #[doc(hidden)] pub fn context(&self) -> &dyn EyreHandler { - self.inner_ref().handler.as_ref().unwrap().as_ref() + header(self.inner.as_ref()) + .handler + .as_ref() + .unwrap() + .as_ref() } /// Get a mutable reference to the Handler for this Report. #[doc(hidden)] pub fn context_mut(&mut self) -> &mut dyn EyreHandler { - unsafe { self.inner.as_mut() } + header_mut(self.inner.as_mut()) .handler .as_mut() .unwrap() @@ -512,25 +504,25 @@ impl Deref for Report { type Target = dyn StdError + Send + Sync + 'static; fn deref(&self) -> &Self::Target { - ErrorImpl::error(self.inner.as_ptr()) + ErrorImpl::error(self.inner.as_ref()) } } impl DerefMut for Report { fn deref_mut(&mut self) -> &mut Self::Target { - ErrorImpl::error_mut(self.inner.as_mut_ptr()) + ErrorImpl::error_mut(self.inner.as_mut()) } } impl Display for Report { fn fmt(&self, formatter: &mut fmt::Formatter<'_>) -> fmt::Result { - ErrorImpl::display(self.inner.as_ptr(), formatter) + ErrorImpl::display(self.inner.as_ref(), formatter) } } impl Debug for Report { fn fmt(&self, formatter: &mut fmt::Formatter<'_>) -> fmt::Result { - ErrorImpl::debug(self.inner.as_ptr(), formatter) + ErrorImpl::debug(self.inner.as_ref(), formatter) } } @@ -739,7 +731,7 @@ where } else { // Recurse down the context chain per the inner error's vtable. let source = &unerased._object.error; - (source.vtable().object_downcast)(source.inner.as_ptr(), target) + (source.vtable().object_downcast)(source.inner.as_ref(), target) } } @@ -760,7 +752,7 @@ where } else { // Recurse down the context chain per the inner error's vtable. let source = &mut unerased._object.error; - (source.vtable().object_downcast_mut)(source.inner.as_mut_ptr(), target) + (source.vtable().object_downcast_mut)(source.inner.as_mut(), target) } } @@ -787,15 +779,20 @@ where let inner = ptr::read(&unerased.as_ref()._object.error.inner); drop(unerased); // Recursively drop the next error using the same target typeid. - (inner.as_ref().vtable.object_drop_rest)(inner, target); + (header(inner.as_ref()).vtable.object_drop_rest)(inner, target); } } -// repr C to ensure that E remains in the final position. #[repr(C)] -pub(crate) struct ErrorImpl { +pub(crate) struct ErrorHeader { vtable: &'static ErrorVTable, pub(crate) handler: Option>, +} + +// repr C to ensure that E remains in the final position. +#[repr(C)] +pub(crate) struct ErrorImpl { + header: ErrorHeader, // NOTE: Don't use directly. Use only through vtable. Erased type may have // different alignment. _object: E, @@ -819,29 +816,39 @@ impl ErrorImpl { } } -// Reads the vtable out of `p`. This is the same as `p.as_ref().vtable`, but -// avoids converting `p` into a reference. -const fn vtable(p: NonNull>) -> &'static ErrorVTable { - // Safety: `ErrorVTable` is the first field of `ErrorImpl` and `ErrorImpl` is `repr(C)`. - unsafe { *(p.as_ptr() as *const &'static ErrorVTable) } +// Reads the header out of `p`. This is the same as `p.as_ref().header`, but +// avoids converting `p` into a reference of a shrunk provenance with a type different than the +// allocation. +fn header(p: RefPtr<'_, ErrorImpl<()>>) -> &'_ ErrorHeader { + // Safety: `ErrorHeader` is the first field of repr(C) `ErrorImpl` + unsafe { p.cast().as_ref() } +} + +fn header_mut(p: MutPtr<'_, ErrorImpl<()>>) -> &mut ErrorHeader { + // Safety: `ErrorHeader` is the first field of repr(C) `ErrorImpl` + unsafe { p.cast().into_mut() } } impl ErrorImpl<()> { pub(crate) fn error(this: RefPtr<'_, Self>) -> &(dyn StdError + Send + Sync + 'static) { // Use vtable to attach E's native StdError vtable for the right // original type E. - unsafe { (vtable(this.ptr).object_ref)(this) } + unsafe { (header(this).vtable.object_ref)(this) } } pub(crate) fn error_mut(this: MutPtr<'_, Self>) -> &mut (dyn StdError + Send + Sync + 'static) { // Use vtable to attach E's native StdError vtable for the right // original type E. - unsafe { (vtable(this.ptr).object_mut)(this) } + unsafe { (header_mut(this).vtable.object_mut)(this) } } pub(crate) fn chain(this: RefPtr<'_, Self>) -> Chain<'_> { Chain::new(Self::error(this)) } + + pub(crate) fn header(this: RefPtr<'_, ErrorImpl>) -> &ErrorHeader { + header(this) + } } impl StdError for ErrorImpl @@ -879,7 +886,7 @@ impl From for Box { // Report has a Drop impl which we want to not run. // Use vtable to attach ErrorImpl's native StdError vtable for // the right original type E. - (vtable(outer.inner.ptr).object_boxed)(outer.inner) + (header(outer.inner.as_ref()).vtable.object_boxed)(outer.inner) } } } diff --git a/src/fmt.rs b/src/fmt.rs index 5ac1662..c66620e 100644 --- a/src/fmt.rs +++ b/src/fmt.rs @@ -3,7 +3,7 @@ use core::fmt; impl ErrorImpl<()> { pub(crate) fn display(this: RefPtr<'_, Self>, f: &mut fmt::Formatter<'_>) -> fmt::Result { - unsafe { this.as_ref() } + ErrorImpl::header(this) .handler .as_ref() .map(|handler| handler.display(Self::error(this), f)) @@ -12,7 +12,7 @@ impl ErrorImpl<()> { /// Debug formats the error using the captured handler pub(crate) fn debug(this: RefPtr<'_, Self>, f: &mut fmt::Formatter<'_>) -> fmt::Result { - unsafe { this.as_ref() } + ErrorImpl::header(this) .handler .as_ref() .map(|handler| handler.debug(Self::error(this), f)) diff --git a/src/lib.rs b/src/lib.rs index 93c0afe..1494ff5 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -376,7 +376,6 @@ mod wrapper; use crate::backtrace::Backtrace; use crate::error::ErrorImpl; use core::fmt::Display; -use core::mem::ManuallyDrop; use std::error::Error as StdError; diff --git a/src/ptr.rs b/src/ptr.rs index 0646007..dc4f3d8 100644 --- a/src/ptr.rs +++ b/src/ptr.rs @@ -4,7 +4,7 @@ use std::{marker::PhantomData, ptr::NonNull}; /// /// **NOTE**: Does not deallocate when dropped pub(crate) struct OwnedPtr { - pub(crate) ptr: NonNull, + ptr: NonNull, } impl Copy for OwnedPtr {} @@ -48,34 +48,14 @@ impl OwnedPtr { Box::from_raw(self.ptr.as_ptr()) } - /// Returns a shared reference to the owned value - /// - /// # Safety - /// - /// See: [`NonNull::as_ref`] - #[inline] - pub(crate) const unsafe fn as_ref(&self) -> &T { - self.ptr.as_ref() - } - - /// Returns a mutable reference to the owned value - /// - /// # Safety - /// - /// See: [`NonNull::as_mut`] - #[inline] - pub(crate) unsafe fn as_mut(&mut self) -> &mut T { - self.ptr.as_mut() - } - - pub(crate) const fn as_ptr(&self) -> RefPtr<'_, T> { + pub(crate) const fn as_ref(&self) -> RefPtr<'_, T> { RefPtr { ptr: self.ptr, _marker: PhantomData, } } - pub(crate) fn as_mut_ptr(&mut self) -> MutPtr<'_, T> { + pub(crate) fn as_mut(&mut self) -> MutPtr<'_, T> { MutPtr { ptr: self.ptr, _marker: PhantomData, @@ -149,13 +129,6 @@ impl<'a, T: ?Sized> Clone for MutPtr<'a, T> { } impl<'a, T: ?Sized> MutPtr<'a, T> { - pub(crate) fn new(ptr: &'a mut T) -> Self { - Self { - ptr: NonNull::from(ptr), - _marker: PhantomData, - } - } - /// Convert the pointer to another type pub(crate) fn cast(self) -> MutPtr<'a, U> { MutPtr { @@ -164,26 +137,6 @@ impl<'a, T: ?Sized> MutPtr<'a, T> { } } - /// Returns a shared reference to the owned value - /// - /// # Safety - /// - /// See: [`NonNull::as_ref`] - #[inline] - pub(crate) unsafe fn as_ref(&self) -> &'a T { - self.ptr.as_ref() - } - - /// Returns a mutable reference to the owned value - /// - /// # Safety - /// - /// See: [`NonNull::as_mut`] - #[inline] - pub(crate) unsafe fn as_mut(&'a mut self) -> &'a mut T { - self.ptr.as_mut() - } - /// Returns a mutable reference to the owned value with the lifetime decoupled from self /// /// # Safety From 9a7426ecb71e7cc40e60d0ecc0afe096d62f576c Mon Sep 17 00:00:00 2001 From: Tei Roberts Date: Sat, 26 Aug 2023 00:38:16 +0200 Subject: [PATCH 05/11] ci: miri --- .github/workflows/ci.yml | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 77b8743..4225a68 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -6,6 +6,9 @@ on: name: Continuous integration +env: + MIRIFLAGS: -Zmiri-strict-provenance + jobs: check: name: Check @@ -126,3 +129,17 @@ jobs: with: command: clippy args: -- -D warnings + miri: + name: Miri + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v1 + - uses: actions-rs/toolchain@v1 + with: + toolchain: nightly + components: miri + override: true + - uses: actions-rs/cargo@v1 + with: + command: miri + args: test From 524a5a8b2a8032642a823203cf178535f3918816 Mon Sep 17 00:00:00 2001 From: Tei Roberts Date: Tue, 5 Sep 2023 23:43:37 +0200 Subject: [PATCH 06/11] fix: `unsafe_op_in_unsafe_fn` --- src/error.rs | 67 +++++++++++++++++++++++++++++----------------------- src/lib.rs | 1 + src/ptr.rs | 6 ++--- 3 files changed, 41 insertions(+), 33 deletions(-) diff --git a/src/error.rs b/src/error.rs index 6d5ac98..5624a3c 100644 --- a/src/error.rs +++ b/src/error.rs @@ -556,7 +556,7 @@ unsafe fn object_drop(e: OwnedPtr>) { // contained in `Box`, which must not be done. In practice this probably won't make any // difference by now, but technically it's unsound. // see: https://github.com/rust-lang/unsafe-code-guidelines/blob/master/wip/stacked-borrows.md - let unerased = e.cast::>().into_box(); + let unerased = unsafe { e.cast::>().into_box() }; drop(unerased); } @@ -572,7 +572,7 @@ unsafe fn object_drop_front(e: OwnedPtr>, target: TypeId) { // contained in `Box`, which must not be done. In practice this probably won't make any // difference by now, but technically it's unsound. // see: https://github.com/rust-lang/unsafe-code-guidelines/blob/master/wip/stacked-borrows.m - let unerased = e.cast::>().into_box(); + let unerased = unsafe { e.cast::>().into_box() }; mem::forget(unerased._object) } @@ -585,7 +585,7 @@ where E: StdError + Send + Sync + 'static, { // Attach E's native StdError vtable onto a pointer to self._object. - &e.cast::>().as_ref()._object + &unsafe { e.cast::>().as_ref() }._object } /// # Safety @@ -596,7 +596,7 @@ where E: StdError + Send + Sync + 'static, { // Attach E's native StdError vtable onto a pointer to self._object. - &mut e.cast::>().into_mut()._object + &mut unsafe { e.cast::>().into_mut() }._object } /// # Safety @@ -607,7 +607,7 @@ where E: StdError + Send + Sync + 'static, { // Attach ErrorImpl's native StdError vtable. The StdError impl is below. - e.cast::>().into_box() + unsafe { e.cast::>().into_box() } } /// # Safety @@ -620,7 +620,7 @@ where if TypeId::of::() == target { // Caller is looking for an E pointer and e is ErrorImpl, take a // pointer to its E field. - let unerased = e.cast::>().as_ref(); + let unerased = unsafe { e.cast::>().as_ref() }; Some(NonNull::from(&(unerased._object)).cast::<()>()) } else { None @@ -640,7 +640,7 @@ where if TypeId::of::() == target { // Caller is looking for an E pointer and e is ErrorImpl, take a // pointer to its E field. - let unerased = e.cast::>().into_mut(); + let unerased = unsafe { e.cast::>().into_mut() }; Some(NonNull::from(&mut (unerased._object)).cast::<()>()) } else { None @@ -659,11 +659,11 @@ where E: 'static, { if TypeId::of::() == target { - let unerased = e.cast::>>().as_ref(); + let unerased = unsafe { e.cast::>>().as_ref() }; let addr = NonNull::from(&unerased._object.msg).cast::<()>(); Some(addr) } else if TypeId::of::() == target { - let unerased = e.cast::>>().as_ref(); + let unerased = unsafe { e.cast::>>().as_ref() }; let addr = NonNull::from(&unerased._object.error).cast::<()>(); Some(addr) } else { @@ -683,11 +683,11 @@ where E: 'static, { if TypeId::of::() == target { - let unerased = e.cast::>>().into_mut(); + let unerased = unsafe { e.cast::>>().into_mut() }; let addr = NonNull::from(&unerased._object.msg).cast::<()>(); Some(addr) } else if TypeId::of::() == target { - let unerased = e.cast::>>().into_mut(); + let unerased = unsafe { e.cast::>>().into_mut() }; let addr = NonNull::from(&mut unerased._object.error).cast::<()>(); Some(addr) } else { @@ -705,12 +705,16 @@ where // Called after downcasting by value to either the D or the E and doing a // ptr::read to take ownership of that value. if TypeId::of::() == target { - e.cast::, E>>>() - .into_box(); + unsafe { + e.cast::, E>>>() + .into_box() + }; } else { debug_assert_eq!(TypeId::of::(), target); - e.cast::>>>() - .into_box(); + unsafe { + e.cast::>>>() + .into_box() + }; } } @@ -724,14 +728,14 @@ unsafe fn context_chain_downcast( where D: 'static, { - let unerased = e.cast::>>().as_ref(); + let unerased = unsafe { e.cast::>>().as_ref() }; if TypeId::of::() == target { let addr = NonNull::from(&unerased._object.msg).cast::<()>(); Some(addr) } else { // Recurse down the context chain per the inner error's vtable. let source = &unerased._object.error; - (source.vtable().object_downcast)(source.inner.as_ref(), target) + unsafe { (source.vtable().object_downcast)(source.inner.as_ref(), target) } } } @@ -745,14 +749,14 @@ unsafe fn context_chain_downcast_mut( where D: 'static, { - let unerased = e.cast::>>().into_mut(); + let unerased = unsafe { e.cast::>>().into_mut() }; if TypeId::of::() == target { let addr = NonNull::from(&unerased._object.msg).cast::<()>(); Some(addr) } else { // Recurse down the context chain per the inner error's vtable. let source = &mut unerased._object.error; - (source.vtable().object_downcast_mut)(source.inner.as_mut(), target) + unsafe { (source.vtable().object_downcast_mut)(source.inner.as_mut(), target) } } } @@ -766,20 +770,23 @@ where // Called after downcasting by value to either the D or one of the causes // and doing a ptr::read to take ownership of that value. if TypeId::of::() == target { - let unerased = e - .cast::, Report>>>() - .into_box(); + let unerased = unsafe { + e.cast::, Report>>>() + .into_box() + }; // Drop the entire rest of the data structure rooted in the next Report. drop(unerased); } else { - let unerased = e - .cast::>>>() - .into_box(); - // Read out a ManuallyDrop>> from the next error. - let inner = ptr::read(&unerased.as_ref()._object.error.inner); - drop(unerased); - // Recursively drop the next error using the same target typeid. - (header(inner.as_ref()).vtable.object_drop_rest)(inner, target); + unsafe { + let unerased = e + .cast::>>>() + .into_box(); + // Read out a ManuallyDrop>> from the next error. + let inner = ptr::read(&unerased.as_ref()._object.error.inner); + drop(unerased); + // Recursively drop the next error using the same target typeid. + (header(inner.as_ref()).vtable.object_drop_rest)(inner, target); + } } } diff --git a/src/lib.rs b/src/lib.rs index 1494ff5..fa5d654 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -334,6 +334,7 @@ missing_docs, // FIXME: this lint is currently nightly only rustdoc::missing_doc_code_examples, + unsafe_op_in_unsafe_fn, rust_2018_idioms, unreachable_pub, bad_style, diff --git a/src/ptr.rs b/src/ptr.rs index dc4f3d8..3d118e5 100644 --- a/src/ptr.rs +++ b/src/ptr.rs @@ -45,7 +45,7 @@ impl OwnedPtr { /// /// A cast pointer must therefore be cast back to the original type before calling this method. pub(crate) unsafe fn into_box(self) -> Box { - Box::from_raw(self.ptr.as_ptr()) + unsafe { Box::from_raw(self.ptr.as_ptr()) } } pub(crate) const fn as_ref(&self) -> RefPtr<'_, T> { @@ -105,7 +105,7 @@ impl<'a, T: ?Sized> RefPtr<'a, T> { /// See: [`NonNull::as_ref`] #[inline] pub(crate) unsafe fn as_ref(&self) -> &'a T { - self.ptr.as_ref() + unsafe { self.ptr.as_ref() } } } @@ -144,6 +144,6 @@ impl<'a, T: ?Sized> MutPtr<'a, T> { /// See: [`NonNull::as_mut`] #[inline] pub(crate) unsafe fn into_mut(mut self) -> &'a mut T { - self.ptr.as_mut() + unsafe { self.ptr.as_mut() } } } From 078a3c5a7fec151ddf95877f8664633da9f36419 Mon Sep 17 00:00:00 2001 From: Tei Roberts Date: Tue, 5 Sep 2023 23:47:18 +0200 Subject: [PATCH 07/11] chore!: bump MSRV --- .github/workflows/ci.yml | 2 +- Cargo.toml | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 4225a68..79912a2 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -69,7 +69,7 @@ jobs: - uses: actions/checkout@v1 - uses: actions-rs/toolchain@v1 with: - toolchain: 1.42 + toolchain: 1.60 override: true - uses: actions-rs/cargo@v1 with: diff --git a/Cargo.toml b/Cargo.toml index ddddab5..8cdfcc3 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -27,7 +27,7 @@ pyo3 = { version = "0.13", default-features = false, features = ["auto-initializ [dependencies] indenter = "0.3.0" -once_cell = "1.4.0" +once_cell = "1.18.0" pyo3 = { version = "0.13", optional = true, default-features = false } [package.metadata.docs.rs] From 81e88f5703a4a4ff536d20508479eb35a915394e Mon Sep 17 00:00:00 2001 From: Tei Roberts Date: Tue, 5 Sep 2023 23:56:06 +0200 Subject: [PATCH 08/11] chore: address PR comments --- src/error.rs | 18 +++++------------- tests/test_location.rs | 2 +- 2 files changed, 6 insertions(+), 14 deletions(-) diff --git a/src/error.rs b/src/error.rs index 5624a3c..701a0fd 100644 --- a/src/error.rs +++ b/src/error.rs @@ -203,13 +203,10 @@ impl Report { _object: error, }; - // Erase the concrete type of E from the compile-time type system. This - // is equivalent to the safe unsize coersion from Box> to - // Box> except that the - // result is a thin pointer. The necessary behavior for manipulating the - // underlying ErrorImpl is preserved in the vtable provided by the - // caller rather than a builtin fat pointer vtable. - // let erased = mem::transmute::>, Box>>(inner); + // Construct a new owned allocation through a raw pointer + // + // This does not keep the allocation around as a `Box` which would invalidate an + // references when moved let ptr = OwnedPtr::>::new(inner); // Safety: the type @@ -550,12 +547,7 @@ struct ErrorVTable { /// /// Requires layout of *e to match ErrorImpl. unsafe fn object_drop(e: OwnedPtr>) { - // Cast back to ErrorImpl so that the allocator receives the correct - // Layout to deallocate the Box's memory. - // Note: This must not use `mem::transmute` because it tries to reborrow the `Unique` - // contained in `Box`, which must not be done. In practice this probably won't make any - // difference by now, but technically it's unsound. - // see: https://github.com/rust-lang/unsafe-code-guidelines/blob/master/wip/stacked-borrows.md + // Cast to a context type and drop the Box allocation. let unerased = unsafe { e.cast::>().into_box() }; drop(unerased); } diff --git a/tests/test_location.rs b/tests/test_location.rs index 58b03b1..58737fe 100644 --- a/tests/test_location.rs +++ b/tests/test_location.rs @@ -60,7 +60,7 @@ fn read_path(path: &str) -> Result { } #[cfg(miri)] -fn read_path(path: &str) -> Result { +fn read_path(_path: &str) -> Result { // Miri doesn't support reading files, so we just return an error Err(std::io::Error::new( std::io::ErrorKind::Other, From f04d9e900a0671b83e2b4fb184b5e0284e327d15 Mon Sep 17 00:00:00 2001 From: Tei Roberts Date: Sat, 16 Sep 2023 20:00:12 +0200 Subject: [PATCH 09/11] fix: ci workflow names --- .github/workflows/ci.yml | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 79912a2..c4f9ea7 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -28,7 +28,7 @@ jobs: command: check test-matrix: - name: Test Suite + name: Test Suite Matrix runs-on: ubuntu-latest strategy: matrix: @@ -55,7 +55,7 @@ jobs: args: ${{ matrix.features }} test-msrv: - name: Test Suite + name: Test MSRV runs-on: ubuntu-latest strategy: matrix: @@ -76,7 +76,7 @@ jobs: command: test test-os: - name: Test Suite + name: Test Platform Matrix runs-on: ${{ matrix.os }} strategy: matrix: From 3f529f4a2614edd9e1eef18d8478835645e43196 Mon Sep 17 00:00:00 2001 From: Tei Roberts Date: Sat, 16 Sep 2023 20:05:50 +0200 Subject: [PATCH 10/11] chore: raise msrv to 1.65 (addr2line) --- .github/workflows/ci.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index c4f9ea7..64be914 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -69,7 +69,7 @@ jobs: - uses: actions/checkout@v1 - uses: actions-rs/toolchain@v1 with: - toolchain: 1.60 + toolchain: 1.65 override: true - uses: actions-rs/cargo@v1 with: From b5dd86ac29d43a53a08b3e31a49650118fc79b52 Mon Sep 17 00:00:00 2001 From: Tei Roberts Date: Sat, 16 Sep 2023 20:20:11 +0200 Subject: [PATCH 11/11] chore: revert distinctive CI names due to branch protection rules The new names, such as `Test Platform Matrix` which do make it easier to see which jobs failed, rather than msrv, test, and miri all being called `Test Suite`, the in-place branch protection rules wait forever until the now non-existent `Test Suite` passes --- .github/workflows/ci.yml | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 64be914..1034a0a 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -28,7 +28,7 @@ jobs: command: check test-matrix: - name: Test Suite Matrix + name: Test Suite runs-on: ubuntu-latest strategy: matrix: @@ -55,7 +55,7 @@ jobs: args: ${{ matrix.features }} test-msrv: - name: Test MSRV + name: Test Suite runs-on: ubuntu-latest strategy: matrix: @@ -76,7 +76,7 @@ jobs: command: test test-os: - name: Test Platform Matrix + name: Test Suite runs-on: ${{ matrix.os }} strategy: matrix: