Open Lej77 opened 1 year ago
Thanks so much for the review!
incorrect_not_in_scope
test case
This is indeed unexpected. I wasn't even aware async behaves this way.
I'll document this issue. Since this library is only being used for Async UI for now, I don't think we need the stack location check yet. But if a use case where the check would be useful comes up it can be added.
failed_abort_recursion
test case
forgettable_stack_allocations
test case
Good catch! As apparent I've completely neglected panicking behaviors. I think it is fair that we abort if drop panic as you suggested, since panic on drop often leads to double panic and aborting anyway. I updated the code to do this.
forgettable_stack_allocations
test case
I've been thinking about cases like this. It seems that scoped_async_spawn and stack allocation are each sound in isolation, but unsound when combined.
Figuring out who is correct is complicated. The pin doc mention that leaking pinned item is okay, but it also say that
Memory can be “invalidated” by deallocation, but also by replacing a Some(v) by None, or calling Vec::set_len to “kill” some elements off of a vector.
To me, leaking in stack allocation feels like Vec::set_len
. The question is how exactly does set_len
invalidate memory? I've asked on IRLO for documentation clarification.
I think that forgetting stack allocations is allowed by the pin docs as long as the stack memory is never re-used after it was forgotten. My example stack allocator does this by aborting the process but I guess the thread could also sleep forever or prevent re-use in other ways.
Slightly unrelated I actually investigated if there was an existing arena allocator that could give out pinned references. I found bumpalo::boxed::Box::pin_in which I think is actually unsound since if the returned Box
is leaked then the backing memory is freed when Bump
is dropped. So the pin guarantees are definitely easy to get wrong!
From the IRLO thread RalfJung mentioned that criteria like Vec::set_len
can be loosened further. So indeed this crate is assuming more than what Pin provide and is thus unsound.
I don't think there's anything that could be done to fix this. I'll try to implement a truly sound (based on FuturesUnordered) version of fragment
for Async UI. I'll probably keep this subtly unsound version as the default though, as it provides better performance and is really hard (requires additional unsafe, abort/infinite loop, manual poll, ...) to break.
You should open an issue in bumpalo about their unsoundness!
It does seem like forgettable stack pinning isn't used much so scoped_async_spawn
should be compatible with most async crates and therefore it should be okay to keep using it.
Also, I have opened an issue with bumpalo
about its unsound pinning.
I reviewed the
scoped_async_spawn
crate after reading the blog post Scoped threads in Rust, and why its async counterpart would be unsound. I found a couple of soundness issues related to panics and some things that probably should be documented.I will briefly describe each issue I found and suggest what should be done about it. You can find the code for the test cases at the end of this post.
incorrect_not_in_scope
test caseThis problem does not cause any soundness issues, but it is somewhat unexpected. If a
SpawnGuard
is not held over an await point then it will be stored on the current thread's stack instead of inside a future. This causes theSpawnGuard::convert_future
method to panic at line 46 of async_ui/guard.rs since thecheck_scope
function returnsfalse
.Suggested solution: document this issue or try to allow futures stored on the current thread's stack (,store a stack address when entering a scope and check if a pointer is between that stored stack address and the current stack address).
failed_abort_recursion
test case<PinCell<Inner<F>> as InnerTrait>::abort
can fail when callingRefCell::borrow_mut
at line 61 of async_ui/common.rs. This causes a panic which prevents theSpawnGuard
from aborting other futures .Suggested solution: abort the process if
SpawnGuard
'sDrop
implementation panics.failed_abort_panic
test case<PinCell<Inner<F>> as InnerTrait>::abort
can fail when callingPin::set
at line 63 of async_ui/common.rs if the overwritten future type'sDrop
implementation panics. This panic prevents theSpawnGuard
from aborting other futures.Suggested solution: abort the process if
SpawnGuard
'sDrop
implementation panics.forgettable_stack_allocations
test caseThe soundness of the
scoped_async_spawn
crate relies on the fact that you can't forget values that are pinned to the stack. This assumption is true for normal stack pinning viapin_mut
and is also upheld by pin projections (AFAIK). Still the assumption isn't guaranteed bystd
's documentation and I think it would be sound for another crate to implement something like theStackAlloc
type in my test case which allows creatingBox
-like types that store their data on the stack. Such crates would break the assumption that thescoped_async_spawn
crate relies on and cause soundness issues.Suggested solution: document that the
scoped_async_spawn
crate is incompatible with stack allocations that can be forgotten.Code for test cases
Click here to show/hide the code
```rust use std::cell::{Cell, UnsafeCell}; use std::future::Future; use std::marker::PhantomPinned; use std::mem::{ManuallyDrop, MaybeUninit}; use std::ops::{Deref, DerefMut}; use std::pin::Pin; use std::sync::{mpsc, Arc}; use std::task::{Context, Poll, Wake, Waker}; use pin_utils::pin_mut; use scoped_async_spawn::{GiveUnforgettableScope, SpawnGuard}; fn poll_once