Data races possible from safe code on LocalObjects type

[Ollie-Pearce]

A HistoryBox struct containing a Cell and an UnsafeCell is wrapped in an Arc and provided as the sole field of LocalObjects.

LocalObjects has a manual Sync implementation which allows for the interior mutable Cell/UnsafeCell to be shared across threads.

Data races on the Cell and UnsafeCell fields of HistoryBox can be triggered if insert() and contains() are invoked concurrently.

#[derive(Default, Clone)]
pub struct LocalObjects {
    inner: Arc<Inner>,
}

#[derive(Default)]
struct Inner {
    map: HistoryBox<BTreeMap<String, HistoryBox<Box<dyn Any>>>>,
}

// ...

impl LocalObjects {

    pub fn insert<T: 'static>(&self, key: impl Into<String>, val: T) {
        let key = key.into();
        let contains = self.map_immut().contains_key(&key);
        if contains {
            self.map_mut().get_mut(&key).unwrap().set(Box::new(val));
        } else {
            self.map_mut().insert(key, HistoryBox::new_with(Box::new(val)));
        }
    }

    pub fn contains(&self, key: &str) -> bool {
        self.map_immut().contains_key(key)
    }

}

// ...

unsafe impl Send for LocalObjects { }
unsafe impl Sync for LocalObjects { }

Reproducing:

Test case:

#[test]
fn race_LocalObjects_insert_vs_contains() {
    let foo: LocalObjects = LocalObjects::new();
    std::thread::scope(|s| {
        s.spawn(|| {
            foo.insert("key", 1u32);
        });
        let _ = foo.contains("key");
    });
}

Verifying with Miri:

MIRIFLAGS="-Zmiri-many-seeds=0..16 -Zmiri-disable-stacked-borrows" cargo +nightly miri test -p teo-runtime --test repro

Output:

error: Undefined Behavior: Data race detected between (1) non-atomic read on thread `race_LocalObjec` and (2) non-atomic write on thread `unnamed-2` at alloc625513
     |
1722 |         *self = Some(value);
     |         ^^^^^ (2) just happened here
     |
help: and (1) occurred earlier here
    --> src/request/local_objects.rs:56:9
     |
  56 |         self.map_immut().contains_key(key)
     |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
     = help: this indicates a bug in the program: it performed an invalid operation, and caused Undefined Behavior
     = help: see https://doc.rust-lang.org/nightly/reference/behavior-considered-undefined.html for further information
     = note: this is on thread `unnamed-2`

Fix:

• If LocalObjects does not need to be shared between threads I would recommend removing the Sync implementation.