TimeCapsule&FrozenFuture is an unsound API, reliant on users to .await the .freeze(&mut …) future properly in-place
#8
Comments
|
Hello @steffahn, Thank you for taking the time to examine I was looking for this kind of reviews earlier, and I thought that announcing the crate through a blogpost would have more chances to give it some visibility, basically as an application of Cunningham's Law. Now, I'm not super happy with the outcome 😅 but such are the risks of trying a new approach. Back to this issue, my understanding of the root cause is that you can decorrelate polling the Possible fixes would entail:
(2) is not even that simple to implement, because there can be multiple levels of future, so a simple use std::marker::PhantomData;
use nolife::{BoxScope, Family, TimeCapsule};
use futures_util::{poll, pending};
struct SingleFamily<T: 'static>(PhantomData<T>);
impl<'a, T: 'static> Family<'a> for SingleFamily<T> {
type Family = T;
}
async fn inner(time_capsule: TimeCapsule<SingleFamily<String>>, data: &mut String) {
nolife::freeze!(time_capsule, data);
}
fn main() {
{
let mut scope = BoxScope::new(
|mut time_capsule: TimeCapsule<SingleFamily<String>>| async move {
let mut x = String::from("Hello World!");
let fut = inner(time_capsule, &mut x);
let _ = poll!(fut); // puts reference into state
drop(x); // then invalidates reference
pending!(); // then yields
panic!();
},
);
scope.enter(|s| {
println!("{s}");
})
}
}Or is it some other macro you had in mind? Of course we could add a |
Yes indeed, that's insufficient, of course. The macro would need to do all of
Even then, I’m not 100% sure yet what the best design could be to allow Perhaps, ultimately, the outer macro would need to generate the |
|
Oh, I’m having some interesting macro ideas now… |
|
Just a thought, could we make the situation better by marking the A |
use nolife::Family;
use std::marker::PhantomData;
struct SingleFamily<T: 'static>(PhantomData<T>);
impl<'a, T: 'static> Family<'a> for SingleFamily<T> {
type Family = T;
}
macro_rules! scope {
{$($t:tt)*} => {{
::nolife::BoxScope::new(|mut time_capsule: ::nolife::TimeCapsule<SingleFamily<String>>| async move {
'check_freeze_scope: {
#[allow(unused_macros)]
macro_rules! freeze {
($e:expr) => {{
#[allow(unreachable_code)]
if false {
break 'check_freeze_scope (loop {});
}
::nolife::TimeCapsule::freeze(&mut time_capsule, $e).await
}};
}
#[allow(unused_macros)]
macro_rules! freeze_forever {
($e:expr) => {{
#[allow(unreachable_code)]
if false {
break 'check_freeze_scope (loop {});
}
::nolife::TimeCapsule::freeze_forever(&mut time_capsule, $e).await
}};
}
{
$($t)*
}
}
})
}}
}
fn main() {
let mut sc = scope! {
let mut x = String::from("Hello World!");
let _fut = async {
// freeze!(&mut x); // usage in nested future errors
#[allow(unused_labels)]
'check_freeze_scope: {
// freeze!(&mut x); // even here, thanks to hygiene
}
};
freeze_forever!(&mut x)
};
sc.enter(|s| {
println!("{s}");
})
}this kind of approach might force all the constraints. (Some of the relevant API, either the (I haven’t tested to what extend replacing the |
|
In case you don’t want to include the |
|
Hmmm, that looks like a very smart solution, I also like the resulting interface a lot! I really, truly, appreciate you taking of reviewing this crate and providing helpful solutions this way ❤️ Am I correct that this obviously limits the ability to factor the scope in multiple functions? Consider a more complex example including error handling like this gist. Do we need to write macros for subscopes now :D ? |
I just tested this: |
Yes.
I wonder if there’s a way to create some I think that doesn’t prevent “time capsule juggling” where you could smuggle the time capsule of an outer scope into an inner scope, in order to write to it (to the outer scope) without propagating the I don’t have the time today to turn this into a concrete implementation, but I believe it could possibly be sound (and not much worse in UX than the macros above), whilst allowing factoring out helper functions as desired. |
|
Alright I think I'm starting to see a bit clearer how that would work at multiple levels.
Would that work? I'm not sure we need to brand the timecapsule with a tag lifetime, because in this variant it remains implicit to the I have an implementation in #11, but in a very rough state.
|
Ah, wrap the whole producer and never expose the actual |
There’s likely going to be some “fun extra challenge” in the form of getting the Oh… that doesn’t even work at all: use core::future::Future;
struct Wrap<P>(P);
struct Capsule;
enum Never {}
fn f(x: String) -> Wrap<impl for<'a> FnOnce(&'a mut Capsule) -> (impl Future<Output = Never> + 'a)> {
Wrap(|t: &mut _| async move {
let _x = t;
async {}.await;
loop {}
})
}and even with the nightly feature… So either use |
Ah, the closure needn’t be generic over the lifetime, I suppose… so at least on #![feature(impl_trait_in_fn_trait_return)]
use core::future::Future;
struct Wrap<P>(P);
struct Capsule;
enum Never {}
fn f<'a>(x: String) -> Wrap<impl FnOnce(&'a mut Capsule) -> (impl Future<Output = Never> + 'a)> {
Wrap(|t| async move {
let _x = t;
async {}.await;
loop {}
})
}but that doesn’t help stable much, still. |
|
Wait… wait!… a trait can help, actually! use core::future::Future;
struct Wrap<P>(P);
struct Capsule;
enum Never {}
trait Producer<'a>: FnOnce(&'a mut Capsule) -> Self::Fut {
type Fut: Future<Output = Never>;
}
impl<'a, P: ?Sized, Fut> Producer<'a> for P
where
P: FnOnce(&'a mut Capsule) -> Fut,
Fut: Future<Output = Never>,
{
type Fut = Fut;
}
fn f<'a>(x: String) -> Wrap<impl Producer<'a>> {
Wrap(|t| async move {
let _x = t;
async {}.await;
loop {}
})
} |
|
And the way to write a function with additional lifetime-having arguments would probably look something like this use core::future::Future;
struct Wrap<P>(P);
struct Capsule;
enum Never {}
trait Producer<'a>: FnOnce(&'a mut Capsule) -> Self::Fut {
type Fut: Future<Output = Never>;
}
impl<'a, P: ?Sized, Fut> Producer<'a> for P
where
P: FnOnce(&'a mut Capsule) -> Fut,
Fut: Future<Output = Never>,
{
type Fut = Fut;
}
fn f<'prod, 'a: 'prod, 'b: 'prod>(x1: &'a str, x2: &'b str) -> Wrap<impl Producer<'prod>> {
Wrap(move |t| async move {
let _x = t;
let _y1 = x1;
let _y2 = x2;
async {}.await;
loop {}
})
} |
|
Yes that's similar to what I had in mind (but I think the Wrapper itself should implement the trait, so that we can have a(n unsafe) trait function to inject the time capsule). Basically as long as we can't name a lambda/future type, this is going to be pain. |
|
Hello, I updated #11 and it now contains a I hope the syntax isn't too alien 😥 Using this new syntax, the more complex example that uses subscopes now looks like this I still need to update the counterexamples and add your counterexample to the mix (not sure how, now that we cannot access a TimeCapsule directly) |
|
You sure are working this out quickly. I'll try to review #11 tomorrow. |
So, well,
TimeCapsule<T>can accept a reference of just any lifetime forfreeze. It produces aFrozenFuture<'a, 'b, T>that does have some lifetime corresponding to that reference, but all that ensures is that the reference stays alive until that future is polled, nothing else. If you do end up just.awaiting that future in-place (in particular, converting thePoll::pendingofFrozenFutureinto an immediately propagatedPoll::pendingof the surrounding future), then all may be well, but who forces you to do this?That’s an easy exploit: Just poll the future, then invalidate the reference, and only then “propagate” the
Poll::pending. Below is a straightforward implementation of this using helper macros from thefuturescrate family:outputs garbage data, e.g.:
miri output
Unfortunately, I cannot immediately think of straightforward fixes for soundness here.1 This might need more thought though. Let’s hope there is some alternative approach out there, and that his doesn’t mean this whole crate’s idea is fundamentally broken.
Footnotes
At least without an API that involves some mandatory usage of a macro that calls
unsafe, possibly-hidden, API. ↩The text was updated successfully, but these errors were encountered: