From a263e44339d3cce066a045cae58d4abb9f429e1d Mon Sep 17 00:00:00 2001 From: = Date: Sun, 10 Mar 2024 21:24:36 +0100 Subject: [PATCH 1/5] Revert 2 commits: * "Multiple soundness fixes for erased futures" * "NoFuture: use a RawFuture trait rather than implementing Drop and Future" This reverts commits fbd2f0588b4afa4f0c91b0aee43006118a8f65be and 6900cf7618809d158f9cfc7b2b7027aef3705c10. --- src/box_scope.rs | 43 +++++------------- src/nofuture.rs | 113 ++++++++++++----------------------------------- src/raw_scope.rs | 31 +++++-------- 3 files changed, 49 insertions(+), 138 deletions(-) diff --git a/src/box_scope.rs b/src/box_scope.rs index a5eefd8..01216d2 100644 --- a/src/box_scope.rs +++ b/src/box_scope.rs @@ -1,10 +1,6 @@ -use std::{alloc::Layout, cell::UnsafeCell, future::Future, ptr::NonNull}; +use std::future::Future; -use crate::{ - nofuture::{NoFuture, RawFuture}, - raw_scope::RawScope, - Family, Never, TopScope, -}; +use crate::{nofuture::NoFuture, raw_scope::RawScope, Family, Never, TopScope}; /// A dynamic scope tied to a Box. /// @@ -14,30 +10,24 @@ use crate::{ pub struct BoxScope(std::ptr::NonNull>) where T: for<'a> Family<'a>, - F: RawFuture; + F: Future; impl Drop for BoxScope where T: for<'a> Family<'a>, - F: RawFuture, + F: Future, { fn drop(&mut self) { // SAFETY: created from a Box in the constructor, so dereference-able. - let this = self.0.as_ptr(); + let this = unsafe { self.0.as_ref() }; // SAFETY: we MUST release the future before calling drop on the `Box` otherwise we'll call its // destructor after releasing its backing memory, causing uaf { - let fut = unsafe { std::ptr::addr_of!((*this).active_fut) }; - let fut = UnsafeCell::raw_get(fut); - let fut: *mut F = fut.cast(); - let fut = NonNull::new(fut).unwrap(); + let fut = unsafe { this.active_fut.get().as_mut() }.unwrap(); // SAFETY: a call to `RawScope::open` happened - unsafe { - RawFuture::drop_future(fut); - let this = self.0.as_ptr(); - RawFuture::dealloc_outer(fut, this); - } + unsafe { fut.assume_init_drop() }; } + unsafe { drop(Box::from_raw(self.0.as_ptr())) } } } @@ -59,16 +49,11 @@ where where S::Future: 'static, { - let outer_layout = Layout::new::>>(); - let raw_scope = NonNull::new( - unsafe { std::alloc::alloc(outer_layout) } as *mut RawScope> - ) - .unwrap(); - - unsafe { raw_scope.as_ptr().write(RawScope::new()) }; + let raw_scope = Box::new(RawScope::new()); + let raw_scope = Box::leak(raw_scope).into(); // SAFETY: `self.0` is dereference-able due to coming from a `Box`. - unsafe { RawScope::open_erased(raw_scope, outer_layout, scope) } + unsafe { RawScope::open_erased(raw_scope, scope) } // SAFETY: open was called as part of `BoxScope::new` let erased_raw_scope = unsafe { RawScope::erase(raw_scope) }; @@ -101,13 +86,7 @@ where BoxScope(raw_scope) } -} -impl BoxScope -where - T: for<'a> Family<'a>, - F: RawFuture, -{ /// Enters the scope, making it possible to access the data frozen inside of the scope. /// /// # Panics diff --git a/src/nofuture.rs b/src/nofuture.rs index a73eb38..c76e6eb 100644 --- a/src/nofuture.rs +++ b/src/nofuture.rs @@ -1,7 +1,6 @@ //! Declares `NoFuture`, a handrolled type-erased `Future` that uses similar tricks as `anyhow::Error`. use std::{ - alloc::Layout, future::Future, mem::ManuallyDrop, ptr::NonNull, @@ -13,39 +12,26 @@ use crate::Never; // SAFETY: repr C to ensure that field layout stays as declared. #[repr(C)] pub struct NoFuture { - // SAFETY: - // - must be the first item of the struct so we can read a pointer to the struct as a pointer to the vtable. - metadata: Metadata, - - marker: std::marker::PhantomPinned, + vtable: &'static FutureVTable, // SAFETY: // - last item of the struct so that vtable doesn't move when cast // - Manually dropped to make sure we don't drop the _object twice if the outer struct is dropped before being erased. _object: ManuallyDrop, } -#[derive(Clone, Copy)] -struct Metadata { - vtable: &'static FutureVTable, - outer_layout: Layout, -} - struct FutureVTable { object_drop: unsafe fn(NonNull), future_poll: for<'a, 'b> unsafe fn(NonNull, &'a mut Context<'b>) -> Poll, } -impl NoFuture -where - F: Future, -{ +impl NoFuture { /// Erase the future. /// /// This boils down to a pointer cast, that is always safe to do. /// /// For the result of the cast to actually be soundly dereferenceable, however, the usual /// conditions apply - pub fn erase(this: NonNull>) -> NonNull> { + pub fn erase(this: NonNull>) -> NonNull> { this.cast() } } @@ -61,17 +47,13 @@ where { /// Builds a not-yet erased but eraseable future from a future. // SAFETY: the only way to build a NoFuture is to start from a future. - pub fn new(future: F, outer_layout: Layout) -> NoFuture { + pub fn new(future: F) -> NoFuture { let vtable = &FutureVTable { object_drop: object_drop::, future_poll: future_poll::, }; NoFuture { - metadata: Metadata { - vtable, - outer_layout, - }, - marker: std::marker::PhantomPinned, + vtable, _object: ManuallyDrop::new(future), } } @@ -81,20 +63,31 @@ where } } -// Reads the vtable out of `p`. This is the same as `p.as_ref().vtable`, but -// avoids converting `p` into a reference. -unsafe fn vtable(p: NonNull>) -> &'static FutureVTable { - // NOTE: This assumes that `FutureVTable` is the first field of NoFuture. - let metadata: Metadata = unsafe { *(p.as_ptr() as *const Metadata) }; - metadata.vtable +impl Future for NoFuture { + type Output = Never; + + fn poll(self: std::pin::Pin<&mut Self>, cx: &mut Context<'_>) -> Poll { + let future_poll = self.vtable.future_poll; + let this = NonNull::from(self.get_mut()); + unsafe { future_poll(this, cx) } + } } -// Reads the vtable out of `p`. This is the same as `p.as_ref().vtable`, but -// avoids converting `p` into a reference. -unsafe fn outer_layout(p: NonNull>) -> Layout { - // NOTE: This assumes that `FutureVTable` is the first field of NoFuture. - let metadata: Metadata = unsafe { *(p.as_ptr() as *const Metadata) }; - metadata.outer_layout +// We really would prefer to implement drop only on NoFuture, but Rust won't let us "specialize" Drop. +impl Drop for NoFuture { + fn drop(&mut self) { + let object_drop = self.vtable.object_drop; + + let this = NonNull::from(self); + + let definitely_erased = NoFuture::erase(this); + + // SAFETY: + // 1. Ensured by the module don't letting you building erased futures from scratch. + // 2. Only called in Drop implementation, then the object is forgotten. + // The future was marked as `ManuallyDrop` in the case `ErasedOrF == F`. + unsafe { object_drop(definitely_erased) }; + } } /// # Safety @@ -131,53 +124,3 @@ unsafe fn future_poll + 'static>( } pub struct Erased; - -pub trait RawFuture { - type Output; - - unsafe fn poll(this: NonNull, cx: &mut Context<'_>) -> Poll; - - unsafe fn drop_future(this: NonNull); - - unsafe fn dealloc_outer(this: NonNull, outer: *mut T); -} - -impl RawFuture for NoFuture { - type Output = Never; - - unsafe fn poll(this: NonNull, cx: &mut Context<'_>) -> Poll { - let vtable = vtable(this); - (vtable.future_poll)(this, cx) - } - - unsafe fn drop_future(this: NonNull) { - let vtable = vtable(this); - (vtable.object_drop)(this) - } - - unsafe fn dealloc_outer(this: NonNull, outer: *mut T) { - let outer_layout = outer_layout(this); - std::alloc::dealloc(outer as *mut _, outer_layout) - } -} - -impl RawFuture for F -where - F: Future, -{ - type Output = F::Output; - - unsafe fn poll(mut this: NonNull, cx: &mut Context<'_>) -> Poll { - let this = this.as_mut(); - let pinned = std::pin::Pin::new_unchecked(this); - pinned.poll(cx) - } - - unsafe fn drop_future(this: NonNull) { - std::ptr::drop_in_place(this.as_ptr()) - } - - unsafe fn dealloc_outer(_this: NonNull, outer: *mut T) { - drop(Box::from_raw(outer)) - } -} diff --git a/src/raw_scope.rs b/src/raw_scope.rs index 359cc10..84dcb8c 100644 --- a/src/raw_scope.rs +++ b/src/raw_scope.rs @@ -1,13 +1,10 @@ -use crate::{ - nofuture::{NoFuture, RawFuture}, - waker, Family, Never, TopScope, -}; +use crate::{nofuture::NoFuture, waker, Family, Never, TopScope}; use std::{ - alloc::Layout, cell::{Cell, UnsafeCell}, future::Future, marker::PhantomData, mem::MaybeUninit, + pin::Pin, ptr::NonNull, task::Poll, }; @@ -149,13 +146,7 @@ where let fut = unsafe { scope.run(time_capsule) }; active_fut.write(fut); } -} -impl RawScope -where - T: for<'a> Family<'a>, - F: RawFuture, -{ /// # Safety /// /// 1. The `this` parameter is *dereference-able*. @@ -167,21 +158,20 @@ where G: for<'a> FnOnce(&'a mut >::Family) -> Output, { // SAFETY: `this` is dereference-able as per precondition (1) - // SAFETY: RawScope is !Sync + the reference is released by the end of the function. - let raw_this = this.as_ptr(); - - let fut: *const UnsafeCell> = std::ptr::addr_of!((*raw_this).active_fut); - let fut: *mut F = UnsafeCell::raw_get(fut).cast(); + let this = unsafe { this.as_ref() }; + // SAFETY: RawScope is !Sync + the reference is released by the end of the function. + let fut = this.active_fut.get().as_mut().unwrap(); // SAFETY: per precondition (2) - let fut = NonNull::new_unchecked(fut); + let fut = fut.assume_init_mut(); + // SAFETY: per precondition (3) + let fut = unsafe { Pin::new_unchecked(fut) }; // SAFETY: we didn't do anything particular here before calling `poll`, which may panic, so // we have nothing to handle. - match RawFuture::poll(fut, &mut std::task::Context::from_waker(&waker::create())) { + match fut.poll(&mut std::task::Context::from_waker(&waker::create())) { Poll::Ready(_) => unreachable!(), Poll::Pending => {} } - let this = unsafe { this.as_ref() }; let state = this.state.0.get(); // SAFETY: cast the lifetime of the Family to `'borrow`. // This is safe to do @@ -223,7 +213,6 @@ where { pub(crate) unsafe fn open_erased>( this: NonNull, - outer_layout: Layout, scope: S, ) { // SAFETY: `this` is dereference-able as per precondition (1) @@ -237,7 +226,7 @@ where let time_capsule = TimeCapsule { state }; // SAFETY: called run from the executor let fut = unsafe { scope.run(time_capsule) }; - active_fut.write(NoFuture::new(fut, outer_layout)); + active_fut.write(NoFuture::new(fut)); } } From 2a27286b29d68b75b7745a9b09a00a96b8e8c122 Mon Sep 17 00:00:00 2001 From: = Date: Sun, 10 Mar 2024 19:32:08 +0100 Subject: [PATCH 2/5] Change implementation to support (and use) dyn Future --- src/box_scope.rs | 86 +++++++++++++------- src/lib.rs | 1 - src/nofuture.rs | 126 ----------------------------- src/raw_scope.rs | 203 ++++++++++++++++++----------------------------- 4 files changed, 135 insertions(+), 281 deletions(-) delete mode 100644 src/nofuture.rs diff --git a/src/box_scope.rs b/src/box_scope.rs index 01216d2..8ff2df6 100644 --- a/src/box_scope.rs +++ b/src/box_scope.rs @@ -1,37 +1,44 @@ -use std::future::Future; +use std::{ + future::Future, + mem::{self, MaybeUninit}, + ptr::NonNull, +}; -use crate::{nofuture::NoFuture, raw_scope::RawScope, Family, Never, TopScope}; +use crate::{raw_scope::RawScope, Family, Never, TopScope}; /// A dynamic scope tied to a Box. /// /// This kind of scopes uses a dynamic allocation. /// In exchange, it is fully `'static` and can be moved after creation. #[repr(transparent)] -pub struct BoxScope(std::ptr::NonNull>) +pub struct BoxScope + 'static>( + std::ptr::NonNull>, +) where T: for<'a> Family<'a>, F: Future; -impl Drop for BoxScope +impl Drop for BoxScope where T: for<'a> Family<'a>, F: Future, { fn drop(&mut self) { - // SAFETY: created from a Box in the constructor, so dereference-able. - let this = unsafe { self.0.as_ref() }; - // SAFETY: we MUST release the future before calling drop on the `Box` otherwise we'll call its - // destructor after releasing its backing memory, causing uaf - { - let fut = unsafe { this.active_fut.get().as_mut() }.unwrap(); - // SAFETY: a call to `RawScope::open` happened - unsafe { fut.assume_init_drop() }; - } - unsafe { drop(Box::from_raw(self.0.as_ptr())) } + // SAFETY: this `Box::from_raw` pairs with a `Box::into_raw` + // in the `new_typed` constructor. The type `F` is not the same, + // but `MaybeUninit` and `F` are repr(transparent)-compatible + // and RawScope is repr(C), so the Box frees the same memory. + // Furthermore, the `new_typed` constructor ensured that F is properly + // initialized so it may be dropped. + // + // Finally, the drop order of implicitly first dropping self.0.state + // and THEN self.0.active_fut goes a bit against the typical self-referencing + // structs assumptions, however self.0.state is a pointer and has no drop glue. + drop(unsafe { Box::from_raw(self.0.as_ptr()) }) } } -impl BoxScope +impl BoxScope where T: for<'a> Family<'a>, { @@ -45,19 +52,12 @@ where /// # Panics /// /// - If `scope` panics. - pub fn new_erased>(scope: S) -> BoxScope + pub fn new_erased>(scope: S) -> Self where S::Future: 'static, { - let raw_scope = Box::new(RawScope::new()); - let raw_scope = Box::leak(raw_scope).into(); - - // SAFETY: `self.0` is dereference-able due to coming from a `Box`. - unsafe { RawScope::open_erased(raw_scope, scope) } - - // SAFETY: open was called as part of `BoxScope::new` - let erased_raw_scope = unsafe { RawScope::erase(raw_scope) }; - BoxScope(erased_raw_scope) + let this = mem::ManuallyDrop::new(BoxScope::new_typed(scope)); + Self(this.0) } } @@ -78,15 +78,41 @@ where where S: TopScope, { - let raw_scope = Box::new(RawScope::new()); - let raw_scope = Box::leak(raw_scope).into(); + let raw_scope = Box::new(RawScope::::new_uninit()); + let raw_scope: *mut RawScope> = Box::into_raw(raw_scope); + struct Guard { + raw_scope: *mut Sc, + } + // guard ensures Box is freed on panic (i.e. if scope.run panics) + let panic_guard = Guard { raw_scope }; + impl Drop for Guard { + fn drop(&mut self) { + // SAFETY: defuse below makes sure this only happens on panic, + // in this case, self.raw_scope is still in the same uninitialized state + // and not otherwise being cleaned up, so this `Box::from_raw` pairs with + // `Box::into_raw` above + drop(unsafe { Box::from_raw(self.raw_scope) }) + } + } - // SAFETY: `self.0` is dereference-able due to coming from a `Box`. - unsafe { RawScope::open(raw_scope, scope) } + let raw_scope: *mut RawScope = raw_scope.cast(); - BoxScope(raw_scope) + unsafe { + RawScope::open(raw_scope, scope); + } + + mem::forget(panic_guard); // defuse guard + // (guard field has no drop glue, so this does not leak anything, it just skips the above `Drop` impl) + + BoxScope(unsafe { NonNull::new_unchecked(raw_scope) }) } +} +impl BoxScope +where + T: for<'a> Family<'a>, + F: Future, +{ /// Enters the scope, making it possible to access the data frozen inside of the scope. /// /// # Panics diff --git a/src/lib.rs b/src/lib.rs index 08a21b2..09f3ae2 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -11,7 +11,6 @@ mod box_scope; #[cfg(not(miri))] pub mod counterexamples; -mod nofuture; mod raw_scope; pub mod scope; #[doc(hidden)] diff --git a/src/nofuture.rs b/src/nofuture.rs deleted file mode 100644 index c76e6eb..0000000 --- a/src/nofuture.rs +++ /dev/null @@ -1,126 +0,0 @@ -//! Declares `NoFuture`, a handrolled type-erased `Future` that uses similar tricks as `anyhow::Error`. - -use std::{ - future::Future, - mem::ManuallyDrop, - ptr::NonNull, - task::{Context, Poll}, -}; - -use crate::Never; - -// SAFETY: repr C to ensure that field layout stays as declared. -#[repr(C)] -pub struct NoFuture { - vtable: &'static FutureVTable, - // SAFETY: - // - last item of the struct so that vtable doesn't move when cast - // - Manually dropped to make sure we don't drop the _object twice if the outer struct is dropped before being erased. - _object: ManuallyDrop, -} - -struct FutureVTable { - object_drop: unsafe fn(NonNull), - future_poll: for<'a, 'b> unsafe fn(NonNull, &'a mut Context<'b>) -> Poll, -} - -impl NoFuture { - /// Erase the future. - /// - /// This boils down to a pointer cast, that is always safe to do. - /// - /// For the result of the cast to actually be soundly dereferenceable, however, the usual - /// conditions apply - pub fn erase(this: NonNull>) -> NonNull> { - this.cast() - } -} - -impl NoFuture -where - // SAFETY: the lifetime must be static otherwise it is going to be erased. - // Alternatively, we could put a lifetime parameter 'a on NoFuture and save the lifetime by - // requesting F: 'a. - // This doesn't seem super useful because most future erasures are necessary in contexts where there are - // no lifetimes. - F: Future + 'static, -{ - /// Builds a not-yet erased but eraseable future from a future. - // SAFETY: the only way to build a NoFuture is to start from a future. - pub fn new(future: F) -> NoFuture { - let vtable = &FutureVTable { - object_drop: object_drop::, - future_poll: future_poll::, - }; - NoFuture { - vtable, - _object: ManuallyDrop::new(future), - } - } - - fn unerase(this: NonNull) -> NonNull> { - this.cast() - } -} - -impl Future for NoFuture { - type Output = Never; - - fn poll(self: std::pin::Pin<&mut Self>, cx: &mut Context<'_>) -> Poll { - let future_poll = self.vtable.future_poll; - let this = NonNull::from(self.get_mut()); - unsafe { future_poll(this, cx) } - } -} - -// We really would prefer to implement drop only on NoFuture, but Rust won't let us "specialize" Drop. -impl Drop for NoFuture { - fn drop(&mut self) { - let object_drop = self.vtable.object_drop; - - let this = NonNull::from(self); - - let definitely_erased = NoFuture::erase(this); - - // SAFETY: - // 1. Ensured by the module don't letting you building erased futures from scratch. - // 2. Only called in Drop implementation, then the object is forgotten. - // The future was marked as `ManuallyDrop` in the case `ErasedOrF == F`. - unsafe { object_drop(definitely_erased) }; - } -} - -/// # Safety -/// -/// 1. f was obtained by calling `NoFuture::::erase(g)` -/// 2. as this function is dropping the object, it should not be called twice on the same object -unsafe fn object_drop + 'static>(f: NonNull) { - // Cast back to NoFuture so that the allocator receives the correct - // Layout to deallocate the Box's memory. - let mut f = NoFuture::::unerase(f); - // SAFETY: per precondition (1) - let f = unsafe { f.as_mut() }; - - // SAFETY: per preconditions - ManuallyDrop::drop(&mut f._object) -} - -/// # Safety -/// -/// 1. f was obtained by calling `NoFuture::::erase(g)` -/// 2. f verifies the same guarantees as if it was pinned -unsafe fn future_poll + 'static>( - f: NonNull, - cx: &mut Context, -) -> Poll { - let mut f = NoFuture::::unerase(f); - // SAFETY: per precondition (1) - let f = unsafe { f.as_mut() }; - - let f = &mut *f._object; - // SAFETY: per precondition (2) - let f = std::pin::Pin::new_unchecked(f); - f.poll(cx) -} - -pub struct Erased; diff --git a/src/raw_scope.rs b/src/raw_scope.rs index 84dcb8c..7eef211 100644 --- a/src/raw_scope.rs +++ b/src/raw_scope.rs @@ -1,11 +1,10 @@ -use crate::{nofuture::NoFuture, waker, Family, Never, TopScope}; +use crate::{waker, Family, Never, TopScope}; use std::{ - cell::{Cell, UnsafeCell}, future::Future, marker::PhantomData, mem::MaybeUninit, pin::Pin, - ptr::NonNull, + ptr::{addr_of_mut, NonNull}, task::Poll, }; @@ -15,8 +14,14 @@ where T: for<'c> Family<'c>, 'b: 'a, { - mut_ref: Option<&'a mut >::Family>, - state: *const State, + // Using a pointer here helps ensure that while RawScope is dropped, + // dropping of F can't assert unique access to the .state field by + // operations that "touch" the FrozenFuture such moving it or passing it to a function. + // (This probably wasn't exploitable with the scope! macro, but it still seems + // more correct this way.) + mut_ref: State, + state: *mut State, + marker: PhantomData<&'a mut >::Family>, } /// Passed to the closures of a scope so that they can freeze the scope. @@ -24,7 +29,7 @@ pub struct TimeCapsule where T: for<'a> Family<'a>, { - state: *const State, + pub(crate) state: *mut State, } impl Clone for TimeCapsule @@ -56,8 +61,9 @@ where 'b: 'a, { FrozenFuture { - mut_ref: Some(t), + mut_ref: Some(NonNull::from(t).cast()), state: self.state, + marker: PhantomData, } } @@ -76,45 +82,54 @@ where } } -struct State(Cell<*mut >::Family>) +// This type is a pointer-type and lifetime-erased equivalent of +// Option<&'a mut >::Family>. +// +// NonNull differs in variance, which would typically be corrected +// with a `PhantomData` marker, however a projection like +// `>::Family>` has T invariant already anyway. +pub(crate) type State = Option>::Family>>; + +/// Underlying representation of a scope. +// SAFETY: repr C to ensure conversion between RawScope> and RawScope +// does not rely on unstable memory layout. +#[repr(C)] +pub(crate) struct RawScope where - T: for<'a> Family<'a>; + T: for<'a> Family<'a>, +{ + state: State, + active_fut: F, +} -impl Default for State +impl RawScope where T: for<'a> Family<'a>, { - fn default() -> Self { - Self(Cell::new(std::ptr::null_mut())) + /// Creates a new closed scope. + pub fn new_uninit() -> RawScope> { + RawScope { + state: None, + active_fut: MaybeUninit::uninit(), + } } } -/// Underlying representation of a scope. -// SAFETY: repr C to ensure that the layout of the struct stays as declared. -#[repr(C)] -pub(crate) struct RawScope +struct RawScopeFields where T: for<'a> Family<'a>, { - phantom: PhantomData<*const fn(TimeCapsule) -> F>, - state: State, - // SAFETY: - // 1. must be the last item of the struct so that the state is still accessible after casting - // 2. unsafe cell allows in place modifications, and is repr[transparent] - // 3. maybeuninit allows the future to be setup only once the outer struct has been pinned, and is transparent. - pub(crate) active_fut: UnsafeCell>, + state: *mut State, + active_fut: *mut F, } - -impl RawScope +impl RawScope where T: for<'a> Family<'a>, { - /// Creates a new closed scope. - pub fn new() -> Self { - Self { - active_fut: UnsafeCell::new(MaybeUninit::uninit()), - phantom: PhantomData, - state: Default::default(), + unsafe fn fields(this: *mut Self) -> RawScopeFields { + RawScopeFields { + state: unsafe { addr_of_mut!((*this).state) }, + active_fut: unsafe { addr_of_mut!((*this).active_fut) }, } } } @@ -126,107 +141,51 @@ where { /// # Safety /// - /// 1. The `this` parameter is *dereference-able*. - /// - /// # Warning - /// - /// Calling this function multiple time will cause the previous future to be dropped. - #[allow(unused_unsafe)] - pub(crate) unsafe fn open>(this: NonNull, scope: S) { - // SAFETY: `this` is dereference-able as per precondition (1) - let this = unsafe { this.as_ref() }; - // SAFETY: the mut reference is exclusive because: - // - the scope is !Sync - // - it is released by the end of the function - let active_fut = unsafe { this.active_fut.get().as_mut() }.unwrap(); + /// TODO docs + pub(crate) unsafe fn open>(this: *mut Self, scope: S) + where + T: for<'a> Family<'a>, + F: Future, + S: TopScope, + { + let RawScopeFields { state, active_fut } = Self::fields(this); - let state: *const State = &this.state; let time_capsule = TimeCapsule { state }; - // SAFETY: called run from the executor - let fut = unsafe { scope.run(time_capsule) }; - active_fut.write(fut); + + unsafe { + active_fut.write(scope.run(time_capsule)); + } } +} +impl RawScope +where + T: for<'a> Family<'a>, + F: Future, +{ /// # Safety /// - /// 1. The `this` parameter is *dereference-able*. - /// 2. `open` was called on `this` - /// 3. The `this` parameter verifies the pin guarantees + /// TODO docs #[allow(unused_unsafe)] pub(crate) unsafe fn enter<'borrow, Output: 'borrow, G>(this: NonNull, f: G) -> Output where G: for<'a> FnOnce(&'a mut >::Family) -> Output, { - // SAFETY: `this` is dereference-able as per precondition (1) - let this = unsafe { this.as_ref() }; + let RawScopeFields { state, active_fut } = Self::fields(this.as_ptr()); - // SAFETY: RawScope is !Sync + the reference is released by the end of the function. - let fut = this.active_fut.get().as_mut().unwrap(); - // SAFETY: per precondition (2) - let fut = fut.assume_init_mut(); - // SAFETY: per precondition (3) - let fut = unsafe { Pin::new_unchecked(fut) }; - // SAFETY: we didn't do anything particular here before calling `poll`, which may panic, so - // we have nothing to handle. - match fut.poll(&mut std::task::Context::from_waker(&waker::create())) { - Poll::Ready(_) => unreachable!(), + let active_fut: Pin<&mut F> = unsafe { Pin::new_unchecked(&mut *active_fut) }; + + match active_fut.poll(&mut std::task::Context::from_waker(&waker::create())) { + Poll::Ready(never) => match never {}, Poll::Pending => {} } - let state = this.state.0.get(); - // SAFETY: cast the lifetime of the Family to `'borrow`. - // This is safe to do - let state: *mut ::Family = state.cast(); - let output; - { - // SAFETY: The `state` variable has been set to a dereference-able value by the future, - // or kept its NULL value. - let state = unsafe { - state - .as_mut() - .expect("The scope's future did not fill the value") - }; - // SAFETY: we're already in a clean state here even if `f` panics. - // (not doing anything afterwards beside returning `output`) - output = f(state); - } - output - } -} - -impl RawScope> -where - T: for<'a> Family<'a>, - F: Future, -{ - /// # SAFETY - /// - /// - This function must be called **after** calling `open(this)` - pub(crate) unsafe fn erase(this: NonNull) -> NonNull> { - this.cast() - } -} -impl RawScope> -where - T: for<'a> Family<'a>, - F: Future + 'static, -{ - pub(crate) unsafe fn open_erased>( - this: NonNull, - scope: S, - ) { - // SAFETY: `this` is dereference-able as per precondition (1) - let this = unsafe { this.as_ref() }; - // SAFETY: the mut reference is exclusive because: - // - the scope is !Sync - // - it is released by the end of the function - let active_fut = unsafe { this.active_fut.get().as_mut() }.unwrap(); + let mut_ref = state + .read() + .expect("The scope's future did not fill the value") + .as_mut(); - let state: *const State = &this.state; - let time_capsule = TimeCapsule { state }; - // SAFETY: called run from the executor - let fut = unsafe { scope.run(time_capsule) }; - active_fut.write(NoFuture::new(fut)); + f(mut_ref) } } @@ -240,21 +199,17 @@ where mut self: std::pin::Pin<&mut Self>, _cx: &mut std::task::Context<'_>, ) -> Poll { - // SAFETY: `state` has been set in the future by the scope - let state = unsafe { self.state.as_ref().unwrap() }; - if state.0.get().is_null() { + let state: &mut State = unsafe { &mut *self.state }; + if state.is_none() { let mut_ref = self .mut_ref .take() .expect("poll called several times on the same future"); - let mut_ref: *mut ::Family = mut_ref; - // SAFETY: Will be given back a reasonable lifetime in the `enter` method. - let mut_ref: *mut >::Family = mut_ref.cast(); - state.0.set(mut_ref); + *state = Some(mut_ref); Poll::Pending } else { - state.0.set(std::ptr::null_mut()); + *state = None; Poll::Ready(()) } } From 0b7d869a461ae35450874418ab03aea6496e5e51 Mon Sep 17 00:00:00 2001 From: = Date: Sun, 10 Mar 2024 21:04:16 +0100 Subject: [PATCH 3/5] Deny elided lifetimes in paths, and unsafe operations in unsafe fn --- src/lib.rs | 2 ++ src/raw_scope.rs | 14 ++++++++------ 2 files changed, 10 insertions(+), 6 deletions(-) diff --git a/src/lib.rs b/src/lib.rs index 09f3ae2..a2d90a9 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -1,5 +1,7 @@ #![warn(rustdoc::broken_intra_doc_links)] #![warn(missing_docs)] +#![deny(elided_lifetimes_in_paths)] +#![deny(unsafe_op_in_unsafe_fn)] #![doc = include_str!("../README.md")] #![doc( html_favicon_url = "https://raw.githubusercontent.com/dureuill/nolife/main/assets/nolife-tr.png?raw=true" diff --git a/src/raw_scope.rs b/src/raw_scope.rs index 7eef211..1ef0639 100644 --- a/src/raw_scope.rs +++ b/src/raw_scope.rs @@ -148,7 +148,7 @@ where F: Future, S: TopScope, { - let RawScopeFields { state, active_fut } = Self::fields(this); + let RawScopeFields { state, active_fut } = unsafe { Self::fields(this) }; let time_capsule = TimeCapsule { state }; @@ -171,7 +171,7 @@ where where G: for<'a> FnOnce(&'a mut >::Family) -> Output, { - let RawScopeFields { state, active_fut } = Self::fields(this.as_ptr()); + let RawScopeFields { state, active_fut } = unsafe { Self::fields(this.as_ptr()) }; let active_fut: Pin<&mut F> = unsafe { Pin::new_unchecked(&mut *active_fut) }; @@ -180,10 +180,12 @@ where Poll::Pending => {} } - let mut_ref = state - .read() - .expect("The scope's future did not fill the value") - .as_mut(); + let mut_ref = unsafe { + state + .read() + .expect("The scope's future did not fill the value") + .as_mut() + }; f(mut_ref) } From bab87c5216fa0acd91d7f833985b1d64a7f8c2a7 Mon Sep 17 00:00:00 2001 From: Louis Dureuil Date: Sat, 16 Mar 2024 13:49:12 +0100 Subject: [PATCH 4/5] Add some SAFETY preconditions and invariants --- src/box_scope.rs | 10 +++++++++- src/raw_scope.rs | 23 +++++++++++++++++++++-- 2 files changed, 30 insertions(+), 3 deletions(-) diff --git a/src/box_scope.rs b/src/box_scope.rs index 8ff2df6..b7534be 100644 --- a/src/box_scope.rs +++ b/src/box_scope.rs @@ -97,6 +97,9 @@ where let raw_scope: *mut RawScope = raw_scope.cast(); + // SAFETY: + // 1. `raw_scope` allocated by the `Box` so is valid memory. + // 2. TODO unsafe { RawScope::open(raw_scope, scope); } @@ -104,6 +107,7 @@ where mem::forget(panic_guard); // defuse guard // (guard field has no drop glue, so this does not leak anything, it just skips the above `Drop` impl) + // SAFETY: `raw_scope` allocated by the `Box` so is non-null. BoxScope(unsafe { NonNull::new_unchecked(raw_scope) }) } } @@ -124,7 +128,11 @@ where where G: for<'a> FnOnce(&'a mut >::Family) -> Output, { - // SAFETY: `self.0` is dereference-able due to coming from a `Box`. + // SAFETY: + // 1. `self.0` is dereference-able due to coming from a `Box`. + // 2. The object pointed to by `self.0` did not move and won't before deallocation. + // 3. `BoxScope::enter` takes an exclusive reference. + // 4. TODO unsafe { RawScope::enter(self.0, f) } } } diff --git a/src/raw_scope.rs b/src/raw_scope.rs index 1ef0639..6bf9b40 100644 --- a/src/raw_scope.rs +++ b/src/raw_scope.rs @@ -141,17 +141,22 @@ where { /// # Safety /// - /// TODO docs + /// 1. `this` is dereferenceable + /// 2. TODO: any precondition on `this` for RawScope::fields is satisfied. pub(crate) unsafe fn open>(this: *mut Self, scope: S) where T: for<'a> Family<'a>, F: Future, S: TopScope, { + // SAFETY: precondition (2) let RawScopeFields { state, active_fut } = unsafe { Self::fields(this) }; let time_capsule = TimeCapsule { state }; + // SAFETY: + // - precondition (1) + // - using `scope.run` from the executor. unsafe { active_fut.write(scope.run(time_capsule)); } @@ -165,14 +170,19 @@ where { /// # Safety /// - /// TODO docs + /// 1. `this` is dereferenceable + /// 2. `this` verifies the guarantees of `Pin` (one of its fields is pinned in this function) + /// 3. No other exclusive reference to the frozen value. In particular, no concurrent calls to this function. + /// 4. TODO: any precondition on `this` for RawScope::fields is satisfied. #[allow(unused_unsafe)] pub(crate) unsafe fn enter<'borrow, Output: 'borrow, G>(this: NonNull, f: G) -> Output where G: for<'a> FnOnce(&'a mut >::Family) -> Output, { + // SAFETY: precondition (4) let RawScopeFields { state, active_fut } = unsafe { Self::fields(this.as_ptr()) }; + // SAFETY: precondition (2) let active_fut: Pin<&mut F> = unsafe { Pin::new_unchecked(&mut *active_fut) }; match active_fut.poll(&mut std::task::Context::from_waker(&waker::create())) { @@ -180,6 +190,12 @@ where Poll::Pending => {} } + // SAFETY: + // - dereferenceable: precondition (1) + // - drop: reading a reference (no drop glue) + // - aliasing: precondition (3) + `mut_ref` cannot escape this function via `f` + // - lifetime: the value is still live due to the precondition on `Scope::run`, + // preventing let mut_ref = unsafe { state .read() @@ -201,6 +217,9 @@ where mut self: std::pin::Pin<&mut Self>, _cx: &mut std::task::Context<'_>, ) -> Poll { + // SAFETY: + // - state was set to a valid value in [`TimeCapsule::freeze`] + // - the value is still 'live', due to the lifetime in `FrozenFuture` let state: &mut State = unsafe { &mut *self.state }; if state.is_none() { let mut_ref = self From 9aa2c0a3247b92fc0f9da7ab4624ef82ffe1cf01 Mon Sep 17 00:00:00 2001 From: Louis Dureuil Date: Sat, 16 Mar 2024 14:58:45 +0100 Subject: [PATCH 5/5] Add safety considerations for `RawScopeFields` --- src/box_scope.rs | 11 ++++++----- src/raw_scope.rs | 23 ++++++++++++++++------- 2 files changed, 22 insertions(+), 12 deletions(-) diff --git a/src/box_scope.rs b/src/box_scope.rs index b7534be..b30889e 100644 --- a/src/box_scope.rs +++ b/src/box_scope.rs @@ -98,8 +98,10 @@ where let raw_scope: *mut RawScope = raw_scope.cast(); // SAFETY: - // 1. `raw_scope` allocated by the `Box` so is valid memory. - // 2. TODO + // 1. `raw_scope` allocated by the `Box` so is valid memory, although the future is not yet initialized + // 2. `raw_scope` was created from a valid `RawScope::>`, so `state` is fully initialized. + // + // Note: as a post-condition of `RawScope`, `raw_scope` is fully initialized. unsafe { RawScope::open(raw_scope, scope); } @@ -129,10 +131,9 @@ where G: for<'a> FnOnce(&'a mut >::Family) -> Output, { // SAFETY: - // 1. `self.0` is dereference-able due to coming from a `Box`. + // 1. `self.0` is valid as a post-condition of `new_typed`. // 2. The object pointed to by `self.0` did not move and won't before deallocation. - // 3. `BoxScope::enter` takes an exclusive reference. - // 4. TODO + // 3. `BoxScope::enter` takes an exclusive reference and the reference passed to `f` cannot escape `f`. unsafe { RawScope::enter(self.0, f) } } } diff --git a/src/raw_scope.rs b/src/raw_scope.rs index 6bf9b40..6c3520b 100644 --- a/src/raw_scope.rs +++ b/src/raw_scope.rs @@ -126,9 +126,15 @@ impl RawScope where T: for<'a> Family<'a>, { + /// SAFETY: + /// + /// 1. `this` points to an allocation that can hold a `RawScope`, + /// not necessarily initialized or properly aligned. unsafe fn fields(this: *mut Self) -> RawScopeFields { RawScopeFields { + // SAFETY: precondition (1) state: unsafe { addr_of_mut!((*this).state) }, + // SAFETY: precondition (1) active_fut: unsafe { addr_of_mut!((*this).active_fut) }, } } @@ -141,22 +147,26 @@ where { /// # Safety /// - /// 1. `this` is dereferenceable - /// 2. TODO: any precondition on `this` for RawScope::fields is satisfied. + /// 1. `this` points to a properly aligned allocation that can hold a `RawScope`, where `active_fut` is not necessarily initialized. + /// 2. `this.state` is initialized. + /// + /// # Post-condition + /// + /// 1. `this.active_fut` is fully initialized pub(crate) unsafe fn open>(this: *mut Self, scope: S) where T: for<'a> Family<'a>, F: Future, S: TopScope, { - // SAFETY: precondition (2) + // SAFETY: precondition (1) let RawScopeFields { state, active_fut } = unsafe { Self::fields(this) }; let time_capsule = TimeCapsule { state }; // SAFETY: // - precondition (1) - // - using `scope.run` from the executor. + // - using `scope.run` from the executor unsafe { active_fut.write(scope.run(time_capsule)); } @@ -170,16 +180,15 @@ where { /// # Safety /// - /// 1. `this` is dereferenceable + /// 1. `this` points to a properly aligned, fully initialized `RawScope`. /// 2. `this` verifies the guarantees of `Pin` (one of its fields is pinned in this function) /// 3. No other exclusive reference to the frozen value. In particular, no concurrent calls to this function. - /// 4. TODO: any precondition on `this` for RawScope::fields is satisfied. #[allow(unused_unsafe)] pub(crate) unsafe fn enter<'borrow, Output: 'borrow, G>(this: NonNull, f: G) -> Output where G: for<'a> FnOnce(&'a mut >::Family) -> Output, { - // SAFETY: precondition (4) + // SAFETY: precondition (1) let RawScopeFields { state, active_fut } = unsafe { Self::fields(this.as_ptr()) }; // SAFETY: precondition (2)