Pinned-scope - soundness review

code review
1

Repo: Scope for async based on limited sharing and structured concurrency.

Base idea:

  • Scope cannot borrow data from outside (can share only owned data) → it can be safely leaked
  • Is !Unpin → cannot be moved after first poll
  • Child tasks have to be fully structured → can't be leaked or dropped and have to be driven to complete to compile.

As of right now i didn't add any panic safety.

Would appreciate if someone could review soundness of unsafe in the repo.

Differences to other async scope crates:
moro / safe-async-scoped - child tasks can run in parallel
async-scoped - does not block the parent thread

2

You can sneak out the result or block indefinitely to circumvent the need to call Scope::finish. This code segfaults:

#[tokio::test(flavor = "multi_thread", worker_threads = 4)]
async fn bypass_2_2() {
    let vec = vec![1; 1 << 20];

    let (send, recv) = futures::channel::oneshot::channel();

    let fut = generic_scope::<TokioSpawner, ()>(async move |scope| {
        let spawn_fut = scope.spawn(async {
            tokio::time::sleep(std::time::Duration::from_millis(100)).await;

            // `vec` is no longer allocated, causing segfault
            vec[0] + vec[1000]
        });

        // Let `spawn_fut` spawn a task by polling once
        let mut spawn_fut = Box::pin(spawn_fut);
        (&mut spawn_fut).now_or_never();

        // And leak it
        std::mem::forget(spawn_fut);

        // Now that `spawn_fut` is leaked, the compiler thinks `vec`
        // is no longer borrowed (even though the spawned task is still
        // running)
        drop(vec);

        // We don't have `Scope` anymore but we can still send a result
        // back to the outer scope
        _ = send.send(42);

        std::future::pending().await
    });

    // Wait for the result while polling `fut`
    let futures::future::Either::Right((Ok(result), fut)) =
        futures::future::select(Box::pin(fut), recv).await
    else {
        unreachable!()
    };

    // The outer future hasn't called `Scope::finish` but
    // we can leak it anyway
    std::mem::forget(fut);

    assert_eq!(result, 42);

    // Wait for segfault to occur
    tokio::time::sleep(std::time::Duration::from_millis(500)).await;
}
3

Thank you for your time. I found solution for this: by abusing RPIT and private trait bound (sealed trait) to not let compiler infer result of main closure - to name it you need struct which is private. Works well for both std::future::pending and loop {} but to make it work i had to eliminate lifetime bound on scope which should make it more breakable.

Repo is updated, would appreciate any insights.

4

Haven't looked at the code yet (I'm on my phone right now, sorry), but just from this description I don't see why something like if true { std::future::pending().await } else { s.finish(()) } wouldn't break this fix/trick.

5

To be able to write else block you need to own s which cannot be owned if any child task was leaked, but this is good place to focus on, idk if it is even safe to abuse inference of the compiler.

6

On the current version, your it_works does not even work when moved out of lib.rs into tests/main.rs.

7

Then I could do if true { /* problematic code from yvt */ } else { s.finish(()) }, which should be fine because in the else condition s was never used/borrowed/consumed.

Chances are, if you rely on type inference for soundness then there's a way to break that.

1 Like
8

This topic was automatically closed 90 days after the last reply. We invite you to open a new topic if you have further questions or comments.