Stream: git-wasmtime

Topic: wasmtime / PR #3124 Fuzz Wasmtime against the official Wa...

Wasmtime GitHub notifications bot (Jul 28 2021 at 20:27):

abrown opened PR #3124 from wasm-spec-fuzzing to main:

This change introduces a new fuzz target, differential_spec that compares the outputs of executing the first exported function of a Wasm module in Wasmtime and then in the official Wasm spec interpreter (using the wasm-spec-interpreter crate). This is an initial step towards more fully-featured fuzzing (e.g. compare memories, add v128, add references, add other proposals, etc.)

Wasmtime GitHub notifications bot (Jul 28 2021 at 20:33):

abrown submitted PR review.

Wasmtime GitHub notifications bot (Jul 28 2021 at 20:33):

abrown submitted PR review.

Wasmtime GitHub notifications bot (Jul 28 2021 at 20:33):

abrown created PR review comment:

This crate has some dependencies that not all users will have: how do we want to configure this so that it will build and run by default when it can but not fail otherwise?

Wasmtime GitHub notifications bot (Jul 28 2021 at 20:33):

abrown created PR review comment:

We currently do not handle this very well (here and in other targets): we should be keeping track of how many modules we are tossing out as erroneous.

Wasmtime GitHub notifications bot (Jul 28 2021 at 20:33):

abrown created PR review comment:

This is an issue, but I'm not exactly sure how to resolve it. I originally used OCamlRuntime::init here but allocating and dropping the runtime in the same process is problematic. The approach above works (I haven't seen any crashes due to this in my local fuzzing) but cargo test occasionally crashes (I don't understand why).

Wasmtime GitHub notifications bot (Jul 28 2021 at 20:33):

abrown requested cfallin for a review on PR #3124.

Wasmtime GitHub notifications bot (Jul 28 2021 at 20:33):

abrown requested alexcrichton for a review on PR #3124.

Wasmtime GitHub notifications bot (Jul 28 2021 at 20:33):

abrown requested fitzgen for a review on PR #3124.

Wasmtime GitHub notifications bot (Jul 28 2021 at 20:34):

bjorn3 created PR review comment:

This submodule is 89MB. It will be checked out by cargo automatically when using Cranelift as git dependency like cg_clif does. Can you please make it lazily clone this repo only when fuzzing?

Wasmtime GitHub notifications bot (Jul 28 2021 at 20:34):

bjorn3 submitted PR review.

Wasmtime GitHub notifications bot (Jul 28 2021 at 20:37):

bjorn3 submitted PR review.

Wasmtime GitHub notifications bot (Jul 28 2021 at 20:37):

bjorn3 submitted PR review.

Wasmtime GitHub notifications bot (Jul 28 2021 at 20:37):

bjorn3 created PR review comment:

git submodule update --init after you cloned works too.

Wasmtime GitHub notifications bot (Jul 28 2021 at 20:39):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Jul 28 2021 at 20:39):

cfallin created PR review comment:

I haven't dug through the Cargo-related details yet but IIUC, this is only built when invoking the particular fuzz target with cargo fuzz run differential_spec, right? If so, I think it's fine -- someone is unlikely to stumble on it by accident.

Wasmtime GitHub notifications bot (Jul 28 2021 at 20:42):

cfallin created PR review comment:

make rather than cp, no?

Here we could also have a more detailed error message that tells the user something like "Build of Wasm spec interpreter failed. Please ensure that you have an OCaml development environment configured as described in .../README.md."

Wasmtime GitHub notifications bot (Jul 28 2021 at 20:42):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Jul 28 2021 at 20:44):

cfallin created PR review comment:

IIRC, the Ocaml runtime is not threadsafe (they're working on a version that is but it's a long-running project). Do we need to protect the runtime with a mutex?

Wasmtime GitHub notifications bot (Jul 28 2021 at 20:44):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Jul 28 2021 at 20:44):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Jul 28 2021 at 20:44):

fitzgen submitted PR review.

Wasmtime GitHub notifications bot (Jul 28 2021 at 20:44):

fitzgen created PR review comment:

The [..] shouldn't be required here, since &Vec<u8> will coerce to &[u8] via automatic deref coercion.

Wasmtime GitHub notifications bot (Jul 28 2021 at 20:44):

fitzgen created PR review comment:

s/wasmi/spec interpreter/

Wasmtime GitHub notifications bot (Jul 28 2021 at 20:44):

fitzgen created PR review comment:

