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

5 files changed, 43 insertions, 18 deletions

diff --git a/src/libstd/sys/cloudabi/thread.rs b/src/libstd/sys/cloudabi/thread.rs
index 3afcae7ae75..a3595debaf5 100644
--- a/src/libstd/sys/cloudabi/thread.rs
+++ b/src/libstd/sys/cloudabi/thread.rs
@@ -22,7 +22,7 @@ unsafe impl Sync for Thread {}
 impl Thread {
     // unsafe: see thread::Builder::spawn_unchecked for safety requirements
     pub unsafe fn new(stack: usize, p: Box<dyn FnOnce()>) -> io::Result<Thread> {
-        let p = box p;
+        let mut p = mem::ManuallyDrop::new(box p);
         let mut native: libc::pthread_t = mem::zeroed();
         let mut attr: libc::pthread_attr_t = mem::zeroed();
         assert_eq!(libc::pthread_attr_init(&mut attr), 0);
@@ -30,13 +30,20 @@ impl Thread {
         let stack_size = cmp::max(stack, min_stack_size(&attr));
         assert_eq!(libc::pthread_attr_setstacksize(&mut attr, stack_size), 0);
 
-        let ret = libc::pthread_create(&mut native, &attr, thread_start, &*p as *const _ as *mut _);
+        let ret = libc::pthread_create(
+            &mut native,
+            &attr,
+            thread_start,
+            &mut *p as &mut Box<dyn FnOnce()> as *mut _ as *mut _,
+        );
         assert_eq!(libc::pthread_attr_destroy(&mut attr), 0);
 
         return if ret != 0 {
+            // The thread failed to start and as a result p was not consumed. Therefore, it is
+            // safe to manually drop it.
+            mem::ManuallyDrop::drop(&mut p);
             Err(io::Error::from_raw_os_error(ret))
         } else {
-            mem::forget(p); // ownership passed to pthread_create
             Ok(Thread { id: native })
         };
 
diff --git a/src/libstd/sys/hermit/thread.rs b/src/libstd/sys/hermit/thread.rs
index c3c29c93826..8e15208abc2 100644
--- a/src/libstd/sys/hermit/thread.rs
+++ b/src/libstd/sys/hermit/thread.rs
@@ -49,21 +49,23 @@ impl Thread {
         p: Box<dyn FnOnce()>,
         core_id: isize,
     ) -> io::Result<Thread> {
-        let p = box p;
+        let mut p = mem::ManuallyDrop::new(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,
+            &mut *p as &mut Box<dyn FnOnce()> as *mut _ as *mut u8 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 manually drop it.
+            mem::ManuallyDrop::drop(&mut p);
             Err(io::Error::new(io::ErrorKind::Other, "Unable to create thread!"))
+        } else {
+            Ok(Thread { tid: tid })
         };
 
         extern "C" fn thread_start(main: usize) {
diff --git a/src/libstd/sys/unix/thread.rs b/src/libstd/sys/unix/thread.rs
index 674d4c71138..efcd6142024 100644
--- a/src/libstd/sys/unix/thread.rs
+++ b/src/libstd/sys/unix/thread.rs
@@ -43,7 +43,7 @@ unsafe fn pthread_attr_setstacksize(
 impl Thread {
     // unsafe: see thread::Builder::spawn_unchecked for safety requirements
     pub unsafe fn new(stack: usize, p: Box<dyn FnOnce()>) -> io::Result<Thread> {
-        let p = box p;
+        let mut p = mem::ManuallyDrop::new(box p);
         let mut native: libc::pthread_t = mem::zeroed();
         let mut attr: libc::pthread_attr_t = mem::zeroed();
         assert_eq!(libc::pthread_attr_init(&mut attr), 0);
@@ -65,13 +65,20 @@ impl Thread {
             }
         };
 
-        let ret = libc::pthread_create(&mut native, &attr, thread_start, &*p as *const _ as *mut _);
+        let ret = libc::pthread_create(
+            &mut native,
+            &attr,
+            thread_start,
+            &mut *p as &mut Box<dyn FnOnce()> as *mut _ as *mut _,
+        );
         assert_eq!(libc::pthread_attr_destroy(&mut attr), 0);
 
         return if ret != 0 {
+            // The thread failed to start and as a result p was not consumed. Therefore, it is
+            // safe to manually drop it.
+            mem::ManuallyDrop::drop(&mut p);
             Err(io::Error::from_raw_os_error(ret))
         } else {
-            mem::forget(p); // ownership passed to pthread_create
             Ok(Thread { id: native })
         };
 
diff --git a/src/libstd/sys/vxworks/thread.rs b/src/libstd/sys/vxworks/thread.rs
index e0d104b5f3e..81233c1975c 100644
--- a/src/libstd/sys/vxworks/thread.rs
+++ b/src/libstd/sys/vxworks/thread.rs
@@ -31,7 +31,7 @@ unsafe fn pthread_attr_setstacksize(
 impl Thread {
     // unsafe: see thread::Builder::spawn_unchecked for safety requirements
     pub unsafe fn new(stack: usize, p: Box<dyn FnOnce()>) -> io::Result<Thread> {
-        let p = box p;
+        let mut p = mem::ManuallyDrop::new(box p);
         let mut native: libc::pthread_t = mem::zeroed();
         let mut attr: libc::pthread_attr_t = mem::zeroed();
         assert_eq!(libc::pthread_attr_init(&mut attr), 0);
@@ -53,13 +53,20 @@ impl Thread {
             }
         };
 
-        let ret = libc::pthread_create(&mut native, &attr, thread_start, &*p as *const _ as *mut _);
+        let ret = libc::pthread_create(
+            &mut native,
+            &attr,
+            thread_start,
+            &mut *p as &mut Box<dyn FnOnce()> as *mut _ as *mut _,
+        );
         assert_eq!(libc::pthread_attr_destroy(&mut attr), 0);
 
         return if ret != 0 {
+            // The thread failed to start and as a result p was not consumed. Therefore, it is
+            // safe to manually drop it.
+            mem::ManuallyDrop::drop(&mut p);
             Err(io::Error::from_raw_os_error(ret))
         } else {
-            mem::forget(p); // ownership passed to pthread_create
             Ok(Thread { id: native })
         };
 
diff --git a/src/libstd/sys/windows/thread.rs b/src/libstd/sys/windows/thread.rs
index c828243a59b..052f51a33ce 100644
--- a/src/libstd/sys/windows/thread.rs
+++ b/src/libstd/sys/windows/thread.rs
@@ -20,7 +20,7 @@ pub struct Thread {
 impl Thread {
     // unsafe: see thread::Builder::spawn_unchecked for safety requirements
     pub unsafe fn new(stack: usize, p: Box<dyn FnOnce()>) -> io::Result<Thread> {
-        let p = box p;
+        let mut p = mem::ManuallyDrop::new(box p);
 
         // FIXME On UNIX, we guard against stack sizes that are too small but
         // that's because pthreads enforces that stacks are at least
@@ -34,15 +34,17 @@ impl Thread {
             ptr::null_mut(),
             stack_size,
             thread_start,
-            &*p as *const _ as *mut _,
+            &mut *p as &mut Box<dyn FnOnce()> as *mut _ as *mut _,
             c::STACK_SIZE_PARAM_IS_A_RESERVATION,
             ptr::null_mut(),
         );
 
         return if ret as usize == 0 {
+            // The thread failed to start and as a result p was not consumed. Therefore, it is
+            // safe to manually drop it.
+            mem::ManuallyDrop::drop(&mut p);
             Err(io::Error::last_os_error())
         } else {
-            mem::forget(p); // ownership passed to CreateThread
             Ok(Thread { handle: Handle::new(ret) })
         };