wasmtime / PR #2453 Add differential fuzzing against wasm... · git-wasmtime

Stream: git-wasmtime

Topic: wasmtime / PR #2453 Add differential fuzzing against wasm...

Wasmtime GitHub notifications bot (Nov 25 2020 at 23:35):

cfallin requested fitzgen and iximeow for a review on PR #2453.

Wasmtime GitHub notifications bot (Nov 25 2020 at 23:35):

cfallin opened PR #2453 from differential-fuzz-interp to main:

This PR adds a new fuzz target, differential_wasmi, that runs a
Cranelift-based Wasm backend alongside a simple third-party Wasm
interpeter crate (wasmi). The fuzzing runs the first function in a
given module to completion on each side, and then diffs the return value
and linear memory contents.

This strategy should provide end-to-end coverage including both the Wasm
translation to CLIF (which has seen some subtle and scary bugs at
times), the lowering from CLIF to VCode, the register allocation, and
the final code emission.

Wasmtime GitHub notifications bot (Nov 25 2020 at 23:35):

cfallin requested fitzgen and iximeow for a review on PR #2453.

Wasmtime GitHub notifications bot (Nov 25 2020 at 23:45):

cfallin updated PR #2453 from differential-fuzz-interp to main:

This PR adds a new fuzz target, differential_wasmi, that runs a
Cranelift-based Wasm backend alongside a simple third-party Wasm
interpeter crate (wasmi). The fuzzing runs the first function in a
given module to completion on each side, and then diffs the return value
and linear memory contents.

This strategy should provide end-to-end coverage including both the Wasm
translation to CLIF (which has seen some subtle and scary bugs at
times), the lowering from CLIF to VCode, the register allocation, and
the final code emission.

Wasmtime GitHub notifications bot (Nov 25 2020 at 23:58):

cfallin updated PR #2453 from differential-fuzz-interp to main:

This PR adds a new fuzz target, differential_wasmi, that runs a
Cranelift-based Wasm backend alongside a simple third-party Wasm
interpeter crate (wasmi). The fuzzing runs the first function in a
given module to completion on each side, and then diffs the return value
and linear memory contents.

This strategy should provide end-to-end coverage including both the Wasm
translation to CLIF (which has seen some subtle and scary bugs at
times), the lowering from CLIF to VCode, the register allocation, and
the final code emission.

Wasmtime GitHub notifications bot (Nov 26 2020 at 00:06):

cfallin updated PR #2453 from differential-fuzz-interp to main:

This PR adds a new fuzz target, differential_wasmi, that runs a
Cranelift-based Wasm backend alongside a simple third-party Wasm
interpeter crate (wasmi). The fuzzing runs the first function in a
given module to completion on each side, and then diffs the return value
and linear memory contents.

This strategy should provide end-to-end coverage including both the Wasm
translation to CLIF (which has seen some subtle and scary bugs at
times), the lowering from CLIF to VCode, the register allocation, and
the final code emission.

Wasmtime GitHub notifications bot (Nov 26 2020 at 00:11):

cfallin updated PR #2453 from differential-fuzz-interp to main:

This PR adds a new fuzz target, differential_wasmi, that runs a
Cranelift-based Wasm backend alongside a simple third-party Wasm
interpeter crate (wasmi). The fuzzing runs the first function in a
given module to completion on each side, and then diffs the return value
and linear memory contents.

This strategy should provide end-to-end coverage including both the Wasm
translation to CLIF (which has seen some subtle and scary bugs at
times), the lowering from CLIF to VCode, the register allocation, and
the final code emission.

Wasmtime GitHub notifications bot (Nov 26 2020 at 00:21):

alexcrichton submitted PR Review.

Wasmtime GitHub notifications bot (Nov 26 2020 at 00:21):

alexcrichton submitted PR Review.

Wasmtime GitHub notifications bot (Nov 26 2020 at 00:21):

alexcrichton created PR Review Comment:

This can be tightened up a bit with let payload = payload.ok()? and in a few other places too

Wasmtime GitHub notifications bot (Nov 26 2020 at 00:21):

