Update the Wasmi fuzzing oracle to version 0.31.0

[Robbepop]

Currently Wasmtime uses the 2 years old Wasmi version 0.20.0.

Since then Wasmi has improved substantially and added support for new Wasm proposals such as bulk-memory, reference-types and tail-calls which we can now enable.
Wasmi v0.31.0 has recently been audited and is used by some large projects, thus is a lot more battle tested than the previous v0.20.0.

Besides that the most notable change are performance improvements which should make fuzzing with Wasmi a tiny bit faster.

Look into the future: Since roughly half a year I am working on the next major Wasmi version v0.32.0 which is a complete rewrite of the Wasmi executor featuring a much more powerful register-machine execution model and optional lazy compilation & validation. I hope that it becomes stable enough for use soon to provide it as fuzzing oracle to Wasmtime. Due to the changes we refer to the new Wasmi version as Wasmi (register) and the old Wasmi as Wasmi (stack). It might even make sense to have both versions as oracles at the same time because they have very different strengths.

[Robbepop]

With respect to cargo vet:

  wasmi:0.31.1 missing ["safe-to-run"]
  wasmi_arena:0.4.0 missing ["safe-to-run"]
  wasmi_core:0.13.0 missing ["safe-to-run"]
  wasmparser-nostd:0.100.1 missing ["safe-to-run"]

All those crates are maintained by Wasmi maintainers (me).

[github-actions]

Subscribe to Label Action

cc @fitzgen

This issue or pull request has been labeled: "fuzzing"

Thus the following users have been cc'd because of the following labels:

• fitzgen: fuzzing

To subscribe or unsubscribe from this label, edit the .github/subscribe-to-label.json configuration file.

Learn more.

[Robbepop]

I am a bit unsure about the new wasmi::Config handling in diff_wasmi.rs with the mutable Config parameter in WasmiEngine::new. Review & feedback required.
In the changes I made Wasmi now better respects the input config and adjusts its wasmi::Config to it as much as possible and sets the other config flags to false in case Wasmi does not support that feature, yet.

[alexcrichton]

Thanks!

To avoid a new vet of wasmparser-nostd would it be possible to conditionally build wasmi with the official wasmparser crate instead? That way we could get that auto-vetted.

[Robbepop]

Thanks!

To avoid a new vet of wasmparser-nostd would it be possible to conditionally build wasmi with the official wasmparser crate instead? That way we could get that auto-vetted.

Wasmi v0.31.0 is no longer in development so I am not going to support the standard wasmparser for it, however, I am going to think about the implications doing so for future Wasmi versions starting from v0.32.0 once released. Since Wasmi already has a std crate feature it might be simple to just use wasmparser if it is enabled and use wasmparser-nostd otherwise.

I have a wishful thinking for no_std support in the official wasmparser crate one day. :)

[alexcrichton]

Ah ok, in that case this is going to have to hold off until one of us gets a chance to vet the dependencies here. I'm stretched a bit thin at the moment so it may be a bit before I can personally get to this (but others can of course beat me to it)

[Robbepop]

Please tell me if and how I can help with vetting. :)

[alexcrichton]

Reading over some code I think that this impl is not sound as it's supposed to be T: Sync. Additionally though I don't think you should need the unsafe impl at all. You can try changing the PhantomData to either fn() -> Idx or fn(Idx) I think as one of them might require the impl and one might not.

Otherwise the wasmparser-nostd diff is quite large or larger than I remember from the wasmparser diff, so diffing those directories is requiring a good deal of time to go through to verify it's the same. Additionally I haven't even gotten to wasmi yet which glancing at it is quite large and additionally contains a good deal of unsafe code to check.

Currently though wasmi is only used for fuzzing so it doesn't necessarily need a thorough review or strict vetting. Do others have thoughts on whether we should add exemptions for this dependency as opposed to vetting it?

[Robbepop]

Reading over some code I think that this impl is not sound as it's supposed to be T: Sync. Additionally though I don't think you should need the unsafe impl at all. You can try changing the PhantomData to either fn() -> Idx or fn(Idx) I think as one of them might require the impl and one might not.

Thanks for catching this bug! The code is quite old and hasn't seen a lot of love lately.

Otherwise the wasmparser-nostd diff is quite large or larger than I remember from the wasmparser diff, so diffing those directories is requiring a good deal of time to go through to verify it's the same. Additionally I haven't even gotten to wasmi yet which glancing at it is quite large and additionally contains a good deal of unsafe code to check.

The unfortunate truth is that especially with the component model a lot of very no_std unfriendly abstractions such as the IndexMap has been added which required me to come up with my own no_std friendly version of it. Focus was to write it as simple as possible since I never intended this to be a long lasting effort but here we are. In that spirit I think it even lacks tests but wasmparser-nostd passes the wasmparser testsuite so I thought it would be good enough as temporary solution at that time.

Currently though wasmi is only used for fuzzing so it doesn't necessarily need a thorough review or strict vetting. Do others have thoughts on whether we should add exemptions for this dependency as opposed to vetting it?

To provide you with a bit of transparency here about the state of Wasmi:

• Cons:

  • I work on the project alone.
  • There are pretty much no code reviews due to above point.
  • There is a fair amount of unsafe code use, but mostly in the Wasmi bytecode executor.

• Pros:

  • There is a good deal of tests. (unit, integration, e2e, fuzz) (ignoring the indexmap-nostd crate)
  • We do have a very extensive CI (incl. miri)
  • Wasmi v0.31.1 is already used in production by different companies for some months.
  • Wasmi v0.31.1 has received a security audit by SRLabs.
  • Wasmi 0.31.1 is much simpler than the Wasmi v0.32.0-beta.n (on master).

[Robbepop]

I just made this use Wasmi v0.31.1 instead of v0.31.0 which is more explicit. Note that v0.31.0 got yanked some time ago so even previously we already used 0.31.1.

[pchickey]

Do others have thoughts on whether we should add exemptions for this dependency as opposed to vetting it?

I'm happy with adding a cargo vet exemption for wasmi 0.31.1 which notes that its only intended to be used in the context of testing. I don't think there is any real benefit to our project to have a higher bar than wasmi already meets for the sake of a fuzzing oracle.

[fitzgen]

I'm happy with adding a cargo vet exemption for wasmi 0.31.1 which notes that its only intended to be used in the context of testing. I don't think there is any real benefit to our project to have a higher bar than wasmi already meets for the sake of a fuzzing oracle.

Agreed.

[Robbepop]

Reading over some code I think that this impl is not sound as it's supposed to be T: Sync. Additionally though I don't think you should need the unsafe impl at all. You can try changing the PhantomData to either fn() -> Idx or fn(Idx) I think as one of them might require the impl and one might not.

btw. I just published a new wasmi_arena v0.4.1 patch release and yanked v0.4.0.

[alexcrichton]

Ok I've posted #7810 with new vet metadata. If you rebase this PR on top of that once it lands should be good to go

[alexcrichton]

Thanks for the patience while we figure out the vetting, and thanks again for helping us out with the update!

[Robbepop]

Thanks for the patience while we figure out the vetting, and thanks again for helping us out with the update!

Happy to help and thanks for having Wasmi as fuzzing oracle! :)