Skip to content

Commit

Permalink
Fix use-after-free on panic during ArcWake::wake_by_ref
Browse files Browse the repository at this point in the history
Wrap temporary `Arc<>`s in `ManuallyDrop` early instead of calling
`forget()` later: that way even during unwinding for panics it doesn't
drop the refcount it doesn't actually own.

Also it means `wake_by_ref` doesn't need an unwind section anymore.

Same thing in `increase_refcount` (although `Arc::clone` should only abort,
not unwind).
  • Loading branch information
stbuehler committed Aug 15, 2019
1 parent 7b86128 commit 6bacce1
Show file tree
Hide file tree
Showing 2 changed files with 25 additions and 8 deletions.
14 changes: 6 additions & 8 deletions futures-util/src/task/waker.rs
Expand Up @@ -22,12 +22,10 @@ where
// code here. We should guard against this by aborting.

unsafe fn increase_refcount<T: ArcWake>(data: *const ()) {
// Retain Arc by creating a copy
let arc: Arc<T> = Arc::from_raw(data as *const T);
let arc_clone = arc.clone();
// Forget the Arcs again, so that the refcount isn't decrased
mem::forget(arc);
mem::forget(arc_clone);
// Retain Arc, but don't touch refcount by wrapping in ManuallyDrop
let arc = mem::ManuallyDrop::new(Arc::<T>::from_raw(data as *const T));
// Now increase refcount, but don't drop new refcount either
let _arc_clone: mem::ManuallyDrop<_> = arc.clone();
}

// used by `waker_ref`
Expand All @@ -43,9 +41,9 @@ unsafe fn wake_arc_raw<T: ArcWake>(data: *const ()) {

// used by `waker_ref`
pub(super) unsafe fn wake_by_ref_arc_raw<T: ArcWake>(data: *const ()) {
let arc: Arc<T> = Arc::from_raw(data as *const T);
// Retain Arc, but don't touch refcount by wrapping in ManuallyDrop
let arc = mem::ManuallyDrop::new(Arc::<T>::from_raw(data as *const T));
ArcWake::wake_by_ref(&arc);
mem::forget(arc);
}

unsafe fn drop_arc_raw<T: ArcWake>(data: *const ()) {
Expand Down
19 changes: 19 additions & 0 deletions futures/tests/arc_wake.rs
Expand Up @@ -44,3 +44,22 @@ fn create_waker_from_arc() {
drop(w1);
assert_eq!(1, Arc::strong_count(&some_w));
}

struct PanicWaker;

impl ArcWake for PanicWaker {
fn wake_by_ref(_arc_self: &Arc<Self>) {
panic!("WAKE UP");
}
}

#[test]
fn proper_refcount_on_wake_panic() {
let some_w = Arc::new(PanicWaker);

let w1: Waker = task::waker(some_w.clone());
assert_eq!("WAKE UP", *std::panic::catch_unwind(|| w1.wake_by_ref()).unwrap_err().downcast::<&str>().unwrap());
assert_eq!(2, Arc::strong_count(&some_w)); // some_w + w1
drop(w1);
assert_eq!(1, Arc::strong_count(&some_w)); // some_w
}

0 comments on commit 6bacce1

Please sign in to comment.