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

TryMutex<T> should have Send bound on T #2

Closed
ammaraskar opened this issue Nov 16, 2020 · 1 comment
Closed

TryMutex<T> should have Send bound on T #2

ammaraskar opened this issue Nov 16, 2020 · 1 comment

Comments

@ammaraskar
Copy link

Hi there, we (Rust group @sslab-gatech) are scanning crates on crates.io for potential soundness bugs. We noticed that TryMutex implements Sync for all types T:

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

This should probably be bounded by T: Send just like the standard library's Mutex, otherwise it allows smuggling non-Send types like Rc across thread-boundaries like so:

#![forbid(unsafe_code)]

use try_mutex::TryMutex;

use std::rc::Rc;
use crossbeam_utils::thread;

fn main() {
    let rc = Rc::new(());
    let rc_clone = rc.clone();

    let try_mutex = TryMutex::new(rc_clone);
    thread::scope(|s| {    
        s.spawn(|_| {
            let smuggled_rc = try_mutex.try_lock().unwrap();
            println!("RC in thread: {:p}", *smuggled_rc);
        });
    });
    println!("RC in main:   {:p}", rc);
}

This outputs:

RC in thread: 0x5642ef5ffa50
RC in main:   0x5642ef5ffa50

and can lead to data races from safe Rust code.

@mpdn
Copy link
Owner

mpdn commented Nov 16, 2020

Yup, looks like a soundness issue.

Fixed via d2691fd and new 0.3 package.

Thanks.

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

No branches or pull requests

2 participants