Rollup merge of #70597 - vakaras:thread_new_double_free_bug_fix, r=Amanieu

Fix double-free and undefined behaviour in libstd::syn::unix::Thread::new

While working on concurrency support for Miri, I found that the `libstd::syn::unix::Thread::new` method has two potential problems: double-free and undefined behaviour.

**Double-free** could occur if the following events happened (credit for pointing this out goes to @RalfJung):

1.  The call to `pthread_create` successfully launched a new thread that executed to completion and deallocated `p`.
2.  The call to `pthread_attr_destroy` returned a non-zero value causing the `assert_eq!` to panic.
3.  Since `mem::forget(p)` was not yet executed, the destructor of `p` would be executed and cause a double-free.

As far as I understand, this code also violates the stacked-borrows aliasing rules and thus would result in **undefined behaviour** if these rules were adopted.  The problem is that the ownership of `p` is passed to the newly created thread before the call to `mem::forget`. Since the call to `mem::forget` is still a call, it counts as a use of `p` and triggers UB.

This pull request changes the code to use `mem::ManuallyDrop` instead of `mem::forget`. As a consequence, in case of a panic, `p` would be potentially leaked, which while undesirable is probably better than double-free or undefined behaviour.

2 files changed, 10 insertions, 17 deletions

diff --git a/src/libstd/sys/hermit/stack_overflow.rs b/src/libstd/sys/hermit/stack_overflow.rs
index 65a1b17acce..121fe42011d 100644
--- a/src/libstd/sys/hermit/stack_overflow.rs
+++ b/src/libstd/sys/hermit/stack_overflow.rs
@@ -1,11 +1,3 @@
-pub struct Handler;
-
-impl Handler {
-    pub unsafe fn new() -> Handler {
-        Handler
-    }
-}
-
 #[inline]
 pub unsafe fn init() {}
 
diff --git a/src/libstd/sys/hermit/thread.rs b/src/libstd/sys/hermit/thread.rs
index c3c29c93826..c7bea168f34 100644
--- a/src/libstd/sys/hermit/thread.rs
+++ b/src/libstd/sys/hermit/thread.rs
@@ -8,8 +8,6 @@ use crate::sys::hermit::abi;
 use crate::time::Duration;
 use core::u32;
 
-use crate::sys_common::thread::*;
-
 pub type Tid = abi::Tid;
 
 /// Priority of a task
@@ -49,26 +47,29 @@ impl Thread {
         p: Box<dyn FnOnce()>,
         core_id: isize,
     ) -> io::Result<Thread> {
-        let p = box p;
+        let p = Box::into_raw(box p);
         let mut tid: Tid = u32::MAX;
         let ret = abi::spawn(
             &mut tid as *mut Tid,
             thread_start,
-            &*p as *const _ as *const u8 as usize,
+            p as usize,
             Priority::into(NORMAL_PRIO),
             core_id,
         );
 
-        return if ret == 0 {
-            mem::forget(p); // ownership passed to pthread_create
-            Ok(Thread { tid: tid })
-        } else {
+        return if ret != 0 {
+            // The thread failed to start and as a result p was not consumed. Therefore, it is
+            // safe to reconstruct the box so that it gets deallocated.
+            drop(Box::from_raw(p));
             Err(io::Error::new(io::ErrorKind::Other, "Unable to create thread!"))
+        } else {
+            Ok(Thread { tid: tid })
         };
 
         extern "C" fn thread_start(main: usize) {
             unsafe {
-                start_thread(main as *mut u8);
+                // Finally, let's run some code.
+                Box::from_raw(main as *mut Box<dyn FnOnce()>)();
             }
         }
     }