Merge pull request #1530 from alexcrichton/drop-glue-closures

Protect against segfaults calling destroyed closures
rustwasm · May 13, 2019 · c504c38 · c504c38
2 parents f977630 + 542076d
commit c504c38
Show file tree

Hide file tree

Showing 3 changed files with 59 additions and 4 deletions.
diff --git a/src/closure.rs b/src/closure.rs
@@ -576,7 +576,14 @@ macro_rules! doit {
                     a: usize,
                     b: usize,
                 ) {
-                    debug_assert!(a != 0, "should never destroy a Fn whose pointer is 0");
+                    // This can be called by the JS glue in erroneous situations
+                    // such as when the closure has already been destroyed. If
+                    // that's the case let's not make things worse by
+                    // segfaulting and/or asserting, so just ignore null
+                    // pointers.
+                    if a == 0 {
+                        return;
+                    }
                     drop(Box::from_raw(FatPtr::<Fn($($var,)*) -> R> {
                         fields: (a, b)
                     }.ptr));
@@ -623,7 +630,10 @@ macro_rules! doit {
                     a: usize,
                     b: usize,
                 ) {
-                    debug_assert!(a != 0, "should never destroy a FnMut whose pointer is 0");
+                    // See `Fn()` above for why we simply return
+                    if a == 0 {
+                        return;
+                    }
                     drop(Box::from_raw(FatPtr::<FnMut($($var,)*) -> R> {
                         fields: (a, b)
                     }.ptr));
@@ -736,7 +746,10 @@ unsafe impl<A, R> WasmClosure for Fn(&A) -> R
             a: usize,
             b: usize,
         ) {
-            debug_assert!(a != 0, "should never destroy a Fn whose pointer is 0");
+            // See `Fn()` above for why we simply return
+            if a == 0 {
+                return;
+            }
             drop(Box::from_raw(FatPtr::<Fn(&A) -> R> {
                 fields: (a, b)
             }.ptr));
@@ -781,7 +794,10 @@ unsafe impl<A, R> WasmClosure for FnMut(&A) -> R
             a: usize,
             b: usize,
         ) {
-            debug_assert!(a != 0, "should never destroy a FnMut whose pointer is 0");
+            // See `Fn()` above for why we simply return
+            if a == 0 {
+                return;
+            }
             drop(Box::from_raw(FatPtr::<FnMut(&A) -> R> {
                 fields: (a, b)
             }.ptr));

diff --git a/tests/wasm/closures.js b/tests/wasm/closures.js
@@ -119,3 +119,7 @@ exports.pass_reference_first_arg_twice = (a, b, c) => {
   c(a);
   a.free();
 };
+
+exports.call_destroyed = f => {
+  assert.throws(f, /invoked recursively or destroyed/);
+};
diff --git a/tests/wasm/closures.rs b/tests/wasm/closures.rs
@@ -102,6 +102,7 @@ extern "C" {
         b: &mut FnMut(&RefFirstArgument),
         c: &mut FnMut(&RefFirstArgument),
     );
+    fn call_destroyed(a: &JsValue);
 }
 
 #[wasm_bindgen_test]
@@ -522,3 +523,37 @@ fn reference_as_first_argument_works2() {
     );
     assert_eq!(a.get(), 2);
 }
+
+#[wasm_bindgen_test]
+fn call_destroyed_doesnt_segfault() {
+    struct A(i32, i32);
+    impl Drop for A {
+        fn drop(&mut self) {
+            assert_eq!(self.0, self.1);
+        }
+    }
+
+    let a = A(1, 1);
+    let a = Closure::wrap(Box::new(move || drop(&a)) as Box<Fn()>);
+    let b = a.as_ref().clone();
+    drop(a);
+    call_destroyed(&b);
+
+    let a = A(2, 2);
+    let a = Closure::wrap(Box::new(move || drop(&a)) as Box<FnMut()>);
+    let b = a.as_ref().clone();
+    drop(a);
+    call_destroyed(&b);
+
+    let a = A(1, 1);
+    let a = Closure::wrap(Box::new(move |_: &JsValue| drop(&a)) as Box<Fn(&JsValue)>);
+    let b = a.as_ref().clone();
+    drop(a);
+    call_destroyed(&b);
+
+    let a = A(2, 2);
+    let a = Closure::wrap(Box::new(move |_: &JsValue| drop(&a)) as Box<FnMut(&JsValue)>);
+    let b = a.as_ref().clone();
+    drop(a);
+    call_destroyed(&b);
+}