I think you will want to check for NaNs here, since they may have different bit patterns but be equivalent for our testing concerns (despite not be "equal" under IEEE754...). Even with cranelift's NaN canonicalization enabled, the spec interpreter might not be canonicalizing or might canonicalize to a different bit pattern.

In fact, we already do this dance elsewhere in this crate, so you can factor out an existing method for comparing values.

Wasmtime GitHub notifications bot (Jul 28 2021 at 20:44):

fitzgen created PR review comment:

This should probably be unreachable!(), since we don't expect to ever get other types of values.

Wasmtime GitHub notifications bot (Jul 28 2021 at 22:02):

abrown submitted PR review.

Wasmtime GitHub notifications bot (Jul 28 2021 at 22:02):

abrown created PR review comment:

I agree it's a bit large but I'm not sure exactly how to do that. Are you thinking of doing a git clone ... in the build.rs file for wasm-spec-interpreter? Isn't there a way to set --depth 1 on the submodule somehow?

Wasmtime GitHub notifications bot (Jul 28 2021 at 22:10):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Jul 28 2021 at 22:10):

cfallin created PR review comment:

From this StackOverflow answer there is git config -f .gitmodules submodule.<name>.shallow true. I agree this would probably be preferable to a magic "git clone within the build script" solution.

If the shallow clone is still too large, another option could be to "rotate the tree", so to speak, and make this fuzzer plus the Wasm spec interpreter and submodule a root repo that pulls in wasmtime as a subrepo. It would make the oss-fuzz config a little more complex (cloning a separate repo to build this target) but would be workable. I'd still prefer it as you've done here over that solution if at all possible, though.

Wasmtime GitHub notifications bot (Jul 28 2021 at 22:16):

abrown updated PR #3124 from wasm-spec-fuzzing to main.

Wasmtime GitHub notifications bot (Jul 28 2021 at 22:24):

abrown updated PR #3124 from wasm-spec-fuzzing to main.

Wasmtime GitHub notifications bot (Jul 28 2021 at 22:26):

abrown updated PR #3124 from wasm-spec-fuzzing to main.

Wasmtime GitHub notifications bot (Jul 28 2021 at 22:26):

abrown submitted PR review.

Wasmtime GitHub notifications bot (Jul 28 2021 at 22:26):

abrown created PR review comment:

I went with the shallow approach... the other options seem more complicated.

Wasmtime GitHub notifications bot (Jul 28 2021 at 22:32):

abrown updated PR #3124 from wasm-spec-fuzzing to main.

Wasmtime GitHub notifications bot (Jul 28 2021 at 22:33):

abrown updated PR #3124 from wasm-spec-fuzzing to main.

Wasmtime GitHub notifications bot (Jul 28 2021 at 22:35):

abrown submitted PR review.

Wasmtime GitHub notifications bot (Jul 28 2021 at 22:35):

abrown created PR review comment:

Hm, well, I added --exclude wasm-spec-interpreter in a few places but I'm still getting errors. I think the problem is that wasmtime-fuzzing is being built and tested in various places and it pulls in wasm-spec-interpreter as a dependency.

Wasmtime GitHub notifications bot (Jul 28 2021 at 22:47):

abrown updated PR #3124 from wasm-spec-fuzzing to main.

Wasmtime GitHub notifications bot (Jul 28 2021 at 22:50):

abrown updated PR #3124 from wasm-spec-fuzzing to main.

Wasmtime GitHub notifications bot (Jul 28 2021 at 23:00):

abrown submitted PR review.

Wasmtime GitHub notifications bot (Jul 28 2021 at 23:00):

abrown created PR review comment:

I tried cargo test -j 1 to no avail as well as:

lazy_static! {
    static ref MUTEX: Mutex<u32> = Mutex::new(42);
}

with let _ = MUTEX.lock().unwrap(); at the top of the interpret function. I still see failures when I test but not when I fuzz.

Wasmtime GitHub notifications bot (Jul 28 2021 at 23:02):

abrown submitted PR review.

Wasmtime GitHub notifications bot (Jul 28 2021 at 23:02):

abrown created PR review comment:

Yeah, there's multiple ways to do this and an experienced Git user will probably know of several of them. Are you OK resolving this comment or are you getting at something else here?

Wasmtime GitHub notifications bot (Jul 29 2021 at 15:02):

alexcrichton created PR review comment:

Could this use the wat crate to avoid checking in a binary?

Wasmtime GitHub notifications bot (Jul 29 2021 at 15:02):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Jul 29 2021 at 15:02):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Jul 29 2021 at 15:02):

alexcrichton created PR review comment:

Similar to above, could log_wasm work here?

Wasmtime GitHub notifications bot (Jul 29 2021 at 15:02):

alexcrichton created PR review comment:

This is a relatively simple makefile, would it be feasible to inline all of this into the build script?

Wasmtime GitHub notifications bot (Jul 29 2021 at 15:02):

alexcrichton created PR review comment:

Could callers of this use log_wasm instead?

Wasmtime GitHub notifications bot (Jul 29 2021 at 15:02):

alexcrichton created PR review comment:

I think this is fine, we have crates in Rust to easily switch between text and binary so it should be fine to not bake this in

Wasmtime GitHub notifications bot (Jul 29 2021 at 15:02):

alexcrichton created PR review comment:

Technically this is also possible when the exported function requires arguments. Currently if that's the case wasmtime will return an error because no arguments are supplied here

Wasmtime GitHub notifications bot (Jul 29 2021 at 15:02):

alexcrichton created PR review comment:

Here you'll want to check the status itself (e.g. assert!(status.success())) because otherwise this is just successfully producing a status, but the status itself may represent an error and that's not checked.

Wasmtime GitHub notifications bot (Jul 29 2021 at 15:02):

alexcrichton created PR review comment:

For example CP has native rust equivalents and ar I think can be replaced with the cc crate. Otherwise I think this is largely a handcrafted invocation of ocaml?

Wasmtime GitHub notifications bot (Jul 29 2021 at 18:41):

abrown updated PR #3124 from wasm-spec-fuzzing to main.

Wasmtime GitHub notifications bot (Jul 30 2021 at 20:08):

abrown updated PR #3124 from wasm-spec-fuzzing to main.

Wasmtime GitHub notifications bot (Aug 02 2021 at 21:33):

abrown updated PR #3124 from wasm-spec-fuzzing to main.

Wasmtime GitHub notifications bot (Aug 02 2021 at 21:38):

abrown submitted PR review.

Wasmtime GitHub notifications bot (Aug 02 2021 at 21:38):

abrown created PR review comment:

Yeah, originally I had this all inlined in the build.rs but I moved away from it because it made it hard to figure out what went wrong when something invariably went wrong. This way, it's relatively easy to run make in the ocaml directory to troubleshoot.

Wasmtime GitHub notifications bot (Aug 02 2021 at 21:39):

abrown submitted PR review.

Wasmtime GitHub notifications bot (Aug 02 2021 at 21:39):

abrown created PR review comment:

:+1:

Wasmtime GitHub notifications bot (Aug 02 2021 at 23:03):

abrown updated PR #3124 from wasm-spec-fuzzing to main.

Wasmtime GitHub notifications bot (Aug 02 2021 at 23:06):

abrown submitted PR review.

Wasmtime GitHub notifications bot (Aug 02 2021 at 23:06):

abrown created PR review comment:

Yeah, that's what I mean by "issues ... running the Wasm module." And it's actually rather unfortunate that we spend time on those cases because they aren't super useful, IMHO. I checked today and about ~50% of cases are getting to this Err - Err state.

Wasmtime GitHub notifications bot (Aug 02 2021 at 23:33):

abrown updated PR #3124 from wasm-spec-fuzzing to main.

Wasmtime GitHub notifications bot (Aug 02 2021 at 23:55):

abrown updated PR #3124 from wasm-spec-fuzzing to main.

Wasmtime GitHub notifications bot (Aug 03 2021 at 00:04):

abrown updated PR #3124 from wasm-spec-fuzzing to main.

Wasmtime GitHub notifications bot (Aug 03 2021 at 02:28):

jlb6740 submitted PR review.

Wasmtime GitHub notifications bot (Aug 03 2021 at 02:28):

jlb6740 created PR review comment:

Should this be a build dependency since you don't want it if ocaml is deemed not available by build.rs?

Wasmtime GitHub notifications bot (Aug 03 2021 at 20:54):

abrown updated PR #3124 from wasm-spec-fuzzing to main.

Wasmtime GitHub notifications bot (Aug 03 2021 at 22:05):

abrown submitted PR review.

Wasmtime GitHub notifications bot (Aug 03 2021 at 22:05):

abrown created PR review comment:

@alexcrichton made a point today that the spec interpreter itself is built with make and pulling that all in to a build.rs is too much work so we might as well keep all of it in a Makefile.

Wasmtime GitHub notifications bot (Aug 03 2021 at 22:19):

abrown updated PR #3124 from wasm-spec-fuzzing to main.

Wasmtime GitHub notifications bot (Aug 03 2021 at 22:24):

abrown submitted PR review.

Wasmtime GitHub notifications bot (Aug 03 2021 at 22:24):

abrown created PR review comment:

No, it's a dependency used at runtime.

Wasmtime GitHub notifications bot (Aug 03 2021 at 22:26):

abrown updated PR #3124 from wasm-spec-fuzzing to main.

Wasmtime GitHub notifications bot (Aug 03 2021 at 22:27):

abrown updated PR #3124 from wasm-spec-fuzzing to main.

Wasmtime GitHub notifications bot (Aug 03 2021 at 23:04):

abrown updated PR #3124 from wasm-spec-fuzzing to main.

Wasmtime GitHub notifications bot (Aug 03 2021 at 23:20):

abrown updated PR #3124 from wasm-spec-fuzzing to main.

Wasmtime GitHub notifications bot (Aug 03 2021 at 23:27):

abrown submitted PR review.

Wasmtime GitHub notifications bot (Aug 03 2021 at 23:27):

abrown created PR review comment:

Yes, let me do that as a separate issue: #3140

Wasmtime GitHub notifications bot (Aug 03 2021 at 23:27):

abrown submitted PR review.

Wasmtime GitHub notifications bot (Aug 03 2021 at 23:27):

abrown created PR review comment:

Yes, let me do that as a separate issue: #3140

Wasmtime GitHub notifications bot (Aug 04 2021 at 14:53):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Aug 04 2021 at 14:53):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Aug 04 2021 at 14:53):

alexcrichton created PR review comment:

(same as above, I think this can be removed)

Wasmtime GitHub notifications bot (Aug 04 2021 at 14:53):

alexcrichton created PR review comment:

I think we can remove this with the cfg(fuzzing) in the deps now?

Wasmtime GitHub notifications bot (Aug 04 2021 at 14:53):

alexcrichton created PR review comment:

Instead of boolean checks plus a println! I think it's safe to remove this and just use cfg!(fuzzing) to guard this make. If the fuzzing cfg is set then we should actually build the library and assert that the build succeeds.

Wasmtime GitHub notifications bot (Aug 04 2021 at 14:53):

alexcrichton created PR review comment:

Basically unless cfg(fuzzing) is set I think this build script should be a noop, intentionally. With cfg(fuzzing) set, though, it would do everything it does now and assert that it succeeds.

Wasmtime GitHub notifications bot (Aug 04 2021 at 16:47):

abrown updated PR #3124 from wasm-spec-fuzzing to main.

Wasmtime GitHub notifications bot (Aug 04 2021 at 17:37):

abrown submitted PR review.

Wasmtime GitHub notifications bot (Aug 04 2021 at 17:37):

abrown created PR review comment:

When I implemented this I ran into {errors while building the fuzz targets](https://github.com/bytecodealliance/wasmtime/runs/3243864078?check_suite_focus=true#step:7:127). I think what is happening is that cfg!(fuzzing) is not set when build.rs is run but _is_ set when the actual crate is compiled (hence the compile_error!). How is that possible?

Wasmtime GitHub notifications bot (Aug 04 2021 at 17:37):

abrown edited PR review comment.

Wasmtime GitHub notifications bot (Aug 05 2021 at 15:55):

abrown updated PR #3124 from wasm-spec-fuzzing to main.

Wasmtime GitHub notifications bot (Aug 05 2021 at 16:45):

abrown updated PR #3124 from wasm-spec-fuzzing to main.

Wasmtime GitHub notifications bot (Aug 05 2021 at 16:52):

abrown submitted PR review.

Wasmtime GitHub notifications bot (Aug 05 2021 at 16:52):

abrown created PR review comment:

Ok, so I just removed the if cfg!(fuzzing) ... guard in the build.rs since it doesn't look like that will work.

Wasmtime GitHub notifications bot (Aug 05 2021 at 19:31):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Aug 05 2021 at 19:31):

alexcrichton created PR review comment:

Ah ok I think in that case we'll want to go the route putting the ocaml bits and pieces behind a feature and activate the feature with a target-specific dependency in the fuzzing crate which enables the feature.

I think it's important to not print warnings when the build fails, because no one ever looks at the build logs for oss-fuzz so if warnings get printed there we'll never know.

Wasmtime GitHub notifications bot (Aug 06 2021 at 22:16):

abrown updated PR #3124 from wasm-spec-fuzzing to main.

Wasmtime GitHub notifications bot (Aug 06 2021 at 22:18):

