Soundness issue in MvccRwLock

[ammaraskar]

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

noise/src/index.rs

Lines 418 to 419 in 5a582a7

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

However, this should probably have tighter bounds on its Send and Sync traits, otherwise its possible to create data-races from safe rust code by using non-Sync types like Cell across threads or sending non-Send types across like Rc. Here's a little proof-of-concept using Rc.

#![forbid(unsafe_code)]

use noise_search::index::MvccRwLock;

use std::rc::Rc;

fn main() {
    let rc = Rc::new(42);

    let lock = MvccRwLock::new(rc.clone());
    std::thread::spawn(move || {
        let smuggled_rc = lock.read();

        println!("Thread: {:p}", *smuggled_rc);
        for _ in 0..100_000_000 {
            (*smuggled_rc).clone();
        }
    });

    println!("Main:   {:p}", rc);
    for _ in 0..100_000_000 {
        rc.clone();
    }
}

This outputs:

Main:   0x561539bdcd40
Thread: 0x561539bdcd40

Terminated with signal 4 (SIGILL)

It seems like this class also potentially allows for aliasing violations, in this case maybe it would be better to mark the methods as unsafe and maybe not expose the class outside the crate?

[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.