From 542076d6583f47e8c08497f172e32f8c9780910f Mon Sep 17 00:00:00 2001
From: Alex Crichton <alex@alexcrichton.com>
Date: Mon, 13 May 2019 07:22:33 -0700
Subject: [PATCH] Protect against segfaults calling destroyed closures

This commit updates the drop glue generated for closures to simply
ignore null pointers. The drop glue can be called in erroneous
situations such as when a closure is invoked after it's been destroyed.
In these cases we don't want to segfault and/or corrupt memory but
instead let the normal error message from the invoke glue continue to
get propagated.

Closes #1526
---
 src/closure.rs         | 24 ++++++++++++++++++++----
 tests/wasm/closures.js |  4 ++++
 tests/wasm/closures.rs | 35 +++++++++++++++++++++++++++++++++++
 3 files changed, 59 insertions(+), 4 deletions(-)

diff --git a/src/closure.rs b/src/closure.rs
index 5067c799d23..a1fc45c8bf3 100644
--- 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
index 7acaa3832ad..03b5d59dbe7 100644
--- 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
index 1e5af49ebbd..c6a16f07e0e 100755
--- 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);
+}