jonhoo / left-right

A lock-free, read-optimized, concurrency primitive.
Apache License 2.0
1.95k stars 94 forks source link

data races due to incorrect `Send + Sync` bounds #75

Closed RustyYato closed 3 years ago

RustyYato commented 3 years ago

evmap let's you trivially ignore Send and Sync requirements on types, which allow for horrors such as:

use std::cell::Cell;
use std::hash::Hash;
use std::sync::{Arc, Barrier};

pub struct Foo(Cell<i32>);

impl Hash for Foo {
    fn hash<H: std::hash::Hasher>(&self, hasher: &mut H) {}
}

impl Eq for Foo {}
impl PartialEq for Foo {
    fn eq(&self, _: &Self) -> bool {
        true
    }
}

fn main() {
    let (x, mut y) = evmap::new();
    let barrier = Arc::new(Barrier::new(2));

    let foo = std::rc::Rc::new(Foo(Cell::new(0)));
    y.insert((), foo.clone());
    y.refresh();

    let j = std::thread::spawn({
        let barrier = barrier.clone();
        move || {
            let read = x.get(&()).unwrap();
            let value = read.get_one().unwrap();
            let x = std::rc::Rc::clone(value);

            barrier.wait();
            for _ in 0..1000 {
                x.0.set(x.0.get() + 1);
            }
        }
    });

    barrier.wait();
    for _ in 0..1000 {
        foo.0.set(foo.0.get() + 1);
    }

    j.join().unwrap();

    println!("{}", foo.0.get());
}

This compile and runs, and is very much UB with data races. On my machine it has given the "correct" result of 2000, but also 1257, 1095, and 1068. Which is some more proof of UB.

This bug is in evmap version 10^ and the current master branch (commit hash: d4c12b8758c7e35145c40856f3d3474befe6ecda).

RustyYato commented 3 years ago

The no-op hash and eq are just for demonstration, and bear no relevance to the bug.

jonhoo commented 3 years ago

Ooof, yes, that's definitely not good! Will need to fix this for 11.0 (which will be based on the modifications in #73). I'm pretty swamped at the moment, so will not be able to fix this up any time very soon for 10 unfortunately. But getting 11.0 out is high on my list of priorities.

RustyYato commented 3 years ago

No problem, I caught your stream about evmap and looked into this, and found this. Looks like a simple oversight, but overall this crate is great! Thanks for the great work.