Remove unsound ScopedCoroutine

This is unsound because it allows values on the stack to be forgotten
without running their associated drop code, which is unsound for
constructs like scoped threads.

Fixes #27

README.md

@@ -172,19 +172,27 @@ fn main() {
 ```text
    0: backtrace::main::{{closure}}
              at examples/backtrace.rs:11:21
-      corosensei::coroutine::ScopedCoroutine<Input,Yield,Return,Stack>::with_stack::coroutine_func::{{closure}}
-             at src/coroutine.rs:174:62
+      corosensei::coroutine::Coroutine<Input,Yield,Return,Stack>::with_stack::coroutine_func::{{closure}}
+             at src/coroutine.rs:163:62
+      <core::panic::unwind_safe::AssertUnwindSafe<F> as core::ops::function::FnOnce<()>>::call_once
+             at /rustc/8f9080db423ca0fb6bef0686ce9a93940cdf1f13/library/core/src/panic/unwind_safe.rs:272:9
+      std::panicking::try::do_call
+             at /rustc/8f9080db423ca0fb6bef0686ce9a93940cdf1f13/library/std/src/panicking.rs:559:40
+      std::panicking::try
+             at /rustc/8f9080db423ca0fb6bef0686ce9a93940cdf1f13/library/std/src/panicking.rs:523:19
+   1: std::panic::catch_unwind
+             at /rustc/8f9080db423ca0fb6bef0686ce9a93940cdf1f13/library/std/src/panic.rs:149:14
       corosensei::unwind::catch_unwind_at_root
-             at src/unwind.rs:43:16
-   1: corosensei::coroutine::ScopedCoroutine<Input,Yield,Return,Stack>::with_stack::coroutine_func
-             at src/coroutine.rs:174:30
+             at src/unwind.rs:227:13
+      corosensei::coroutine::Coroutine<Input,Yield,Return,Stack>::with_stack::coroutine_func
+             at src/coroutine.rs:163:30
    2: stack_init_trampoline
    3: corosensei::arch::x86_64::switch_and_link
-             at src/arch/x86_64.rs:302:5
-      corosensei::coroutine::ScopedCoroutine<Input,Yield,Return,Stack>::resume_inner
-             at src/coroutine.rs:256:13
-      corosensei::coroutine::ScopedCoroutine<Input,Yield,Return,Stack>::resume
-             at src/coroutine.rs:235:19
+             at src/unwind.rs:137:17
+      corosensei::coroutine::Coroutine<Input,Yield,Return,Stack>::resume_inner
+             at src/coroutine.rs:239:13
+      corosensei::coroutine::Coroutine<Input,Yield,Return,Stack>::resume
+             at src/coroutine.rs:218:19
       backtrace::sub_function
              at examples/backtrace.rs:6:5
    4: backtrace::main

src/coroutine.rs

@@ -5,9 +5,11 @@ use core::mem::{self, ManuallyDrop};
 use core::ptr;
 
 use crate::arch::{self, STACK_ALIGNMENT};
+#[cfg(feature = "default-stack")]
+use crate::stack::DefaultStack;
 #[cfg(windows)]
 use crate::stack::StackTebFields;
-use crate::stack::{self, DefaultStack, StackPointer};
+use crate::stack::{self, StackPointer};
 use crate::trap::CoroutineTrapHandler;
 use crate::unwind::{self, initial_func_abi, CaughtPanic, ForcedUnwindErr};
 use crate::util::{self, EncodedValue};
@@ -40,22 +42,9 @@ impl<Yield, Return> CoroutineResult<Yield, Return> {
     }
 }
 
-/// Alias for a [`ScopedCoroutine`] with a `'static` lifetime.
-///
-/// This means that the function executing in the coroutine does not borrow
-/// anything from its caller.
-pub type Coroutine<Input, Yield, Return, Stack = DefaultStack> =
-    ScopedCoroutine<'static, Input, Yield, Return, Stack>;
-
 /// A coroutine wraps a closure and allows suspending its execution more than
 /// once, returning a value each time.
 ///
-/// # Lifetime
-///
-/// The `'a` lifetime here refers to the lifetime of the initial function in a
-/// coroutine and ensures that the coroutine doesn't exceed the lifetime of the
-/// initial function.
-///
 /// # Dropping a coroutine
 ///
 /// When a coroutine is dropped, its stack must be unwound so that all object on
@@ -73,7 +62,8 @@ pub type Coroutine<Input, Yield, Return, Stack = DefaultStack> =
 /// However if all of the code executed by a coroutine is under your control and
 /// you can ensure that all types on the stack when a coroutine is suspended
 /// are `Send` then it is safe to manually implement `Send` for a coroutine.
-pub struct ScopedCoroutine<'a, Input, Yield, Return, Stack: stack::Stack> {
+#[cfg(feature = "default-stack")]
+pub struct Coroutine<Input, Yield, Return, Stack: stack::Stack = DefaultStack> {
     // Stack that the coroutine is executing on.
     stack: Stack,
 
@@ -93,14 +83,14 @@ pub struct ScopedCoroutine<'a, Input, Yield, Return, Stack: stack::Stack> {
     // never been resumed.
     drop_fn: unsafe fn(ptr: *mut u8),
 
-    // We want to be covariant over 'a, Yield and Return, and contravariant
+    // We want to be covariant over Yield and Return, and contravariant
     // over Input.
     //
     // Effectively this means that we can pass a
-    //   ScopedCoroutine<'static, &'a (), &'static (), &'static ()>
+    //   Coroutine<&'a (), &'static (), &'static ()>
     // to a function that expects a
-    //   ScopedCoroutine<'b, &'static (), &'c (), &'d ()>
-    marker: PhantomData<&'a fn(Input) -> CoroutineResult<Yield, Return>>,
+    //   Coroutine<&'static (), &'c (), &'d ()>
+    marker: PhantomData<fn(Input) -> CoroutineResult<Yield, Return>>,
 
     // Coroutine must be !Send.
     /// ```compile_fail
@@ -110,14 +100,44 @@ pub struct ScopedCoroutine<'a, Input, Yield, Return, Stack: stack::Stack> {
     marker2: PhantomData<*mut ()>,
 }
 
+/// A coroutine wraps a closure and allows suspending its execution more than
+/// once, returning a value each time.
+///
+/// # Dropping a coroutine
+///
+/// When a coroutine is dropped, its stack must be unwound so that all object on
+/// it are properly dropped. This is done by calling `force_unwind` to unwind
+/// the stack. If `force_unwind` fails then the program is aborted.
+///
+/// See the [`Coroutine::force_unwind`] function for more details.
+///
+/// # `Send`
+///
+/// In the general case, a coroutine can only be sent to another if all of the
+/// data on its stack is `Send`. There is no way to guarantee this using Rust
+/// language features so `Coroutine` does not implement the `Send` trait.
+///
+/// However if all of the code executed by a coroutine is under your control and
+/// you can ensure that all types on the stack when a coroutine is suspended
+/// are `Send` then it is safe to manually implement `Send` for a coroutine.
+#[cfg(not(feature = "default-stack"))]
+pub struct Coroutine<Input, Yield, Return, Stack: stack::Stack> {
+    stack: Stack,
+    stack_ptr: Option<StackPointer>,
+    initial_stack_ptr: StackPointer,
+    drop_fn: unsafe fn(ptr: *mut u8),
+    marker: PhantomData<fn(Input) -> CoroutineResult<Yield, Return>>,
+    marker2: PhantomData<*mut ()>,
+}
+
 // Coroutines can be Sync if the stack is Sync.
 unsafe impl<Input, Yield, Return, Stack: stack::Stack + Sync> Sync
-    for ScopedCoroutine<'_, Input, Yield, Return, Stack>
+    for Coroutine<Input, Yield, Return, Stack>
 {
 }
 
 #[cfg(feature = "default-stack")]
-impl<'a, Input, Yield, Return> ScopedCoroutine<'a, Input, Yield, Return, DefaultStack> {
+impl<Input, Yield, Return> Coroutine<Input, Yield, Return, DefaultStack> {
     /// Creates a new coroutine which will execute `func` on a new stack.
     ///
     /// This function returns a `Coroutine` which, when resumed, will execute
@@ -126,15 +146,13 @@ impl<'a, Input, Yield, Return> ScopedCoroutine<'a, Input, Yield, Return, Default
     pub fn new<F>(f: F) -> Self
     where
         F: FnOnce(&Yielder<Input, Yield>, Input) -> Return,
-        F: 'a,
+        F: 'static,
     {
         Self::with_stack(Default::default(), f)
     }
 }
 
-impl<'a, Input, Yield, Return, Stack: stack::Stack>
-    ScopedCoroutine<'a, Input, Yield, Return, Stack>
-{
+impl<Input, Yield, Return, Stack: stack::Stack> Coroutine<Input, Yield, Return, Stack> {
     /// Creates a new coroutine which will execute `func` on the given stack.
     ///
     /// This function returns a coroutine which, when resumed, will execute
@@ -143,7 +161,7 @@ impl<'a, Input, Yield, Return, Stack: stack::Stack>
     pub fn with_stack<F>(stack: Stack, f: F) -> Self
     where
         F: FnOnce(&Yielder<Input, Yield>, Input) -> Return,
-        F: 'a,
+        F: 'static,
     {
         // The ABI of the initial function is either "C" or "C-unwind" depending
         // on whether the "asm-unwind" feature is enabled.
@@ -435,9 +453,7 @@ impl<'a, Input, Yield, Return, Stack: stack::Stack>
     }
 }
 
-impl<'a, Input, Yield, Return, Stack: stack::Stack> Drop
-    for ScopedCoroutine<'a, Input, Yield, Return, Stack>
-{
+impl<Input, Yield, Return, Stack: stack::Stack> Drop for Coroutine<Input, Yield, Return, Stack> {
     fn drop(&mut self) {
         let guard = scopeguard::guard((), |()| {
             // We can't catch panics in #![no_std], force an abort using

src/lib.rs

@@ -165,19 +165,27 @@
 //! ```text
 //!    0: backtrace::main::{{closure}}
 //!              at examples/backtrace.rs:11:21
-//!       corosensei::coroutine::ScopedCoroutine<Input,Yield,Return,Stack>::with_stack::coroutine_func::{{closure}}
-//!              at src/coroutine.rs:174:62
+//!       corosensei::coroutine::Coroutine<Input,Yield,Return,Stack>::with_stack::coroutine_func::{{closure}}
+//!              at src/coroutine.rs:163:62
+//!       <core::panic::unwind_safe::AssertUnwindSafe<F> as core::ops::function::FnOnce<()>>::call_once
+//!              at /rustc/8f9080db423ca0fb6bef0686ce9a93940cdf1f13/library/core/src/panic/unwind_safe.rs:272:9
+//!       std::panicking::try::do_call
+//!              at /rustc/8f9080db423ca0fb6bef0686ce9a93940cdf1f13/library/std/src/panicking.rs:559:40
+//!       std::panicking::try
+//!              at /rustc/8f9080db423ca0fb6bef0686ce9a93940cdf1f13/library/std/src/panicking.rs:523:19
+//!    1: std::panic::catch_unwind
+//!              at /rustc/8f9080db423ca0fb6bef0686ce9a93940cdf1f13/library/std/src/panic.rs:149:14
 //!       corosensei::unwind::catch_unwind_at_root
-//!              at src/unwind.rs:43:16
-//!    1: corosensei::coroutine::ScopedCoroutine<Input,Yield,Return,Stack>::with_stack::coroutine_func
-//!              at src/coroutine.rs:174:30
+//!              at src/unwind.rs:227:13
+//!       corosensei::coroutine::Coroutine<Input,Yield,Return,Stack>::with_stack::coroutine_func
+//!              at src/coroutine.rs:163:30
 //!    2: stack_init_trampoline
 //!    3: corosensei::arch::x86_64::switch_and_link
-//!              at src/arch/x86_64.rs:302:5
-//!       corosensei::coroutine::ScopedCoroutine<Input,Yield,Return,Stack>::resume_inner
-//!              at src/coroutine.rs:256:13
-//!       corosensei::coroutine::ScopedCoroutine<Input,Yield,Return,Stack>::resume
-//!              at src/coroutine.rs:235:19
+//!              at src/unwind.rs:137:17
+//!       corosensei::coroutine::Coroutine<Input,Yield,Return,Stack>::resume_inner
+//!              at src/coroutine.rs:239:13
+//!       corosensei::coroutine::Coroutine<Input,Yield,Return,Stack>::resume
+//!              at src/coroutine.rs:218:19
 //!       backtrace::sub_function
 //!              at examples/backtrace.rs:6:5
 //!    4: backtrace::main
@@ -227,7 +235,7 @@
 //! Implies `unwind`.
 
 #![no_std]
-#![cfg_attr(feature = "asm-unwind", feature(asm_unwind, c_unwind, asm_sym))]
+#![cfg_attr(feature = "asm-unwind", feature(asm_unwind, c_unwind))]
 #![warn(missing_docs)]
 
 // Must come first because it defines macros used by other modules.

src/stack/mod.rs

@@ -15,11 +15,6 @@ cfg_if::cfg_if! {
     } else if #[cfg(all(feature = "default-stack", windows))] {
         mod windows;
         pub use self::windows::DefaultStack;
-    } else {
-        /// Dummy stack for platforms that do not provide a default stack.
-        ///
-        /// This is only here for use as a default generic parameter.
-        pub struct DefaultStack;
     }
 }
 

src/tests/coroutine.rs

@@ -1,12 +1,15 @@
+extern crate alloc;
 extern crate std;
 
+use core::sync::atomic::{AtomicBool, Ordering};
 use std::cell::Cell;
 use std::rc::Rc;
 use std::string::ToString;
 use std::{println, ptr};
 
-use crate::coroutine::Coroutine;
-use crate::{CoroutineResult, ScopedCoroutine};
+use alloc::sync::Arc;
+
+use crate::{Coroutine, CoroutineResult};
 
 #[test]
 fn smoke() {
@@ -95,7 +98,7 @@ fn panics_propagated() {
     let a = Rc::new(Cell::new(false));
     let b = SetOnDrop(a.clone());
     let mut coroutine = Coroutine::<(), (), ()>::new(move |_, ()| {
-        drop(&b);
+        drop(b);
         panic!("foobar");
     });
     let result = panic::catch_unwind(AssertUnwindSafe(|| coroutine.resume(())));
@@ -121,7 +124,7 @@ fn panics_propagated_via_parent() {
     let a = Rc::new(Cell::new(false));
     let b = SetOnDrop(a.clone());
     let mut coroutine = Coroutine::<(), (), ()>::new(move |y, ()| {
-        drop(&b);
+        drop(b);
         y.on_parent_stack(|| {
             panic!("foobar");
         });
@@ -156,6 +159,7 @@ fn suspend_and_resume_values() {
 
 #[test]
 fn stateful() {
+    #[allow(dead_code)]
     #[repr(align(128))]
     struct Aligned(u8);
     let state = [41, 42, 43, 44, 45];
@@ -174,18 +178,18 @@ fn stateful() {
 
 #[test]
 fn force_unwind() {
-    struct SetOnDrop<'a>(&'a mut bool);
-    impl<'a> Drop for SetOnDrop<'a> {
+    struct SetOnDrop(Arc<AtomicBool>);
+    impl Drop for SetOnDrop {
         fn drop(&mut self) {
-            *self.0 = true;
+            self.0.store(true, Ordering::Relaxed);
         }
     }
 
-    let mut a = false;
-    let mut b = false;
-    let a_drop = SetOnDrop(&mut a);
-    let b_drop = SetOnDrop(&mut b);
-    let mut coroutine = ScopedCoroutine::<(), (), (), _>::new(move |y, ()| {
+    let a = Arc::new(AtomicBool::new(false));
+    let b = Arc::new(AtomicBool::new(false));
+    let a_drop = SetOnDrop(a.clone());
+    let b_drop = SetOnDrop(b.clone());
+    let mut coroutine = Coroutine::<(), (), (), _>::new(move |y, ()| {
         drop(a_drop);
         y.suspend(());
         drop(b_drop);
@@ -196,16 +200,16 @@ fn force_unwind() {
     assert!(coroutine.started());
     assert!(coroutine.done());
     drop(coroutine);
-    assert!(a);
-    assert!(b);
+    assert!(a.load(Ordering::Relaxed));
+    assert!(b.load(Ordering::Relaxed));
 
     #[cfg(feature = "unwind")]
     {
-        let mut a = false;
-        let mut b = false;
-        let a_drop = SetOnDrop(&mut a);
-        let b_drop = SetOnDrop(&mut b);
-        let mut coroutine = ScopedCoroutine::<(), (), (), _>::new(move |y, ()| {
+        let a = Arc::new(AtomicBool::new(false));
+        let b = Arc::new(AtomicBool::new(false));
+        let a_drop = SetOnDrop(a.clone());
+        let b_drop = SetOnDrop(b.clone());
+        let mut coroutine = Coroutine::<(), (), (), _>::new(move |y, ()| {
             drop(a_drop);
             y.suspend(());
             drop(b_drop);
@@ -217,15 +221,15 @@ fn force_unwind() {
         assert!(coroutine.started());
         assert!(coroutine.done());
         drop(coroutine);
-        assert!(a);
-        assert!(b);
+        assert!(a.load(Ordering::Relaxed));
+        assert!(b.load(Ordering::Relaxed));
     }
 
-    let mut a = false;
-    let mut b = false;
-    let a_drop = SetOnDrop(&mut a);
-    let b_drop = SetOnDrop(&mut b);
-    let mut coroutine = ScopedCoroutine::<(), (), (), _>::new(move |y, ()| {
+    let a = Arc::new(AtomicBool::new(false));
+    let b = Arc::new(AtomicBool::new(false));
+    let a_drop = SetOnDrop(a.clone());
+    let b_drop = SetOnDrop(b.clone());
+    let mut coroutine = Coroutine::<(), (), (), _>::new(move |y, ()| {
         drop(a_drop);
         y.suspend(());
         drop(b_drop);
@@ -238,8 +242,8 @@ fn force_unwind() {
     assert!(coroutine.started());
     assert!(coroutine.done());
     drop(coroutine);
-    assert!(a);
-    assert!(b);
+    assert!(a.load(Ordering::Relaxed));
+    assert!(b.load(Ordering::Relaxed));
 }
 
 // The Windows stack starts out small with only one page comitted. Check that it

src/tests/on_stack.rs

@@ -59,7 +59,7 @@ fn panics_propagated() {
     let b = SetOnDrop(a.clone());
     let result = panic::catch_unwind(AssertUnwindSafe(move || {
         on_stack(DefaultStack::default(), move || {
-            drop(&b);
+            drop(b);
             panic!("foobar");
         })
     }));