From 9931f5f000c8a40f5ca803ba6bb302c08af1378c Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?L=C3=A9o=20Gaspard?= <leo@near.org>
Date: Wed, 27 Sep 2023 00:20:59 +0000
Subject: [PATCH] relax unwind safety requirements
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

As far as I understand unwind safety in rust, this should be safe. First
of all, because unwind "unsafety" in rust is already safe anyway, as
shown by the absence of `unsafe` keyword.

What it tries to prevent is witnessing a broken invariant in a capture,
after a panic.

Here, we replace `RefUnwindSafe` with an `AssertUnwindSafe` that is
exclusively set on a value that will never be reused after the closure
ends.

Hence, to the best of my understanding this still keeps the informal
contract of unwind safety in rust.

In practice, it allows using wasm-smith’s Arbitrary implementation with
`with_arbitrary`, without having to take bytes as arguments. Without
that, wasm-smith’s ConfiguredModule contains elements that are not
unwind safe, and would be rejected.

I would add that the error message is particularly cryptic, and figuring
out that unwind safety was the source of the issue required me to dig
into bolero’s source code and use the fully-qualified method call
syntax, because without that the full error would not be shown.

It might be possible to hoist the remaining type checks to be earlier in
the call chain (eg. at the with_arbitrary stage), but I haven’t
investigated much deeper yet.
---
 lib/bolero-engine/src/test.rs | 19 ++++++++++++-------
 1 file changed, 12 insertions(+), 7 deletions(-)

diff --git a/lib/bolero-engine/src/test.rs b/lib/bolero-engine/src/test.rs
index 37181638..4cedde5a 100644
--- a/lib/bolero-engine/src/test.rs
+++ b/lib/bolero-engine/src/test.rs
@@ -178,11 +178,11 @@ impl<F, G, V> BorrowedGeneratorTest<F, G, V> {
     }
 }
 
-impl<F: RefUnwindSafe + FnMut(&G::Output) -> Ret, G: RefUnwindSafe + ValueGenerator, Ret> Test
+impl<F: RefUnwindSafe + FnMut(&G::Output) -> Ret, G: ValueGenerator, Ret> Test
     for BorrowedGeneratorTest<F, G, G::Output>
 where
     Ret: IntoTestResult,
-    G::Output: RefUnwindSafe + core::fmt::Debug,
+    G::Output: core::fmt::Debug,
 {
     type Value = G::Output;
 
@@ -193,10 +193,11 @@ where
         input.with_driver(&mut |driver| {
             let fun = &mut self.fun;
 
-            let value = generate_value!(self, driver);
+            // The value will not be reused after being captured, so it is unwind safe
+            let value = core::panic::AssertUnwindSafe(generate_value!(self, driver));
 
             panic::catch(|| {
-                let res = (fun)(value);
+                let res = (fun)(*value);
                 match res.into_test_result() {
                     Ok(()) => Ok(true),
                     Err(err) => Err(err),
@@ -231,11 +232,11 @@ impl<F, G, V> ClonedGeneratorTest<F, G, V> {
     }
 }
 
-impl<F: RefUnwindSafe + FnMut(G::Output) -> Ret, G: RefUnwindSafe + ValueGenerator, Ret> Test
+impl<F: RefUnwindSafe + FnMut(G::Output) -> Ret, G: ValueGenerator, Ret> Test
     for ClonedGeneratorTest<F, G, G::Output>
 where
     Ret: IntoTestResult,
-    G::Output: RefUnwindSafe + core::fmt::Debug + Clone,
+    G::Output: core::fmt::Debug + Clone,
 {
     type Value = G::Output;
 
@@ -257,7 +258,11 @@ where
             #[cfg(not(kani))]
             let input = value.clone();
 
-            panic::catch(|| {
+            // The value will not be reused after being captured, so it is unwind safe
+            let input = core::panic::AssertUnwindSafe(input);
+
+            panic::catch(move || {
+                let core::panic::AssertUnwindSafe(input) = input;
                 let res = (fun)(input);
                 match res.into_test_result() {
                     Ok(()) => Ok(true),