Skip to content

Commit 1ea8653

Browse files
CentrilCentril
authored
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 parents 17a59fb + d512b22 commit 1ea8653

File tree

10 files changed

+59
-77
lines changed

10 files changed

+59
-77
lines changed

‎src/libstd/sys/cloudabi/stack_overflow.rs

-8
Original file line numberDiff line numberDiff line change
@@ -1,13 +1,5 @@
11
#![cfg_attr(test, allow(dead_code))]
22

3-
pub struct Handler;
4-
5-
impl Handler {
6-
pub unsafe fn new() -> Handler {
7-
Handler
8-
}
9-
}
10-
113
pub unsafe fn init() {}
124

135
pub unsafe fn cleanup() {}

‎src/libstd/sys/cloudabi/thread.rs

+10-5
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,6 @@ use crate::mem;
55
use crate::ptr;
66
use crate::sys::cloudabi::abi;
77
use crate::sys::time::checked_dur2intervals;
8-
use crate::sys_common::thread::*;
98
use crate::time::Duration;
109

1110
pub const DEFAULT_MIN_STACK_SIZE: usize = 2 * 1024 * 1024;
@@ -22,27 +21,33 @@ unsafe impl Sync for Thread {}
2221
impl Thread {
2322
// unsafe: see thread::Builder::spawn_unchecked for safety requirements
2423
pub unsafe fn new(stack: usize, p: Box<dyn FnOnce()>) -> io::Result<Thread> {
25-
let p = box p;
24+
let p = Box::into_raw(box p);
2625
let mut native: libc::pthread_t = mem::zeroed();
2726
let mut attr: libc::pthread_attr_t = mem::zeroed();
2827
assert_eq!(libc::pthread_attr_init(&mut attr), 0);
2928

3029
let stack_size = cmp::max(stack, min_stack_size(&attr));
3130
assert_eq!(libc::pthread_attr_setstacksize(&mut attr, stack_size), 0);
3231

33-
let ret = libc::pthread_create(&mut native, &attr, thread_start, &*p as *const _ as *mut _);
32+
let ret = libc::pthread_create(&mut native, &attr, thread_start, p as *mut _);
33+
// Note: if the thread creation fails and this assert fails, then p will
34+
// be leaked. However, an alternative design could cause double-free
35+
// which is clearly worse.
3436
assert_eq!(libc::pthread_attr_destroy(&mut attr), 0);
3537

3638
return if ret != 0 {
39+
// The thread failed to start and as a result p was not consumed. Therefore, it is
40+
// safe to reconstruct the box so that it gets deallocated.
41+
drop(Box::from_raw(p));
3742
Err(io::Error::from_raw_os_error(ret))
3843
} else {
39-
mem::forget(p); // ownership passed to pthread_create
4044
Ok(Thread { id: native })
4145
};
4246

4347
extern "C" fn thread_start(main: *mut libc::c_void) -> *mut libc::c_void {
4448
unsafe {
45-
start_thread(main as *mut u8);
49+
// Let's run some code.
50+
Box::from_raw(main as *mut Box<dyn FnOnce()>)();
4651
}
4752
ptr::null_mut()
4853
}

‎src/libstd/sys/hermit/stack_overflow.rs

-8
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,3 @@
1-
pub struct Handler;
2-
3-
impl Handler {
4-
pub unsafe fn new() -> Handler {
5-
Handler
6-
}
7-
}
8-
91
#[inline]
102
pub unsafe fn init() {}
113

‎src/libstd/sys/hermit/thread.rs

+10-9
Original file line numberDiff line numberDiff line change
@@ -8,8 +8,6 @@ use crate::sys::hermit::abi;
88
use crate::time::Duration;
99
use core::u32;
1010

11-
use crate::sys_common::thread::*;
12-
1311
pub type Tid = abi::Tid;
1412

1513
/// Priority of a task
@@ -49,26 +47,29 @@ impl Thread {
4947
p: Box<dyn FnOnce()>,
5048
core_id: isize,
5149
) -> io::Result<Thread> {
52-
let p = box p;
50+
let p = Box::into_raw(box p);
5351
let mut tid: Tid = u32::MAX;
5452
let ret = abi::spawn(
5553
&mut tid as *mut Tid,
5654
thread_start,
57-
&*p as *const _ as *const u8 as usize,
55+
p as usize,
5856
Priority::into(NORMAL_PRIO),
5957
core_id,
6058
);
6159

62-
return if ret == 0 {
63-
mem::forget(p); // ownership passed to pthread_create
64-
Ok(Thread { tid: tid })
65-
} else {
60+
return if ret != 0 {
61+
// The thread failed to start and as a result p was not consumed. Therefore, it is
62+
// safe to reconstruct the box so that it gets deallocated.
63+
drop(Box::from_raw(p));
6664
Err(io::Error::new(io::ErrorKind::Other, "Unable to create thread!"))
65+
} else {
66+
Ok(Thread { tid: tid })
6767
};
6868

6969
extern "C" fn thread_start(main: usize) {
7070
unsafe {
71-
start_thread(main as *mut u8);
71+
// Finally, let's run some code.
72+
Box::from_raw(main as *mut Box<dyn FnOnce()>)();
7273
}
7374
}
7475
}

‎src/libstd/sys/sgx/stack_overflow.rs

-8
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,3 @@
1-
pub struct Handler;
2-
3-
impl Handler {
4-
pub unsafe fn new() -> Handler {
5-
Handler
6-
}
7-
}
8-
91
#[cfg_attr(test, allow(dead_code))]
102
pub unsafe fn init() {}
113

‎src/libstd/sys/unix/thread.rs

+14-7
Original file line numberDiff line numberDiff line change
@@ -3,11 +3,9 @@ use crate::ffi::CStr;
33
use crate::io;
44
use crate::mem;
55
use crate::ptr;
6-
use crate::sys::os;
6+
use crate::sys::{os, stack_overflow};
77
use crate::time::Duration;
88

9-
use crate::sys_common::thread::*;
10-
119
#[cfg(not(target_os = "l4re"))]
1210
pub const DEFAULT_MIN_STACK_SIZE: usize = 2 * 1024 * 1024;
1311
#[cfg(target_os = "l4re")]
@@ -43,7 +41,7 @@ unsafe fn pthread_attr_setstacksize(
4341
impl Thread {
4442
// unsafe: see thread::Builder::spawn_unchecked for safety requirements
4543
pub unsafe fn new(stack: usize, p: Box<dyn FnOnce()>) -> io::Result<Thread> {
46-
let p = box p;
44+
let p = Box::into_raw(box p);
4745
let mut native: libc::pthread_t = mem::zeroed();
4846
let mut attr: libc::pthread_attr_t = mem::zeroed();
4947
assert_eq!(libc::pthread_attr_init(&mut attr), 0);
@@ -65,19 +63,28 @@ impl Thread {
6563
}
6664
};
6765

68-
let ret = libc::pthread_create(&mut native, &attr, thread_start, &*p as *const _ as *mut _);
66+
let ret = libc::pthread_create(&mut native, &attr, thread_start, p as *mut _);
67+
// Note: if the thread creation fails and this assert fails, then p will
68+
// be leaked. However, an alternative design could cause double-free
69+
// which is clearly worse.
6970
assert_eq!(libc::pthread_attr_destroy(&mut attr), 0);
7071

7172
return if ret != 0 {
73+
// The thread failed to start and as a result p was not consumed. Therefore, it is
74+
// safe to reconstruct the box so that it gets deallocated.
75+
drop(Box::from_raw(p));
7276
Err(io::Error::from_raw_os_error(ret))
7377
} else {
74-
mem::forget(p); // ownership passed to pthread_create
7578
Ok(Thread { id: native })
7679
};
7780

7881
extern "C" fn thread_start(main: *mut libc::c_void) -> *mut libc::c_void {
7982
unsafe {
80-
start_thread(main as *mut u8);
83+
// Next, set up our stack overflow handler which may get triggered if we run
84+
// out of stack.
85+
let _handler = stack_overflow::Handler::new();
86+
// Finally, let's run some code.
87+
Box::from_raw(main as *mut Box<dyn FnOnce()>)();
8188
}
8289
ptr::null_mut()
8390
}

‎src/libstd/sys/vxworks/thread.rs

+14-7
Original file line numberDiff line numberDiff line change
@@ -3,11 +3,9 @@ use crate::ffi::CStr;
33
use crate::io;
44
use crate::mem;
55
use crate::ptr;
6-
use crate::sys::os;
6+
use crate::sys::{os, stack_overflow};
77
use crate::time::Duration;
88

9-
use crate::sys_common::thread::*;
10-
119
pub const DEFAULT_MIN_STACK_SIZE: usize = 0x40000; // 256K
1210

1311
pub struct Thread {
@@ -31,7 +29,7 @@ unsafe fn pthread_attr_setstacksize(
3129
impl Thread {
3230
// unsafe: see thread::Builder::spawn_unchecked for safety requirements
3331
pub unsafe fn new(stack: usize, p: Box<dyn FnOnce()>) -> io::Result<Thread> {
34-
let p = box p;
32+
let p = Box::into_raw(box p);
3533
let mut native: libc::pthread_t = mem::zeroed();
3634
let mut attr: libc::pthread_attr_t = mem::zeroed();
3735
assert_eq!(libc::pthread_attr_init(&mut attr), 0);
@@ -53,19 +51,28 @@ impl Thread {
5351
}
5452
};
5553

56-
let ret = libc::pthread_create(&mut native, &attr, thread_start, &*p as *const _ as *mut _);
54+
let ret = libc::pthread_create(&mut native, &attr, thread_start, p as *mut _);
55+
// Note: if the thread creation fails and this assert fails, then p will
56+
// be leaked. However, an alternative design could cause double-free
57+
// which is clearly worse.
5758
assert_eq!(libc::pthread_attr_destroy(&mut attr), 0);
5859

5960
return if ret != 0 {
61+
// The thread failed to start and as a result p was not consumed. Therefore, it is
62+
// safe to reconstruct the box so that it gets deallocated.
63+
drop(Box::from_raw(p));
6064
Err(io::Error::from_raw_os_error(ret))
6165
} else {
62-
mem::forget(p); // ownership passed to pthread_create
6366
Ok(Thread { id: native })
6467
};
6568

6669
extern "C" fn thread_start(main: *mut libc::c_void) -> *mut libc::c_void {
6770
unsafe {
68-
start_thread(main as *mut u8);
71+
// Next, set up our stack overflow handler which may get triggered if we run
72+
// out of stack.
73+
let _handler = stack_overflow::Handler::new();
74+
// Finally, let's run some code.
75+
Box::from_raw(main as *mut Box<dyn FnOnce()>)();
6976
}
7077
ptr::null_mut()
7178
}

‎src/libstd/sys/wasm/stack_overflow.rs

-8
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,3 @@
1-
pub struct Handler;
2-
3-
impl Handler {
4-
pub unsafe fn new() -> Handler {
5-
Handler
6-
}
7-
}
8-
91
pub unsafe fn init() {}
102

113
pub unsafe fn cleanup() {}

‎src/libstd/sys/windows/thread.rs

+11-6
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,9 @@
11
use crate::ffi::CStr;
22
use crate::io;
3-
use crate::mem;
43
use crate::ptr;
54
use crate::sys::c;
65
use crate::sys::handle::Handle;
7-
use crate::sys_common::thread::*;
6+
use crate::sys::stack_overflow;
87
use crate::time::Duration;
98

109
use libc::c_void;
@@ -20,7 +19,7 @@ pub struct Thread {
2019
impl Thread {
2120
// unsafe: see thread::Builder::spawn_unchecked for safety requirements
2221
pub unsafe fn new(stack: usize, p: Box<dyn FnOnce()>) -> io::Result<Thread> {
23-
let p = box p;
22+
let p = Box::into_raw(box p);
2423

2524
// FIXME On UNIX, we guard against stack sizes that are too small but
2625
// that's because pthreads enforces that stacks are at least
@@ -34,21 +33,27 @@ impl Thread {
3433
ptr::null_mut(),
3534
stack_size,
3635
thread_start,
37-
&*p as *const _ as *mut _,
36+
p as *mut _,
3837
c::STACK_SIZE_PARAM_IS_A_RESERVATION,
3938
ptr::null_mut(),
4039
);
4140

4241
return if ret as usize == 0 {
42+
// The thread failed to start and as a result p was not consumed. Therefore, it is
43+
// safe to reconstruct the box so that it gets deallocated.
44+
drop(Box::from_raw(p));
4345
Err(io::Error::last_os_error())
4446
} else {
45-
mem::forget(p); // ownership passed to CreateThread
4647
Ok(Thread { handle: Handle::new(ret) })
4748
};
4849

4950
extern "system" fn thread_start(main: *mut c_void) -> c::DWORD {
5051
unsafe {
51-
start_thread(main as *mut u8);
52+
// Next, set up our stack overflow handler which may get triggered if we run
53+
// out of stack.
54+
let _handler = stack_overflow::Handler::new();
55+
// Finally, let's run some code.
56+
Box::from_raw(main as *mut Box<dyn FnOnce()>)();
5257
}
5358
0
5459
}

‎src/libstd/sys_common/thread.rs

-11
Original file line numberDiff line numberDiff line change
@@ -1,18 +1,7 @@
11
use crate::env;
22
use crate::sync::atomic::{self, Ordering};
3-
use crate::sys::stack_overflow;
43
use crate::sys::thread as imp;
54

6-
#[allow(dead_code)]
7-
pub unsafe fn start_thread(main: *mut u8) {
8-
// Next, set up our stack overflow handler which may get triggered if we run
9-
// out of stack.
10-
let _handler = stack_overflow::Handler::new();
11-
12-
// Finally, let's run some code.
13-
Box::from_raw(main as *mut Box<dyn FnOnce()>)()
14-
}
15-
165
pub fn min_stack() -> usize {
176
static MIN: atomic::AtomicUsize = atomic::AtomicUsize::new(0);
187
match MIN.load(Ordering::SeqCst) {

0 commit comments

Comments
 (0)