alexcrichton created PR Review Comment:

Does wasmi not support larger memories? Or is this to just limit how much memory we're comparing?

Wasmtime GitHub notifications bot (Nov 26 2020 at 00:21):

alexcrichton created PR Review Comment:

This function may actually be best done elsewhere perhaps? It might make more sense to create a wasmtime::Config and use wasmtime to compile the module first, and that can do a lot of the gating here. We can disable wasm features that wasmi hasn't implemented to prevent them getting past validation.

Additionally once we've got wasmtime::Module we can inspect the exports for what to locate.

Wasmtime GitHub notifications bot (Nov 26 2020 at 00:21):

alexcrichton created PR Review Comment:

Another alternative is to configure wasm_smith::Config to generate only appropriate modules for running in wasmi. We could presumably add configuration options for things like "disable this proposal", "memories can be at most this big", or "there must be a main function which takes no arguments"

Wasmtime GitHub notifications bot (Nov 30 2020 at 18:00):

fitzgen submitted PR Review.

Wasmtime GitHub notifications bot (Nov 30 2020 at 18:00):

fitzgen created PR Review Comment:

    if wasmi_mem.current_size().0 != wasmtime_mem.size() {
We have a method to get the size in pages, so no need for the manual conversion here.

Wasmtime GitHub notifications bot (Nov 30 2020 at 18:00):

fitzgen created PR Review Comment:

Out of curiosity, why can't we handle start functions?

Wasmtime GitHub notifications bot (Nov 30 2020 at 18:00):

fitzgen submitted PR Review.

Wasmtime GitHub notifications bot (Nov 30 2020 at 18:00):

fitzgen created PR Review Comment:

Yeah, I think wasm_smith::Config is the way to go here.

Wasmtime GitHub notifications bot (Nov 30 2020 at 18:00):

fitzgen created PR Review Comment:

I assume this is to limit the size of memory, since we are comparing wasmtime and wasmi memories.

Again, rather than filtering generated wasm modules, we can get better throughput by only generating Wasm modules that have the shape we're looking for (via wasm_smith::Config).

Aside: it would be good to do this memory comparison with our other differential fuzz targets as well. I had assumed it would be too slow, but now that we have wasm-smith and can control memory sizes, that assumption doesn't necessarily hold anymore.

Wasmtime GitHub notifications bot (Dec 02 2020 at 00:10):

cfallin updated PR #2453 from differential-fuzz-interp to main:

This PR adds a new fuzz target, differential_wasmi, that runs a
Cranelift-based Wasm backend alongside a simple third-party Wasm
interpeter crate (wasmi). The fuzzing runs the first function in a
given module to completion on each side, and then diffs the return value
and linear memory contents.

This strategy should provide end-to-end coverage including both the Wasm
translation to CLIF (which has seen some subtle and scary bugs at
times), the lowering from CLIF to VCode, the register allocation, and
the final code emission.

Wasmtime GitHub notifications bot (Dec 02 2020 at 00:23):

cfallin updated PR #2453 from differential-fuzz-interp to main:

This PR adds a new fuzz target, differential_wasmi, that runs a
Cranelift-based Wasm backend alongside a simple third-party Wasm
interpeter crate (wasmi). The fuzzing runs the first function in a
given module to completion on each side, and then diffs the return value
and linear memory contents.

This strategy should provide end-to-end coverage including both the Wasm
translation to CLIF (which has seen some subtle and scary bugs at
times), the lowering from CLIF to VCode, the register allocation, and
the final code emission.

Wasmtime GitHub notifications bot (Dec 02 2020 at 00:24):

cfallin submitted PR Review.

Wasmtime GitHub notifications bot (Dec 02 2020 at 00:24):

cfallin created PR Review Comment:

Done!

Wasmtime GitHub notifications bot (Dec 02 2020 at 00:27):

cfallin submitted PR Review.

Wasmtime GitHub notifications bot (Dec 02 2020 at 00:27):

cfallin created PR Review Comment:

This is mostly just for simplicity -- instantiate module first on both sides, then execute and compare. Also start() can't return a value, and it seemed better to (i) only execute one function, to economize on fuzzing input bytes (more efficient search) and (ii) allow a return-value comparison. Since we have allow_start_export() in the Config now I think this is probably fine, as we aren't rejecting any modules because of it?

Wasmtime GitHub notifications bot (Dec 02 2020 at 00:27):

cfallin submitted PR Review.

Wasmtime GitHub notifications bot (Dec 02 2020 at 00:27):

cfallin created PR Review Comment:

Done!

Wasmtime GitHub notifications bot (Dec 02 2020 at 00:28):

cfallin submitted PR Review.

Wasmtime GitHub notifications bot (Dec 02 2020 at 00:28):

cfallin created PR Review Comment:

Indeed, this is both to limit the comparison time and to reduce memory usage; I was getting OOMs otherwise during fuzzing.

Wasmtime GitHub notifications bot (Dec 02 2020 at 00:29):

cfallin submitted PR Review.

Wasmtime GitHub notifications bot (Dec 02 2020 at 00:29):

cfallin created PR Review Comment:

Oh, neat, I didn't know that Option was question-mark-operator-friendly too; thanks!

Wasmtime GitHub notifications bot (Dec 02 2020 at 00:48):

cfallin updated PR #2453 from differential-fuzz-interp to main:

This PR adds a new fuzz target, differential_wasmi, that runs a
Cranelift-based Wasm backend alongside a simple third-party Wasm
interpeter crate (wasmi). The fuzzing runs the first function in a
given module to completion on each side, and then diffs the return value
and linear memory contents.

This strategy should provide end-to-end coverage including both the Wasm
translation to CLIF (which has seen some subtle and scary bugs at
times), the lowering from CLIF to VCode, the register allocation, and
the final code emission.

Wasmtime GitHub notifications bot (Dec 02 2020 at 15:01):

alexcrichton submitted PR Review.

Wasmtime GitHub notifications bot (Dec 02 2020 at 15:01):

alexcrichton submitted PR Review.

Wasmtime GitHub notifications bot (Dec 02 2020 at 15:01):

alexcrichton created PR Review Comment:

It's ok to omit default here since it doesn't do anything.

Wasmtime GitHub notifications bot (Dec 02 2020 at 15:01):

alexcrichton created PR Review Comment:

In this crate you might be able to do this with #[derive(Arbitrary)] I think too

Wasmtime GitHub notifications bot (Dec 02 2020 at 15:01):

alexcrichton created PR Review Comment:

For .ok()? here, is that ignoring traps? Or are there other failure conditions? Ideally we'd also test that if wasmi trapped that wasmtime also traps (and the memories still look the same)

Wasmtime GitHub notifications bot (Dec 02 2020 at 15:01):

alexcrichton created PR Review Comment:

FWIW this can also be let mut wasmi_buf = vec![0; wasmtime_mem.data_size()];

Wasmtime GitHub notifications bot (Dec 02 2020 at 15:01):

alexcrichton created PR Review Comment:

It should be ok to remove default here too

Wasmtime GitHub notifications bot (Dec 02 2020 at 22:52):

cfallin updated PR #2453 from differential-fuzz-interp to main:

This PR adds a new fuzz target, differential_wasmi, that runs a
Cranelift-based Wasm backend alongside a simple third-party Wasm
interpeter crate (wasmi). The fuzzing runs the first function in a
given module to completion on each side, and then diffs the return value
and linear memory contents.

This strategy should provide end-to-end coverage including both the Wasm
translation to CLIF (which has seen some subtle and scary bugs at
times), the lowering from CLIF to VCode, the register allocation, and
the final code emission.

Wasmtime GitHub notifications bot (Dec 02 2020 at 22:53):

cfallin submitted PR Review.

Wasmtime GitHub notifications bot (Dec 02 2020 at 22:53):

cfallin created PR Review Comment:

Yes, originally I had figured it would be the most useful to just compare successful runs on both sides, but I agree that allowing traps allows for more interesting coverage -- fixed (here and in value-matching below), thanks!