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

impl Sync for Bunch<T> need a Sync bound on T #1

Open
JOE1994 opened this issue Nov 12, 2020 · 2 comments · May be fixed by #2
Open

impl Sync for Bunch<T> need a Sync bound on T #1

JOE1994 opened this issue Nov 12, 2020 · 2 comments · May be fixed by #2

Comments

@JOE1994
Copy link

JOE1994 commented Nov 12, 2020

Hello 🦀 ,

Currently in the Sync impl for Bunch<T>, there is no Sync bound on T.
With that, it's possible to write code that causes undefined behavior when Bunch is used from multiple threads.

unsafe impl<T> Sync for Bunch<T> {}

Below is a small & contrived proof-of-concept, where a segmentation fault can occur due to data race.
Running the example code below in Debug mode results in a segmentation fault.

#![forbid(unsafe_code)]

use bunch::Bunch;
use std::cell::Cell;
use std::sync::Arc;
use std::thread;

// A simple tagged union used to demonstrate problems with data races in Cell.
#[derive(Debug, Clone, Copy)]
enum RefOrInt<'a> {
    Ref(&'a u64),
    Int(u64),
}
static X: u64 = 0;

fn main() {
    let bunch = Bunch::new();
    // This item is not `Sync`, but yet can be pushed to `Bunch`.
    let item_not_sync = Cell::new(RefOrInt::Ref(&X));
    bunch.push(item_not_sync);

    let arc_0 = Arc::new(bunch);
    let arc_1 = Arc::clone(&arc_0);

    let _child = thread::spawn(move || {
        let smuggled_cell = arc_1.get(0);

        loop {
            smuggled_cell.set(RefOrInt::Int(0xdeadbeef));
            smuggled_cell.set(RefOrInt::Ref(&X))
        }
    });

    loop {
        if let RefOrInt::Ref(addr) = arc_0.get(0).get() {
            if addr as *const _ as usize != 0xdeadbeef {
                continue;
            }
            // Due to the data race, obtaining Ref(0xdeadbeef) is possible
            println!("Pointer is now: {:p}", addr);
            println!("Dereferencing addr will now segfault: {}", *addr);
        }
    }
}

I think this issue can be resolved by adding a T: Sync bound to unsafe impl<T> Sync for Bunch<T> {}.
(Issue found by @sslab-gatech's Rust group)

@JOE1994
Copy link
Author

JOE1994 commented Jan 20, 2021

I forgot to mention in the above report, but it is also possible to send non-Send types to other threads using Bunch<T> since Bunch<T> implements Send without a Send bound on T.

@Shnatsel
Copy link

Heads up: this issue has been included in the RustSec advisory database. It will be surfaced by tools such as cargo-audit or cargo-deny from now on.

Once a fix is released to crates.io, please open a pull request to update the advisory with the patched version, or file an issue on the advisory database repository.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants