fix windows join/detach and add tests

Author: beepster4096

src/machine.rs

@@ -418,6 +418,7 @@ impl<'mir, 'tcx> Evaluator<'mir, 'tcx> {
     ) -> InterpResult<'tcx> {
         EnvVars::init(this, config)?;
         Evaluator::init_extern_statics(this)?;
+        ThreadManager::init(this);
         Ok(())
     }
 

src/shims/time.rs

@@ -233,4 +233,30 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
 
         Ok(0)
     }
+
+    #[allow(non_snake_case)]
+    fn Sleep(&mut self, timeout: &OpTy<'tcx, Provenance>) -> InterpResult<'tcx> {
+        let this = self.eval_context_mut();
+
+        this.check_no_isolation("`Sleep`")?;
+
+        let timeout_ms = this.read_scalar(timeout)?.to_u32()?;
+
+        let duration = Duration::from_millis(timeout_ms.into());
+        let timeout_time = Time::Monotonic(Instant::now().checked_add(duration).unwrap());
+
+        let active_thread = this.get_active_thread();
+        this.block_thread(active_thread);
+
+        this.register_timeout_callback(
+            active_thread,
+            timeout_time,
+            Box::new(move |ecx| {
+                ecx.unblock_thread(active_thread);
+                Ok(())
+            }),
+        );
+
+        Ok(())
+    }
 }

src/shims/unix/thread.rs

@@ -37,7 +37,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
         }
 
         let thread_id = this.read_scalar(thread)?.to_machine_usize(this)?;
-        this.join_thread(thread_id.try_into().expect("thread ID should fit in u32"))?;
+        this.join_thread_exclusive(thread_id.try_into().expect("thread ID should fit in u32"))?;
 
         Ok(0)
     }
@@ -46,7 +46,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
         let this = self.eval_context_mut();
 
         let thread_id = this.read_scalar(thread)?.to_machine_usize(this)?;
-        this.detach_thread(thread_id.try_into().expect("thread ID should fit in u32"))?;
+        this.detach_thread(thread_id.try_into().expect("thread ID should fit in u32"), false)?;
 
         Ok(0)
     }

src/shims/windows/dlsym.rs

@@ -5,7 +5,7 @@ use rustc_target::spec::abi::Abi;
 use log::trace;
 
 use crate::helpers::check_arg_count;
-use crate::shims::windows::handle::Handle;
+use crate::shims::windows::handle::{EvalContextExt as _, Handle};
 use crate::*;
 
 #[derive(Debug, Copy, Clone)]
@@ -118,7 +118,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
                     match Handle::from_scalar(this.read_scalar(handle)?.check_init()?, this)? {
                         Some(Handle::Thread(thread)) => thread,
                         Some(Handle::CurrentThread) => this.get_active_thread(),
-                        _ => throw_ub_format!("invalid handle"),
+                        _ => this.invalid_handle("SetThreadDescription")?,
                     };
 
                 this.set_thread_name_wide(thread, name);

src/shims/windows/foreign_items.rs

@@ -228,24 +228,7 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
                 let [timeout] =
                     this.check_shim(abi, Abi::System { unwind: false }, link_name, args)?;
 
-                this.check_no_isolation("`Sleep`")?;
-
-                let timeout_ms = this.read_scalar(timeout)?.to_u32()?;
-
-                let duration = Duration::from_millis(timeout_ms as u64);
-                let timeout_time = Time::Monotonic(Instant::now().checked_add(duration).unwrap());
-
-                let active_thread = this.get_active_thread();
-                this.block_thread(active_thread);
-
-                this.register_timeout_callback(
-                    active_thread,
-                    timeout_time,
-                    Box::new(move |ecx| {
-                        ecx.unblock_thread(active_thread);
-                        Ok(())
-                    }),
-                );
+                this.Sleep(timeout)?;
             }
 
             // Synchronization primitives

src/shims/windows/handle.rs

@@ -146,16 +146,19 @@ impl<'mir, 'tcx> EvalContextExt<'mir, 'tcx> for crate::MiriEvalContext<'mir, 'tc
 
 #[allow(non_snake_case)]
 pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx> {
+    fn invalid_handle(&mut self, function_name: &str) -> InterpResult<'tcx, !> {
+        throw_machine_stop!(TerminationInfo::Abort(format!(
+            "invalid handle passed to `{function_name}`"
+        )))
+    }
+
     fn CloseHandle(&mut self, handle_op: &OpTy<'tcx, Provenance>) -> InterpResult<'tcx> {
         let this = self.eval_context_mut();
 
         match Handle::from_scalar(this.read_scalar(handle_op)?.check_init()?, this)? {
-            Some(Handle::Thread(thread)) => this.detach_thread(thread)?,
-            _ =>
-                throw_machine_stop!(TerminationInfo::Abort(
-                    "invalid handle passed to `CloseHandle`".into()
-                )),
-        };
+            Some(Handle::Thread(thread)) => this.detach_thread(thread, true)?,
+            _ => this.invalid_handle("CloseHandle")?,
+        }
 
         Ok(())
     }

