From 64e5327b6e7ad79f4a3ca7de17ac105c8c59277e Mon Sep 17 00:00:00 2001 From: Vytautas Astrauskas Date: Mon, 30 Mar 2020 20:55:17 -0700 Subject: Fix double-free and undefined behaviour in libstd::syn::unix::Thread::new. --- src/libstd/sys/windows/thread.rs | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) (limited to 'src/libstd/sys/windows') 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) -> io::Result { - 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 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) }) }; -- cgit 1.4.1-3-g733a5 From 753bc7ddf8a0f00acf039731947a12d06ad30884 Mon Sep 17 00:00:00 2001 From: Vytautas Astrauskas Date: Tue, 31 Mar 2020 12:35:07 -0700 Subject: Inline start_thread into its callers. --- src/libstd/sys/cloudabi/thread.rs | 8 ++++++-- src/libstd/sys/hermit/thread.rs | 9 ++++++--- src/libstd/sys/unix/thread.rs | 10 ++++++---- src/libstd/sys/vxworks/thread.rs | 10 ++++++---- src/libstd/sys/windows/thread.rs | 8 ++++++-- src/libstd/sys_common/thread.rs | 11 ----------- 6 files changed, 30 insertions(+), 26 deletions(-) (limited to 'src/libstd/sys/windows') diff --git a/src/libstd/sys/cloudabi/thread.rs b/src/libstd/sys/cloudabi/thread.rs index a3595debaf5..112bb1ce3af 100644 --- a/src/libstd/sys/cloudabi/thread.rs +++ b/src/libstd/sys/cloudabi/thread.rs @@ -4,8 +4,8 @@ use crate::io; use crate::mem; use crate::ptr; use crate::sys::cloudabi::abi; +use crate::sys::stack_overflow; use crate::sys::time::checked_dur2intervals; -use crate::sys_common::thread::*; use crate::time::Duration; pub const DEFAULT_MIN_STACK_SIZE: usize = 2 * 1024 * 1024; @@ -49,7 +49,11 @@ impl Thread { extern "C" fn thread_start(main: *mut libc::c_void) -> *mut libc::c_void { unsafe { - start_thread(main as *mut u8); + // Next, set up our stack overflow handler which may get triggered if we run + // out of stack. + let _handler = stack_overflow::Handler::new(); + // Finally, let's run some code. + Box::from_raw(main as *mut Box)(); } ptr::null_mut() } diff --git a/src/libstd/sys/hermit/thread.rs b/src/libstd/sys/hermit/thread.rs index 8e15208abc2..f92c18a3a45 100644 --- a/src/libstd/sys/hermit/thread.rs +++ b/src/libstd/sys/hermit/thread.rs @@ -5,11 +5,10 @@ use crate::fmt; use crate::io; use crate::mem; use crate::sys::hermit::abi; +use crate::sys::stack_overflow; use crate::time::Duration; use core::u32; -use crate::sys_common::thread::*; - pub type Tid = abi::Tid; /// Priority of a task @@ -70,7 +69,11 @@ impl Thread { extern "C" fn thread_start(main: usize) { unsafe { - start_thread(main as *mut u8); + // Next, set up our stack overflow handler which may get triggered if we run + // out of stack. + let _handler = stack_overflow::Handler::new(); + // Finally, let's run some code. + Box::from_raw(main as *mut Box)(); } } } diff --git a/src/libstd/sys/unix/thread.rs b/src/libstd/sys/unix/thread.rs index efcd6142024..513a1fde331 100644 --- a/src/libstd/sys/unix/thread.rs +++ b/src/libstd/sys/unix/thread.rs @@ -3,11 +3,9 @@ use crate::ffi::CStr; use crate::io; use crate::mem; use crate::ptr; -use crate::sys::os; +use crate::sys::{os, stack_overflow}; use crate::time::Duration; -use crate::sys_common::thread::*; - #[cfg(not(target_os = "l4re"))] pub const DEFAULT_MIN_STACK_SIZE: usize = 2 * 1024 * 1024; #[cfg(target_os = "l4re")] @@ -84,7 +82,11 @@ impl Thread { extern "C" fn thread_start(main: *mut libc::c_void) -> *mut libc::c_void { unsafe { - start_thread(main as *mut u8); + // Next, set up our stack overflow handler which may get triggered if we run + // out of stack. + let _handler = stack_overflow::Handler::new(); + // Finally, let's run some code. + Box::from_raw(main as *mut Box)(); } ptr::null_mut() } diff --git a/src/libstd/sys/vxworks/thread.rs b/src/libstd/sys/vxworks/thread.rs index 81233c1975c..92163865b79 100644 --- a/src/libstd/sys/vxworks/thread.rs +++ b/src/libstd/sys/vxworks/thread.rs @@ -3,11 +3,9 @@ use crate::ffi::CStr; use crate::io; use crate::mem; use crate::ptr; -use crate::sys::os; +use crate::sys::{os, stack_overflow}; use crate::time::Duration; -use crate::sys_common::thread::*; - pub const DEFAULT_MIN_STACK_SIZE: usize = 0x40000; // 256K pub struct Thread { @@ -72,7 +70,11 @@ impl Thread { extern "C" fn thread_start(main: *mut libc::c_void) -> *mut libc::c_void { unsafe { - start_thread(main as *mut u8); + // Next, set up our stack overflow handler which may get triggered if we run + // out of stack. + let _handler = stack_overflow::Handler::new(); + // Finally, let's run some code. + Box::from_raw(main as *mut Box)(); } ptr::null_mut() } diff --git a/src/libstd/sys/windows/thread.rs b/src/libstd/sys/windows/thread.rs index 052f51a33ce..a1cad19d0f5 100644 --- a/src/libstd/sys/windows/thread.rs +++ b/src/libstd/sys/windows/thread.rs @@ -4,7 +4,7 @@ use crate::mem; use crate::ptr; use crate::sys::c; use crate::sys::handle::Handle; -use crate::sys_common::thread::*; +use crate::sys::stack_overflow; use crate::time::Duration; use libc::c_void; @@ -50,7 +50,11 @@ impl Thread { extern "system" fn thread_start(main: *mut c_void) -> c::DWORD { unsafe { - start_thread(main as *mut u8); + // Next, set up our stack overflow handler which may get triggered if we run + // out of stack. + let _handler = stack_overflow::Handler::new(); + // Finally, let's run some code. + Box::from_raw(main as *mut Box)(); } 0 } diff --git a/src/libstd/sys_common/thread.rs b/src/libstd/sys_common/thread.rs index 6ab0d5cbe9c..f3a8bef8f71 100644 --- a/src/libstd/sys_common/thread.rs +++ b/src/libstd/sys_common/thread.rs @@ -1,18 +1,7 @@ use crate::env; use crate::sync::atomic::{self, Ordering}; -use crate::sys::stack_overflow; use crate::sys::thread as imp; -#[allow(dead_code)] -pub unsafe fn start_thread(main: *mut u8) { - // Next, set up our stack overflow handler which may get triggered if we run - // out of stack. - let _handler = stack_overflow::Handler::new(); - - // Finally, let's run some code. - Box::from_raw(main as *mut Box)() -} - pub fn min_stack() -> usize { static MIN: atomic::AtomicUsize = atomic::AtomicUsize::new(0); match MIN.load(Ordering::SeqCst) { -- cgit 1.4.1-3-g733a5 From 5382347064ac47a2a5ac56b57cec0d91b9b40edc Mon Sep 17 00:00:00 2001 From: Vytautas Astrauskas Date: Tue, 31 Mar 2020 17:51:51 -0700 Subject: Use Box::into_raw instead of ManuallyDrop in Thread::new. --- src/libstd/sys/cloudabi/thread.rs | 13 ++++--------- src/libstd/sys/hermit/thread.rs | 8 ++++---- src/libstd/sys/unix/thread.rs | 13 ++++--------- src/libstd/sys/vxworks/thread.rs | 13 ++++--------- src/libstd/sys/windows/thread.rs | 9 ++++----- 5 files changed, 20 insertions(+), 36 deletions(-) (limited to 'src/libstd/sys/windows') diff --git a/src/libstd/sys/cloudabi/thread.rs b/src/libstd/sys/cloudabi/thread.rs index 112bb1ce3af..9d95a61c315 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) -> io::Result { - let mut p = mem::ManuallyDrop::new(box p); + let p = Box::into_raw(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,18 +30,13 @@ 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, - &mut *p as &mut Box as *mut _ as *mut _, - ); + let ret = libc::pthread_create(&mut native, &attr, thread_start, p 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); + // safe to reconstruct the box so that it gets deallocated. + let _ = Box::from_raw(p); Err(io::Error::from_raw_os_error(ret)) } else { Ok(Thread { id: native }) diff --git a/src/libstd/sys/hermit/thread.rs b/src/libstd/sys/hermit/thread.rs index f92c18a3a45..6b009037805 100644 --- a/src/libstd/sys/hermit/thread.rs +++ b/src/libstd/sys/hermit/thread.rs @@ -48,20 +48,20 @@ impl Thread { p: Box, core_id: isize, ) -> io::Result { - let mut p = mem::ManuallyDrop::new(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, - &mut *p as &mut Box as *mut _ as *mut u8 as usize, + p as *mut u8 as usize, Priority::into(NORMAL_PRIO), core_id, ); 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); + // safe to reconstruct the box so that it gets deallocated. + let _ = Box::from_raw(p); Err(io::Error::new(io::ErrorKind::Other, "Unable to create thread!")) } else { Ok(Thread { tid: tid }) diff --git a/src/libstd/sys/unix/thread.rs b/src/libstd/sys/unix/thread.rs index 513a1fde331..1cad474e33e 100644 --- a/src/libstd/sys/unix/thread.rs +++ b/src/libstd/sys/unix/thread.rs @@ -41,7 +41,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) -> io::Result { - let mut p = mem::ManuallyDrop::new(box p); + let p = Box::into_raw(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); @@ -63,18 +63,13 @@ impl Thread { } }; - let ret = libc::pthread_create( - &mut native, - &attr, - thread_start, - &mut *p as &mut Box as *mut _ as *mut _, - ); + let ret = libc::pthread_create(&mut native, &attr, thread_start, p 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); + // safe to reconstruct the box so that it gets deallocated. + let _ = Box::from_raw(p); Err(io::Error::from_raw_os_error(ret)) } else { Ok(Thread { id: native }) diff --git a/src/libstd/sys/vxworks/thread.rs b/src/libstd/sys/vxworks/thread.rs index 92163865b79..3c9557db94a 100644 --- a/src/libstd/sys/vxworks/thread.rs +++ b/src/libstd/sys/vxworks/thread.rs @@ -29,7 +29,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) -> io::Result { - let mut p = mem::ManuallyDrop::new(box p); + let p = Box::into_raw(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); @@ -51,18 +51,13 @@ impl Thread { } }; - let ret = libc::pthread_create( - &mut native, - &attr, - thread_start, - &mut *p as &mut Box as *mut _ as *mut _, - ); + let ret = libc::pthread_create(&mut native, &attr, thread_start, p 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); + // safe to reconstruct the box so that it gets deallocated. + let _ = Box::from_raw(p); Err(io::Error::from_raw_os_error(ret)) } else { Ok(Thread { id: native }) diff --git a/src/libstd/sys/windows/thread.rs b/src/libstd/sys/windows/thread.rs index a1cad19d0f5..e39c1c0a132 100644 --- a/src/libstd/sys/windows/thread.rs +++ b/src/libstd/sys/windows/thread.rs @@ -1,6 +1,5 @@ use crate::ffi::CStr; use crate::io; -use crate::mem; use crate::ptr; use crate::sys::c; use crate::sys::handle::Handle; @@ -20,7 +19,7 @@ pub struct Thread { impl Thread { // unsafe: see thread::Builder::spawn_unchecked for safety requirements pub unsafe fn new(stack: usize, p: Box) -> io::Result { - let mut p = mem::ManuallyDrop::new(box p); + let p = Box::into_raw(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 +33,15 @@ impl Thread { ptr::null_mut(), stack_size, thread_start, - &mut *p as &mut Box as *mut _ as *mut _, + p 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); + // safe to reconstruct the box so that it gets deallocated. + let _ = Box::from_raw(p); Err(io::Error::last_os_error()) } else { Ok(Thread { handle: Handle::new(ret) }) -- cgit 1.4.1-3-g733a5 From baa6d557a7b965ff8277f940a43e0ce3df3b8913 Mon Sep 17 00:00:00 2001 From: Vytautas Astrauskas Date: Wed, 1 Apr 2020 12:46:14 -0700 Subject: In Thread::new, add a comment that a panic could cause a memory leak. --- src/libstd/sys/cloudabi/thread.rs | 5 ++++- src/libstd/sys/hermit/thread.rs | 2 +- src/libstd/sys/unix/thread.rs | 5 ++++- src/libstd/sys/vxworks/thread.rs | 5 ++++- src/libstd/sys/windows/thread.rs | 2 +- 5 files changed, 14 insertions(+), 5 deletions(-) (limited to 'src/libstd/sys/windows') diff --git a/src/libstd/sys/cloudabi/thread.rs b/src/libstd/sys/cloudabi/thread.rs index 9d95a61c315..abc15b18e32 100644 --- a/src/libstd/sys/cloudabi/thread.rs +++ b/src/libstd/sys/cloudabi/thread.rs @@ -31,12 +31,15 @@ impl Thread { assert_eq!(libc::pthread_attr_setstacksize(&mut attr, stack_size), 0); let ret = libc::pthread_create(&mut native, &attr, thread_start, p as *mut _); + // Note: if the thread creation fails and this assert fails, then p will + // be leaked. However, an alternative design could cause double-free + // which is clearly worse. 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 reconstruct the box so that it gets deallocated. - let _ = Box::from_raw(p); + drop(Box::from_raw(p)); Err(io::Error::from_raw_os_error(ret)) } else { Ok(Thread { id: native }) diff --git a/src/libstd/sys/hermit/thread.rs b/src/libstd/sys/hermit/thread.rs index 6b009037805..4f20a6453fc 100644 --- a/src/libstd/sys/hermit/thread.rs +++ b/src/libstd/sys/hermit/thread.rs @@ -61,7 +61,7 @@ impl Thread { 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. - let _ = Box::from_raw(p); + drop(Box::from_raw(p)); Err(io::Error::new(io::ErrorKind::Other, "Unable to create thread!")) } else { Ok(Thread { tid: tid }) diff --git a/src/libstd/sys/unix/thread.rs b/src/libstd/sys/unix/thread.rs index 1cad474e33e..aab5a92a7ad 100644 --- a/src/libstd/sys/unix/thread.rs +++ b/src/libstd/sys/unix/thread.rs @@ -64,12 +64,15 @@ impl Thread { }; let ret = libc::pthread_create(&mut native, &attr, thread_start, p as *mut _); + // Note: if the thread creation fails and this assert fails, then p will + // be leaked. However, an alternative design could cause double-free + // which is clearly worse. 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 reconstruct the box so that it gets deallocated. - let _ = Box::from_raw(p); + drop(Box::from_raw(p)); Err(io::Error::from_raw_os_error(ret)) } else { Ok(Thread { id: native }) diff --git a/src/libstd/sys/vxworks/thread.rs b/src/libstd/sys/vxworks/thread.rs index 3c9557db94a..4d0196e4b4d 100644 --- a/src/libstd/sys/vxworks/thread.rs +++ b/src/libstd/sys/vxworks/thread.rs @@ -52,12 +52,15 @@ impl Thread { }; let ret = libc::pthread_create(&mut native, &attr, thread_start, p as *mut _); + // Note: if the thread creation fails and this assert fails, then p will + // be leaked. However, an alternative design could cause double-free + // which is clearly worse. 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 reconstruct the box so that it gets deallocated. - let _ = Box::from_raw(p); + drop(Box::from_raw(p)); Err(io::Error::from_raw_os_error(ret)) } else { Ok(Thread { id: native }) diff --git a/src/libstd/sys/windows/thread.rs b/src/libstd/sys/windows/thread.rs index e39c1c0a132..38839ea5e90 100644 --- a/src/libstd/sys/windows/thread.rs +++ b/src/libstd/sys/windows/thread.rs @@ -41,7 +41,7 @@ impl Thread { return if ret as usize == 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. - let _ = Box::from_raw(p); + drop(Box::from_raw(p)); Err(io::Error::last_os_error()) } else { Ok(Thread { handle: Handle::new(ret) }) -- cgit 1.4.1-3-g733a5