mikros/rust
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

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

Browse Source 
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.
This commit is contained in:
Mazdak Farrokhzad 2020-04-03 22:55:05 +02:00 committed by GitHub
commit 1ea8653d01
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
10 changed files with 59 additions and 77 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

8
src/libstd/sys/cloudabi/stack_overflow.rs
View File

@ -1,13 +1,5 @@
#![cfg_attr(test, allow(dead_code))]
pub struct Handler;
impl Handler {
pub unsafe fn new() -> Handler {
Handler
}
}
pub unsafe fn init() {}
pub unsafe fn cleanup() {}

15
src/libstd/sys/cloudabi/thread.rs
View File

@ -5,7 +5,6 @@ use crate::mem;
use crate::ptr;
use crate::sys::cloudabi::abi;
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;
@ -22,7 +21,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 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,19 +29,25 @@ 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, 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.
drop(Box::from_raw(p));
Err(io::Error::from_raw_os_error(ret))
} else {
mem::forget(p); // ownership passed to pthread_create
Ok(Thread { id: native })
};
extern "C" fn thread_start(main: *mut libc::c_void) -> *mut libc::c_void {
unsafe {
start_thread(main as *mut u8);
// Let's run some code.
Box::from_raw(main as *mut Box<dyn FnOnce()>)();
}
ptr::null_mut()
}

8
src/libstd/sys/hermit/stack_overflow.rs
View File

@ -1,11 +1,3 @@
pub struct Handler;
impl Handler {
pub unsafe fn new() -> Handler {
Handler
}
}
#[inline]
pub unsafe fn init() {}

19
src/libstd/sys/hermit/thread.rs
View File

@ -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()>)();
}
}
}

8
src/libstd/sys/sgx/stack_overflow.rs
View File

@ -1,11 +1,3 @@
pub struct Handler;
impl Handler {
pub unsafe fn new() -> Handler {
Handler
}
}
#[cfg_attr(test, allow(dead_code))]
pub unsafe fn init() {}

21
src/libstd/sys/unix/thread.rs
View File

@ -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")]
@ -43,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<dyn FnOnce()>) -> io::Result<Thread> {
let p = 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);
@ -65,19 +63,28 @@ 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, 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.
drop(Box::from_raw(p));
Err(io::Error::from_raw_os_error(ret))
} else {
mem::forget(p); // ownership passed to pthread_create
Ok(Thread { id: native })
};
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<dyn FnOnce()>)();
}
ptr::null_mut()
}

21
src/libstd/sys/vxworks/thread.rs
View File

@ -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 {
@ -31,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<dyn FnOnce()>) -> io::Result<Thread> {
let p = 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);
@ -53,19 +51,28 @@ 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, 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.
drop(Box::from_raw(p));
Err(io::Error::from_raw_os_error(ret))
} else {
mem::forget(p); // ownership passed to pthread_create
Ok(Thread { id: native })
};
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<dyn FnOnce()>)();
}
ptr::null_mut()
}

8
src/libstd/sys/wasm/stack_overflow.rs
View File

@ -1,11 +1,3 @@
pub struct Handler;
impl Handler {
pub unsafe fn new() -> Handler {
Handler
}
}
pub unsafe fn init() {}
pub unsafe fn cleanup() {}

17
src/libstd/sys/windows/thread.rs
View File

@ -1,10 +1,9 @@
use crate::ffi::CStr;
use crate::io;
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;
@ -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<dyn FnOnce()>) -> io::Result<Thread> {
let p = 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,21 +33,27 @@ impl Thread {
ptr::null_mut(),
stack_size,
thread_start,
&*p as *const _ 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 reconstruct the box so that it gets deallocated.
drop(Box::from_raw(p));
Err(io::Error::last_os_error())
} else {
mem::forget(p); // ownership passed to CreateThread
Ok(Thread { handle: Handle::new(ret) })
};
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<dyn FnOnce()>)();
}
0
}

11
src/libstd/sys_common/thread.rs
View File

@ -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<dyn FnOnce()>)()
}
pub fn min_stack() -> usize {
static MIN: atomic::AtomicUsize = atomic::AtomicUsize::new(0);
match MIN.load(Ordering::SeqCst) {