Send/Sync bound needed on T for Send/Sync impl of RcuCell<T>

[JOE1994]

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

• There is no Send bound on T of Send impl of RcuCell<T>
• There is no Sync bound on T of Sync impl of RcuCell<T>

unsafe impl<T> Send for RcuCell<T> {}
unsafe impl<T> Sync for RcuCell<T> {}

By exploiting this issue, it's possible to send non-Send items to other threads,
or share a non-Sync item concurrently across multiple threads.

Proof of Concept

You need to run the below program in Debug mode in order to observe undefined behavior.
In the program below, multiple threads clone & drop Rc(which is neither Send nor Sync).
Since Rc's internal strong_count is updated by multiple threads without synchronization,
the program will terminate in either one of the following states.

• strong_count > 1
  • program panics at the assertion check at the end (indicates memory leak in this case)
• strong_count == 0
  • Rc is dropped while references to it are still alive.
    When run on Ubuntu 18.04, program crashes with error: Illegal Instruction (Core Dumped)
• strong_count == 1
  • Not impossible, but highly unlikely

use rcu_cell::RcuCell;

use std::rc::Rc;
use std::sync::Arc;
use std::thread;

fn main() {
    // `Rc` is neither `Send` nor `Sync`
    let rcu_cell = RcuCell::new(Some(Rc::new(0_i32)));
    let arc_parent = Arc::new(rcu_cell);
    
    let mut child_threads = vec![];
    for _ in 0..5 {
        let arc_child = Arc::clone(&arc_parent);
        child_threads.push(thread::spawn(move || {
            for _ in 0..1000 {
                let reader = arc_child.as_ref().read();
                // data race on internal `strong_count` of `Rc`
                let _ = Rc::clone(&reader.unwrap());
            }
        }));
    }
    for child in child_threads {
        child.join().expect("failed to join child thread");
    }

    assert_eq!(Rc::strong_count(arc_parent.read().as_ref().unwrap()), 1);
}

How to fix

Adding trait bounds to Send/Sync impls of RcuCell as below would be one way to fix the issue.

unsafe impl<T: Send> Send for RcuCell<T> {}
unsafe impl<T: Sync> Sync for RcuCell<T> {}

Thank you for reviewing this issue! 🦀

[Shnatsel]

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.