Skip to content

Send for Iter is missing a Sync bound, so two threads can write the same value #246

Description

@fereidani

Hey, I've found this while scanning the top 5000 downloaded crates with my UB static analyzer.

src/lib.rs:1962:

unsafe impl<'a, K: Send, V: Send> Send for Iter<'a, K, V> {}

Iter hands out &K and &V, and it is Clone, so with only Send on K and V safe code can clone an iterator, move the clone to another thread, and get two shared references to the same value on two threads. If that type has interior mutability, that is a data race. Cell<u32> is Send and !Sync, which is all it takes.

use std::cell::Cell;
use std::num::NonZeroUsize;
use std::sync::{Arc, Barrier};
use std::thread;

#[derive(PartialEq, Eq)]
struct Key(Cell<u32>);

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

#[test]
fn two_threads_write_one_cell() {
    let mut cache = lru::LruCache::new(NonZeroUsize::new(1).unwrap());
    cache.put(Key(Cell::new(0)), 0u32);
    let cache: &'static lru::LruCache<Key, u32> = Box::leak(Box::new(cache));

    let mut left = cache.iter();
    let mut right = left.clone();

    let barrier = Arc::new(Barrier::new(2));
    let other = barrier.clone();

    let h = thread::spawn(move || {
        let (key, _) = left.next().unwrap();
        other.wait();
        key.0.set(1);
    });

    let (key, _) = right.next().unwrap();
    barrier.wait();
    key.0.set(2);
    h.join().unwrap();
}
$ cargo +nightly miri test

error: Undefined Behavior: Data race detected between (1) non-atomic write on thread `unnamed-2` and (2) retag write of type `u32` on thread `iter_send_lets_` at alloc41550+0x10
   --> library/core/src/cell.rs:516:31
    |
516 |         mem::replace(unsafe { &mut *self.value.get() }, val)
    |                               ^^^^^^^^^^^^^^^^^^^^^^ (2) just happened here
help: and (1) occurred earlier here
   --> tests/lru.rs:28:9

Fix: require K: Sync, V: Sync for Send for Iter, the same way the standard library's std::slice::Iter does. IterMut at src/lib.rs:2028 hands out &mut V, so Send is right there, but its Sync impl needs the same look.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions