Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Task dropped in wrong thread when aborting LocalSet task #3929

Closed
Darksonn opened this issue Jul 6, 2021 · 2 comments
Closed

Task dropped in wrong thread when aborting LocalSet task #3929

Darksonn opened this issue Jul 6, 2021 · 2 comments
Labels
A-tokio Area: The main tokio crate C-bug Category: This is a bug. I-unsound 💥 A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness M-task Module: tokio/task

Comments

@Darksonn
Copy link
Contributor

Darksonn commented Jul 6, 2021

When aborting a task with JoinHandle::abort, the future is dropped in the thread calling abort if the task is not currently being executed. This is incorrect for tasks spawned on a LocalSet. See this example that exploits it to send a non-send value to a different thread:

use tokio::task::LocalSet;
use std::time::Duration;

struct PrintOnDrop {
    created_on: std::thread::ThreadId,
    marker: std::marker::PhantomData<*const ()>,
}

impl PrintOnDrop {
    fn new() -> Self {
        Self {
            created_on: std::thread::current().id(),
            marker: std::marker::PhantomData,
        }
    }
}
impl Drop for PrintOnDrop {
    fn drop(&mut self) {
        if std::thread::current().id() != self.created_on {
            println!("non-Send value dropped in another thread!");
        }
    }
}

#[tokio::main]
async fn main() {
    let ls = LocalSet::new();
    let pod = PrintOnDrop::new();
    let jh = ls.spawn_local(async move {
        std::future::pending::<()>().await;
        drop(pod);
    });
    
    std::thread::spawn(move || {
        std::thread::sleep(Duration::from_millis(10));
        jh.abort();
    });
    
    ls.await;
}

This can easily result in race conditions as many projects use Rc or RefCell in their Tokio tasks for better performance.

@Darksonn Darksonn added C-bug Category: This is a bug. A-tokio Area: The main tokio crate M-task Module: tokio/task I-unsound 💥 A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness labels Jul 6, 2021
@Darksonn
Copy link
Contributor Author

Darksonn commented Jul 6, 2021

Note: I have some follow-up PRs to #3909 planned, which will fix this issue.

@Darksonn
Copy link
Contributor Author

Darksonn commented Jul 7, 2021

This was fixed in #3934. The fix has been backported to all minor releases that are at most 3 months old which includes 1.5 and newer.

@Darksonn Darksonn closed this as completed Jul 7, 2021
bors bot added a commit to nervosnetwork/ckb that referenced this issue Jul 8, 2021
2813: chore(deps): bump tokio from 1.8.0 to 1.8.1 r=quake,driftluo a=yangby-cryptape

Fix [RUSTSEC-2021-0072: Task dropped in wrong thread when aborting LocalSet task](https://rustsec.org/advisories/RUSTSEC-2021-0072).

See tokio-rs/tokio#3929 for more details.

Co-authored-by: Boyu Yang <yangby@cryptape.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-tokio Area: The main tokio crate C-bug Category: This is a bug. I-unsound 💥 A soundness hole (worst kind of bug), see: https://en.wikipedia.org/wiki/Soundness M-task Module: tokio/task
Projects
None yet
Development

No branches or pull requests

1 participant