src/shims/windows/thread.rs

@@ -1,11 +1,8 @@
-use std::time::{Duration, Instant};
-
 use rustc_middle::ty::layout::LayoutOf;
 use rustc_target::spec::abi::Abi;
 
-use crate::thread::Time;
 use crate::*;
-use shims::windows::handle::Handle;
+use shims::windows::handle::{EvalContextExt as _, Handle};
 
 impl<'mir, 'tcx: 'mir> EvalContextExt<'mir, 'tcx> for crate::MiriEvalContext<'mir, 'tcx> {}
 
@@ -59,25 +56,17 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
 
         let thread = match Handle::from_scalar(this.read_scalar(handle)?.check_init()?, this)? {
             Some(Handle::Thread(thread)) => thread,
-            Some(Handle::CurrentThread) => throw_ub_format!("trying to wait on itself"),
-            _ => throw_ub_format!("invalid handle"),
+            // Unlike on posix, joining the current thread is not UB on windows.
+            // It will just deadlock.
+            Some(Handle::CurrentThread) => this.get_active_thread(),
+            _ => this.invalid_handle("WaitForSingleObject")?,
         };
 
         if this.read_scalar(timeout)?.to_u32()? != this.eval_windows("c", "INFINITE")?.to_u32()? {
-            this.check_no_isolation("`WaitForSingleObject` with non-infinite timeout")?;
+            throw_unsup_format!("`WaitForSingleObject` with non-infinite timeout");
         }
 
-        let timeout_ms = this.read_scalar(timeout)?.to_u32()?;
-
-        let timeout_time = if timeout_ms == this.eval_windows("c", "INFINITE")?.to_u32()? {
-            None
-        } else {
-            let duration = Duration::from_millis(timeout_ms as u64);
-
-            Some(Time::Monotonic(Instant::now().checked_add(duration).unwrap()))
-        };
-
-        this.wait_on_thread(timeout_time, thread)?;
+        this.join_thread(thread)?;
 
         Ok(())
     }

src/thread.rs

@@ -405,6 +405,31 @@ impl<'mir, 'tcx: 'mir> ThreadManager<'mir, 'tcx> {
         Ok(())
     }
 
+    /// Mark that the active thread tries to exclusively join the thread with `joined_thread_id`.
+    /// If the thread is already joined by another thread
+    fn join_thread_exclusive(
+        &mut self,
+        joined_thread_id: ThreadId,
+        data_race: Option<&mut data_race::GlobalState>,
+    ) -> InterpResult<'tcx> {
+        if self.threads[joined_thread_id].join_status == ThreadJoinStatus::Joined {
+            throw_ub_format!("trying to join an already joined thread");
+        }
+
+        if joined_thread_id == self.active_thread {
+            throw_ub_format!("trying to join itself");
+        }
+
+        assert!(
+            self.threads
+                .iter()
+                .all(|thread| thread.state != ThreadState::BlockedOnJoin(joined_thread_id)),
+            "a joinable thread already has threads waiting for its termination"
+        );
+
+        self.join_thread(joined_thread_id, data_race)
+    }
+
     /// Set the name of the given thread.
     pub fn set_thread_name(&mut self, thread: ThreadId, new_thread_name: Vec<u8>) {
         self.threads[thread].thread_name = Some(new_thread_name);
@@ -700,6 +725,15 @@ pub trait EvalContextExt<'mir, 'tcx: 'mir>: crate::MiriEvalContextExt<'mir, 'tcx
         Ok(())
     }
 
