Skip to content

Commit

Permalink
relax unwind safety requirements
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
Ekleog-NEAR committed Sep 27, 2023
1 parent f1c1de2 commit 9931f5f
Showing 1 changed file with 12 additions and 7 deletions.
19 changes: 12 additions & 7 deletions lib/bolero-engine/src/test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand All @@ -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),
Expand Down Expand Up @@ -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;

Expand All @@ -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),
Expand Down

0 comments on commit 9931f5f

Please sign in to comment.