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

Send bound needed on T (for Send impl of Bucket2) #2

Closed
JOE1994 opened this issue Nov 29, 2020 · 7 comments · Fixed by #3
Closed

Send bound needed on T (for Send impl of Bucket2) #2

JOE1994 opened this issue Nov 29, 2020 · 7 comments · Fixed by #3

Comments

@JOE1994
Copy link
Contributor

JOE1994 commented Nov 29, 2020

Hello 🦀 ,
while scanning crates.io, we (Rust group @sslab-gatech) have noticed a soundness/memory safety issue in this crate which allows safe Rust code to trigger undefined behavior.

Issue

Currently Send is implemented for Bucket2<T> even when T is not bound by Send.

unsafe impl<T> Send for Bucket2<T> {}

This makes it possible to use SyncPool<T> to send a non-Send object to other threads.

Proof of Concept

Below is an example program that exhibits undefined behavior using thesyncpool crate.
There is a data race on the internal reference count of Rc, and the program either crashes at runtime
(e.g. on Ubuntu: Illegal Instruction (Core Dumped))
, or panics at the end of the program (indicating a memory leak).
Such behavior can be observed when the program is compiled in Debug mode.

use syncpool::prelude::*;

use std::boxed::Box;
use std::rc::Rc;

const N_ITER: usize = 900_000;
const N_THREADS: usize = 6;
fn main() {
    // Non-Send object (to be sent to other threads).
    let rc = Rc::new(0_i32);

    let mut pools = vec![];
    for _ in 0..N_THREADS {
        let mut pool = SyncPool::new();
        let _dummy = pool.get();
        let malicious = Box::new(Rc::clone(&rc));
        pool.put(malicious);
        pools.push(pool);
    }

    let mut children = vec![];
    while let Some(pool) = pools.pop() {
        let c = std::thread::spawn(move || {
            // Moved `pool` to child thread.
            let mut pool = pool;
            let boxed_rc = pool.get();
    
            for _ in 0..N_ITER {
                // Data race on the internal ref count of `Rc`.
                Rc::clone(boxed_rc.as_ref());
            }
        });
        children.push(c);
    }
    // Join child threads.
    for child in children {
        child.join().unwrap();
    }

    assert_eq!(Rc::strong_count(&rc), 1);
}

The example is a bit contrived, but it triggers undefined behavior in safe Rust code.

How to fix the issue

The solution is to add a Send bound on T in the Send impl for Bucket2<T> as below.
I tested the above example using the modified version of the crate, and the compiler was able to successfully
revoke the program.

unsafe impl<T: Send> Send for Bucket2<T> {}

Thank you for checking out this issue 🦀

@Chopinsky
Copy link
Owner

Good catch! And well done with your PoC example, I like it. I think the suggested solution is reasonable, please feel free to post a PR fix, otherwise, I will work on this and try to get the fix in before next week.

@JOE1994
Copy link
Contributor Author

JOE1994 commented Jan 20, 2021

Would you mind publishing a new release that includes the fix to crates.io?

@Shnatsel
Copy link

Thank you for the swift fix!

Heads up: this issue has been submitted to the RustSec advisory database. It will be surfaced by tools such as cargo-audit or cargo-deny once merged.

I see the issue was fixed in git, but there seems to be no release incorporating the change. Could you publish a new release to crates.io?

Once it's published, please let me know and I'll include the fixed version in the advisory.

@JOE1994
Copy link
Contributor Author

JOE1994 commented Jan 29, 2021

@Chopinsky
Would you mind publishing a new release to crates.io? Thank you!

@Chopinsky
Copy link
Owner

Chopinsky commented Feb 1, 2021

Sorry that I was quite busy recently. I will release the fix today.

@JOE1994
Copy link
Contributor Author

JOE1994 commented Feb 2, 2021

@Chopinsky
Thank you for your response :)
FYI, I noted that a new release was published today for the byte_buffer crate and not the syncpool crate.
This issue is for the syncpool crate.

@Chopinsky
Copy link
Owner

yep, my bad, just published the new syncpool version as well :)

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.

3 participants