abrown submitted PR review.

Wasmtime GitHub notifications bot (Aug 06 2021 at 22:18):

abrown created PR review comment:

Ok, this is resolved by the latest push (thanks @alexcrichton and @cfallin for the debugging help!):

1. we _do_ need to maintain a mutex to avoid re-entrancy to the OCaml runtime but this needs to be locked with let _lock = ..., not let _ = ...
2. we need to run Wasmtime last in the oracle so that its signal handlers do not get messed up by the handlers OCaml registers.

Wasmtime GitHub notifications bot (Aug 06 2021 at 22:23):

abrown submitted PR review.

Wasmtime GitHub notifications bot (Aug 06 2021 at 22:23):

abrown created PR review comment:

Ok, after examining what Cargo prints on failure I realize that the warnings are unnecessary: now that build.rs correctly panics when make fails Cargo will print out enough information to troubleshoot with. So I removed the warnings and have refactored the crate to use two features:

• build-libinterpret is what we turn on during fuzzing and we assume the toolchain is enabled
• has-libinterpret is turned on by build.rs to indicate that the OCaml library was successfully built and we should compile the non-panicking interpret function

Wasmtime GitHub notifications bot (Aug 06 2021 at 22:38):

abrown has marked PR #3124 as ready for review.

Wasmtime GitHub notifications bot (Aug 06 2021 at 23:18):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Aug 06 2021 at 23:18):

cfallin submitted PR review.

Wasmtime GitHub notifications bot (Aug 06 2021 at 23:18):

cfallin created PR review comment:

I think it might be worthwhile to create a tracking issue for "Wasm semantics fully fuzzed against spec interpreter", with at least (i) this issue, i.e. compare memory contents, and also compare globals; and (ii) other Wasm features we want to enable in fuzzing, like SIMD, with the appropriate interpreter updates.

Wasmtime GitHub notifications bot (Aug 06 2021 at 23:18):

cfallin created PR review comment:

It doesn't seem like this is used anywhere?

Wasmtime GitHub notifications bot (Aug 07 2021 at 08:45):

bjorn3 submitted PR review.

Wasmtime GitHub notifications bot (Aug 07 2021 at 08:45):

bjorn3 created PR review comment:

This doesn't help. Cargo (using libgit2) does a deep clone of the spec and then recursively clones all submodules of the spec. That is KaTeX/KaTeX, KaTeX/katex-fonts and KaTeX/katex-test-fonts for a total checkout size of 260MB! That is reaching the size of the LLVM submodule of rust, which is a real pain to download and for that reason has even be made to only lazily get cloned when necessary. It is not necessary for ./x.py check, but this submodule will get recursively cloned with ./x.py check by cargo when building cg_clif.

Could you please instead vendor just the interpreter? The interpreter itself is less than 400kb.

Wasmtime GitHub notifications bot (Aug 10 2021 at 18:10):

abrown updated PR #3124 from wasm-spec-fuzzing to main.

Wasmtime GitHub notifications bot (Aug 10 2021 at 18:11):

abrown updated PR #3124 from wasm-spec-fuzzing to main.

Wasmtime GitHub notifications bot (Aug 10 2021 at 18:13):

abrown updated PR #3124 from wasm-spec-fuzzing to main.

Wasmtime GitHub notifications bot (Aug 10 2021 at 18:18):

abrown updated PR #3124 from wasm-spec-fuzzing to main.

Wasmtime GitHub notifications bot (Aug 10 2021 at 18:23):

abrown submitted PR review.

Wasmtime GitHub notifications bot (Aug 10 2021 at 18:23):

abrown created PR review comment:

https://github.com/bytecodealliance/wasmtime/issues/3175

Wasmtime GitHub notifications bot (Aug 10 2021 at 18:37):

bjorn3 submitted PR review.

Wasmtime GitHub notifications bot (Aug 10 2021 at 18:37):

bjorn3 created PR review comment:

I don't know if you ever want to publish this crate, but if you do, this will need to be moved to the OUT_DIR. Cargo refuses to publish crates that modify their source directory when building.

Wasmtime GitHub notifications bot (Aug 10 2021 at 18:38):

abrown submitted PR review.

Wasmtime GitHub notifications bot (Aug 10 2021 at 18:38):

abrown created PR review comment:

This crate is marked publish = false.

Wasmtime GitHub notifications bot (Aug 10 2021 at 18:56):

abrown merged PR #3124.

Last updated: Nov 22 2024 at 17:03 UTC