+    #[inline]
+    fn join_thread_exclusive(&mut self, joined_thread_id: ThreadId) -> InterpResult<'tcx> {
+        let this = self.eval_context_mut();
+        this.machine
+            .threads
+            .join_thread_exclusive(joined_thread_id, this.machine.data_race.as_mut())?;
+        Ok(())
+    }
+
     #[inline]
     fn set_active_thread(&mut self, thread_id: ThreadId) -> ThreadId {
         let this = self.eval_context_mut();

tests/fail/concurrency/unwind_top_of_stack.rs

@@ -1,4 +1,4 @@
-//@ignore-windows: No libc on Windows
+//@ignore-target-windows: No libc on Windows
 
 //@compile-flags: -Zmiri-disable-abi-check
 

tests/fail/concurrency/windows_join_detached.rs

@@ -0,0 +1,21 @@
+//@only-target-windows: Uses win32 api functions
+//@error-pattern: Undefined Behavior: trying to join a detached thread
+
+// Joining a detached thread is undefined behavior.
+
+use std::os::windows::io::{AsRawHandle, RawHandle};
+use std::thread;
+
+extern "system" {
+    fn CloseHandle(handle: RawHandle) -> u32;
+}
+
+fn main() {
+    let thread = thread::spawn(|| ());
+
+    unsafe {
+        assert_ne!(CloseHandle(thread.as_raw_handle()), 0);
+    }
+
+    thread.join().unwrap();
+}

tests/fail/concurrency/windows_join_detached.stderr

@@ -0,0 +1,22 @@
+error: Undefined Behavior: trying to join a detached thread
+  --> RUSTLIB/std/src/sys/PLATFORM/thread.rs:LL:CC
+   |
+LL |         let rc = unsafe { c::WaitForSingleObject(self.handle.as_raw_handle(), c::INFINITE) };
+   |                           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ trying to join a detached thread
+   |
+   = 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: backtrace:
+   = note: inside `std::sys::PLATFORM::thread::Thread::join` at RUSTLIB/std/src/sys/PLATFORM/thread.rs:LL:CC
+   = note: inside `std::thread::JoinInner::<()>::join` at RUSTLIB/std/src/thread/mod.rs:LL:CC
+   = note: inside `std::thread::JoinHandle::<()>::join` at RUSTLIB/std/src/thread/mod.rs:LL:CC
+note: inside `main` at $DIR/windows_join_detached.rs:LL:CC
+  --> $DIR/windows_join_detached.rs:LL:CC
+   |
+LL |     thread.join().unwrap();
+   |     ^^^^^^^^^^^^^
+
+note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace
+
+error: aborting due to previous error
+

tests/fail/concurrency/windows_join_main.rs

@@ -0,0 +1,28 @@
+//@only-target-windows: Uses win32 api functions
+// We are making scheduler assumptions here.
+//@compile-flags: -Zmiri-preemption-rate=0
+
+// On windows, joining main is not UB, but it will block a thread forever.
+
+use std::thread;
+
+extern "system" {
+    fn WaitForSingleObject(handle: usize, timeout: u32) -> u32;
+}
+
+const INFINITE: u32 = u32::MAX;
+
+// This is how miri represents the handle for thread 0.
+// This value can be "legitimately" obtained by using `GetCurrentThread` with `DuplicateHandle`
+// but miri does not implement `DuplicateHandle` yet.
+const MAIN_THREAD: usize = 1 << 30;
+
+fn main() {
+    thread::spawn(|| {
+        unsafe {
+            assert_eq!(WaitForSingleObject(MAIN_THREAD, INFINITE), 0); //~ ERROR: deadlock: the evaluated program deadlocked
+        }
+    })
+    .join()
+    .unwrap();
+}

tests/fail/concurrency/windows_join_main.stderr

@@ -0,0 +1,12 @@
+error: deadlock: the evaluated program deadlocked
+  --> $DIR/windows_join_main.rs:LL:CC
+   |
+LL |         WaitForSingleObject(MAIN_THREAD, INFINITE);
+   |                                                   ^ the evaluated program deadlocked
+   |
+   = note: inside closure at $DIR/windows_join_main.rs:LL:CC
+
+note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace
+
+error: aborting due to previous error
+

tests/fail/concurrency/windows_join_self.rs

@@ -0,0 +1,25 @@
+//@only-target-windows: Uses win32 api functions
+// We are making scheduler assumptions here.
+//@compile-flags: -Zmiri-preemption-rate=0
+
+// On windows, a thread joining itself is not UB, but it will deadlock.
+
+use std::thread;
+
+extern "system" {
+    fn GetCurrentThread() -> usize;
+    fn WaitForSingleObject(handle: usize, timeout: u32) -> u32;
+}
+
+const INFINITE: u32 = u32::MAX;
+
+fn main() {
+    thread::spawn(|| {
+        unsafe {
+            let native = GetCurrentThread();
+            assert_eq!(WaitForSingleObject(native, INFINITE), 0); //~ ERROR: deadlock: the evaluated program deadlocked
+        }
+    })
+    .join()
+    .unwrap();
+}

tests/fail/concurrency/windows_join_self.stderr

@@ -0,0 +1,12 @@
+error: deadlock: the evaluated program deadlocked
+  --> $DIR/windows_join_self.rs:LL:CC
+   |
+LL |             assert_eq!(WaitForSingleObject(native, INFINITE), 0);
+   |                                                            ^ the evaluated program deadlocked
+   |
+   = note: inside closure at $DIR/windows_join_self.rs:LL:CC
+
+note: some details are omitted, run with `MIRIFLAGS=-Zmiri-backtrace=full` for a verbose backtrace
+
+error: aborting due to previous error
+

tests/pass/concurrency/windows_detach_terminated.rs

@@ -0,0 +1,21 @@
+//@only-target-windows: Uses win32 api functions
+// We are making scheduler assumptions here.
+//@compile-flags: -Zmiri-preemption-rate=0
+
+use std::os::windows::io::IntoRawHandle;
+use std::thread;
+
+extern "system" {
+    fn CloseHandle(handle: usize) -> i32;
+}
+
+fn main() {
+    let thread = thread::spawn(|| {}).into_raw_handle() as usize;
+
+    // this yield ensures that `thread` is terminated by this point
+    thread::yield_now();
+
+    unsafe {
+        assert_ne!(CloseHandle(thread), 0);
+    }
+}