More tests, fix issue 1643 and detect races with allocation.

Author: JCTyblaidd

src/data_race.rs

@@ -192,6 +192,25 @@ struct AtomicMemoryCellClocks {
     sync_vector: VClock,
 }
 
+#[derive(Copy, Clone, PartialEq, Eq, Debug)]
+enum WriteType {
+    /// Allocate memory.
+    Allocate,
+    /// Standard unsynchronized write.
+    Write,
+    /// Deallocate memory
+    Deallocate,
+}
+impl WriteType {
+    fn get_descriptor(self) -> &'static str {
+        match self {
+            WriteType::Allocate => "ALLOCATE",
+            WriteType::Write => "WRITE",
+            WriteType::Deallocate => "DEALLOCATE",
+        }
+    }
+}
+
 /// Memory Cell vector clock metadata
 /// for data-race detection.
 #[derive(Clone, PartialEq, Eq, Debug)]
@@ -204,6 +223,11 @@ struct MemoryCellClocks {
     /// that performed the last write operation.
     write_index: VectorIdx,
 
+    /// The type of operation that the write index represents,
+    /// either newly allocated memory, a non-atomic write or
+    /// a deallocation of memory.
+    write_type: WriteType,
+
     /// The vector-clock of the timestamp of the last read operation
     /// performed by a thread since the last write operation occurred.
     /// It is reset to zero on each write operation.
@@ -215,20 +239,19 @@ struct MemoryCellClocks {
     atomic_ops: Option<Box<AtomicMemoryCellClocks>>,
 }
 
-/// Create a default memory cell clocks instance
-/// for uninitialized memory.
-impl Default for MemoryCellClocks {
-    fn default() -> Self {
+impl MemoryCellClocks {
+
+    /// Create a new set of clocks representing memory allocated
+    ///  at a given vector timestamp and index.
+    fn new(alloc: VTimestamp, alloc_index: VectorIdx) -> Self {
         MemoryCellClocks {
             read: VClock::default(),
-            write: 0,
-            write_index: VectorIdx::MAX_INDEX,
+            write: alloc,
+            write_index: alloc_index,
+            write_type: WriteType::Allocate,
             atomic_ops: None,
         }
     }
-}
-
-impl MemoryCellClocks {
 
     /// Load the internal atomic memory cells if they exist.
     #[inline]
@@ -382,6 +405,7 @@ impl MemoryCellClocks {
         &mut self,
         clocks: &ThreadClockSet,
         index: VectorIdx,
+        write_type: WriteType,
     ) -> Result<(), DataRace> {
         log::trace!("Unsynchronized write with vectors: {:#?} :: {:#?}", self, clocks);
         if self.write <= clocks.clock[self.write_index] && self.read <= clocks.clock {
@@ -393,6 +417,7 @@ impl MemoryCellClocks {
             if race_free {
                 self.write = clocks.clock[index];
                 self.write_index = index;
+                self.write_type = write_type;
                 self.read.set_zero_vector();
                 Ok(())
             } else {
@@ -646,16 +671,28 @@ pub struct VClockAlloc {
     /// Assigning each byte a MemoryCellClocks.
     alloc_ranges: RefCell<RangeMap<MemoryCellClocks>>,
 
-    // Pointer to global state.
+    /// Pointer to global state.
     global: MemoryExtra,
 }
 
 impl VClockAlloc {
-    /// Create a new data-race allocation detector.
-    pub fn new_allocation(global: &MemoryExtra, len: Size) -> VClockAlloc {
+
+    /// Create a new data-race detector for newly allocated memory.
+    pub fn new_allocation(global: &MemoryExtra, len: Size, track_alloc: bool) -> VClockAlloc {
+        //FIXME: stack allocations are currently ignored due to the lazy nature of stack
+        // allocation, this results in data-races being missed.
+        let (alloc_timestamp, alloc_index) = if !track_alloc {
+            (0, VectorIdx::MAX_INDEX)
+        }else{
+            let (alloc_index, clocks) = global.current_thread_state();
+            let alloc_timestamp = clocks.clock[alloc_index];
+            (alloc_timestamp, alloc_index)
+        };
         VClockAlloc {
             global: Rc::clone(global),
-            alloc_ranges: RefCell::new(RangeMap::new(len, MemoryCellClocks::default())),
+            alloc_ranges: RefCell::new(RangeMap::new(
+                len, MemoryCellClocks::new(alloc_timestamp, alloc_index)
+            )),
         }
     }
 
@@ -712,7 +749,7 @@ impl VClockAlloc {
             // Convert the write action into the vector clock it
             // represents for diagnostic purposes.
             write_clock = VClock::new_with_index(range.write_index, range.write);
-            ("WRITE", range.write_index, &write_clock)
+            (range.write_type.get_descriptor(), range.write_index, &write_clock)
         } else if let Some(idx) = Self::find_gt_index(&range.read, &current_clocks.clock) {
             ("READ", idx, &range.read)
         } else if !is_atomic {
@@ -792,17 +829,17 @@ impl VClockAlloc {
         &mut self,
         pointer: Pointer<Tag>,
         len: Size,
-        action: &str,
+        write_type: WriteType,
     ) -> InterpResult<'tcx> {
         if self.global.multi_threaded.get() {
             let (index, clocks) = self.global.current_thread_state();
             for (_, range) in self.alloc_ranges.get_mut().iter_mut(pointer.offset, len) {
-                if let Err(DataRace) = range.write_race_detect(&*clocks, index) {
+                if let Err(DataRace) = range.write_race_detect(&*clocks, index, write_type) {
                     // Report data-race
                     return Self::report_data_race(
                         &self.global,
                         range,
-                        action,
+                        write_type.get_descriptor(),
                         false,
                         pointer,
                         len,
@@ -820,15 +857,15 @@ impl VClockAlloc {
     /// being created or if it is temporarily disabled during a racy read or write
     /// operation
     pub fn write<'tcx>(&mut self, pointer: Pointer<Tag>, len: Size) -> InterpResult<'tcx> {
-        self.unique_access(pointer, len, "Write")
+        self.unique_access(pointer, len, WriteType::Write)
     }
 
     /// Detect data-races for an unsynchronized deallocate operation, will not perform
     /// data-race threads if `multi-threaded` is false, either due to no threads
     /// being created or if it is temporarily disabled during a racy read or write
     /// operation
     pub fn deallocate<'tcx>(&mut self, pointer: Pointer<Tag>, len: Size) -> InterpResult<'tcx> {
-        self.unique_access(pointer, len, "Deallocate")
+        self.unique_access(pointer, len, WriteType::Deallocate)
     }
 }
 

src/machine.rs

@@ -478,7 +478,24 @@ impl<'mir, 'tcx> Machine<'mir, 'tcx> for Evaluator<'mir, 'tcx> {
                 (None, Tag::Untagged)
             };
         let race_alloc = if let Some(data_race) = &memory_extra.data_race {
-            Some(data_race::AllocExtra::new_allocation(&data_race, alloc.size))
+            match kind {
+                // V-Table generation is lazy and so racy, so do not track races.
+                // Also V-Tables are read only so no data races can be detected.
+                MemoryKind::Vtable => None,
+                // User allocated and stack memory should track allocation.
+                MemoryKind::Machine(
+                    MiriMemoryKind::Rust | MiriMemoryKind::C | MiriMemoryKind::WinHeap
+                ) | MemoryKind::Stack => Some(
+                    data_race::AllocExtra::new_allocation(&data_race, alloc.size, true)
+                ),
+                // Other global memory should trace races but be allocated at the 0 timestamp.
+                MemoryKind::Machine(
+                    MiriMemoryKind::Global | MiriMemoryKind::Machine | MiriMemoryKind::Env |
+                    MiriMemoryKind::ExternStatic | MiriMemoryKind::Tls
+                ) | MemoryKind::CallerLocation => Some(
+                    data_race::AllocExtra::new_allocation(&data_race, alloc.size, false)
+                )
+            }
         } else {
             None
         };

tests/compile-fail/data_race/alloc_read_race.rs

@@ -0,0 +1,45 @@
+// ignore-windows: Concurrency on Windows is not supported yet.
+
+use std::thread::spawn;
+use std::ptr::null_mut;
+use std::sync::atomic::{Ordering, AtomicPtr};
+
+#[derive(Copy, Clone)]
+struct EvilSend<T>(pub T);
+
+unsafe impl<T> Send for EvilSend<T> {}
+unsafe impl<T> Sync for EvilSend<T> {}
+
+pub fn main() {
+    // Shared atomic pointer
+    let pointer = AtomicPtr::new(null_mut::<usize>());
+    let ptr = EvilSend(&pointer as *const AtomicPtr<usize>);
+
+    // Note: this is scheduler-dependent
+    // the operations need to occur in
+    // order, otherwise the allocation is
+    // not visible to the other-thread to
+    // detect the race:
+    //  1. alloc
+    //  2. write
+    unsafe {
+        let j1 = spawn(move || {
+            //Concurrent allocate the memory.
+            //Uses relaxed semantics to not generate
+            //a release sequence.
+            let pointer = &*ptr.0;
+            pointer.store(Box::into_raw(Box::new(0usize)), Ordering::Relaxed);
+        });
+
+        let j2 = spawn(move || {
+            let pointer = &*ptr.0;
+            *pointer.load(Ordering::Relaxed) //~ ERROR Data race
+        });
+
+        j1.join().unwrap();
+        j2.join().unwrap();
+
+        // Clean up memory, will never be executed
+        drop(Box::from_raw(pointer.load(Ordering::Relaxed)));
+    }
+}

tests/compile-fail/data_race/alloc_write_race.rs

@@ -0,0 +1,45 @@
+// ignore-windows: Concurrency on Windows is not supported yet.
+
+use std::thread::spawn;
+use std::ptr::null_mut;
+use std::sync::atomic::{Ordering, AtomicPtr};
+
+#[derive(Copy, Clone)]
+struct EvilSend<T>(pub T);
+
+unsafe impl<T> Send for EvilSend<T> {}
+unsafe impl<T> Sync for EvilSend<T> {}
+
+pub fn main() {
+    // Shared atomic pointer
+    let pointer = AtomicPtr::new(null_mut::<usize>());
+    let ptr = EvilSend(&pointer as *const AtomicPtr<usize>);
+
+    // Note: this is scheduler-dependent
+    // the operations need to occur in
+    // order, otherwise the allocation is
+    // not visible to the other-thread to
+    // detect the race:
+    //  1. alloc
+    //  2. write
+    unsafe {
+        let j1 = spawn(move || {
+            //Concurrent allocate the memory.
+            //Uses relaxed semantics to not generate
+            //a release sequence.
+            let pointer = &*ptr.0;
+            pointer.store(Box::into_raw(Box::new(0usize)), Ordering::Relaxed);
+        });
+
+        let j2 = spawn(move || {
+            let pointer = &*ptr.0;
+            *pointer.load(Ordering::Relaxed) = 2; //~ ERROR Data race
+        });
+
+        j1.join().unwrap();
+        j2.join().unwrap();
+
+        // Clean up memory, will never be executed
+        drop(Box::from_raw(pointer.load(Ordering::Relaxed)));
+    }
+}

tests/compile-fail/data_race/dealloc_read_race.rs

@@ -0,0 +1,32 @@
+// ignore-windows: Concurrency on Windows is not supported yet.
+
+use std::thread::spawn;
+
+#[derive(Copy, Clone)]
+struct EvilSend<T>(pub T);
+
+unsafe impl<T> Send for EvilSend<T> {}
+unsafe impl<T> Sync for EvilSend<T> {}
+
+extern "Rust" {
+    fn __rust_dealloc(ptr: *mut u8, size: usize, align: usize);
+}
+
+pub fn main() {
+    // Shared atomic pointer
+    let pointer: *mut usize = Box::into_raw(Box::new(0usize));
+    let ptr = EvilSend(pointer);
+
+    unsafe {
+        let j1 = spawn(move || {
+            *ptr.0
+        });
+
+        let j2 = spawn(move || {
+            __rust_dealloc(ptr.0 as *mut _, std::mem::size_of::<usize>(), std::mem::align_of::<usize>());  //~ ERROR Data race
+        });
+
+        j1.join().unwrap();
+        j2.join().unwrap();
+    }
+}

tests/compile-fail/data_race/dealloc_read_race_stack.rs

@@ -0,0 +1,52 @@
+// ignore-windows: Concurrency on Windows is not supported yet.
+// compile-flags: -Zmiri-disable-isolation
+
+use std::thread::{spawn, sleep};
+use std::ptr::null_mut;
+use std::sync::atomic::{Ordering, AtomicPtr};
+use std::time::Duration;
+
+#[derive(Copy, Clone)]
+struct EvilSend<T>(pub T);
+
+unsafe impl<T> Send for EvilSend<T> {}
+unsafe impl<T> Sync for EvilSend<T> {}
+
+pub fn main() {
+    // Shared atomic pointer
+    let pointer = AtomicPtr::new(null_mut::<usize>());
+    let ptr = EvilSend(&pointer as *const AtomicPtr<usize>);
+
+    // Note: this is scheduler-dependent
+    // the operations need to occur in
+    // order, otherwise the allocation is
+    // not visible to the other-thread to
+    // detect the race:
+    //  1. stack-allocate
+    //  2. read
+    //  3. stack-deallocate
+    unsafe {
+        let j1 = spawn(move || {
+            //Concurrent allocate the memory.
+            //Uses relaxed semantics to not generate
+            //a release sequence.
+            let pointer = &*ptr.0;
+            {
+                let mut stack_var = 0usize;
+
+                pointer.store(&mut stack_var as *mut _, Ordering::Release);
+
+                sleep(Duration::from_millis(100));
+
+            } //~ ERROR Data race
+        });
+
+        let j2 = spawn(move || {
+            let pointer = &*ptr.0;
+            *pointer.load(Ordering::Acquire)
+        });
+
+        j1.join().unwrap();
+        j2.join().unwrap();
+    }
+}