Wasmtime GitHub notifications bot (May 02 2023 at 22:01): Wasmtime GitHub notifications bot (May 02 2023 at 22:01):alexcrichton requested fitzgen for a review on PR #6332.
Wasmtime GitHub notifications bot (May 02 2023 at 22:01):alexcrichton requested wasmtime-compiler-reviewers for a review on PR #6332.
Wasmtime GitHub notifications bot (May 02 2023 at 22:01):alexcrichton opened PR #6332 from alexcrichton:miri to bytecodealliance:main:
This commit is an implementation of getting at least chunks of Wasmtime to run in MIRI on CI. The full test suite is not possible to run in MIRI because MIRI cannot run Cranelift-produced code at runtime (aka it doesn't support JITs). Running MIRI, however, is still quite valuable if we can manage it because it would have trivially detected GHSA-ch89-5g45-qwc7, our most recent security advisory. The goal of this PR is to select a subset of the test suite to execute in CI under MIRI and boost our confidence in the copious amount of
unsafecode in Wasmtime's runtime.Under MIRI's default settings, which is to use the [Stacked Borrows][stacked] model, much of the code in
InstanceandVMContextis considered invalid. Under the optional [Tree Borrows][tree] model, however, this same code is accepted. After some [extremely helpful discussion][discuss] on the Rust Zulip my current conclusion is that what we're doing is not fundamentally un-sound but we need to model it in a different way. This PR, however, uses the Tree Borrows model for MIRI to get something onto CI sooner rather than later, and I hope to follow this up with something that passed Stacked Borrows. Additionally that'll hopefully make this diff smaller and easier to digest.Given all that, the end result of this PR is to get 131 separate unit tests executing on CI. These unit tests largely exercise the embedding API where wasm function compilation is not involved. Some tests compile wasm functions but don't run them, but compiling wasm through Cranelift in MIRI is so slow that it doesn't seem worth it at this time. This does mean that there's a pretty big hole in MIRI's test coverage, but that's to be expected as we're a JIT compiler after all.
To get tests working in MIRI this PR uses a number of strategies:
When platform-specific code is involved there's now
#[cfg(miri)]for MIRI's version. For example there's a custom-built "mmap" just for MIRI now. Many of these are simple noops, some areunimplemented!()as they shouldn't be reached, and some are slightly nontrivial implementations such as mmaps and trap handling (for native-to-native function calls).Many test modules are simply excluded via
#![cfg(not(miri))]at the top of the file. This excludes the entire module's worth of tests from MIRI. Other modules have#[cfg_attr(miri, ignore)]annotations to ignore tests by default on MIRI. The latter form is used in modules where some tests work and some don't. This means that all future test additions will need to be effectively annotated whether they work in MIRI or not. My hope though is that there's enough precedent in the test suite of what to do to not cause too much burden.A number of locations are fixed with respect to MIRI's analysis. For example
ComponentInstance, the component equivalent ofwasmtime_runtime::Instance, was actually left out from the fix for the CVE by accident. MIRI dutifully highlighted the issues here and I've fixed them locally. Some locations fixed for MIRI are changed to something that looks similar but is subtly different. For example retaining items in aStore<T>is now done with a Wasmtime-specificStoreBox<T>type. This is because, with MIRI's analyses, moving aBox<T>invalidates all pointers derived from thisBox<T>. We don't want these semantics, so we effectively have a customBox<T>to suit our needs in this regard.Some default configuration is different under MIRI. For example most linear memories are dynamic with no guards and no space reserved for growth. Settings such as parallel compilation are disabled. These are applied to make MIRI "work by default" in more places ideally. Some tests which perform N iterations of something perform fewer iterations on MIRI to not take quite so long.
This PR is not intended to be a one-and-done-we-never-look-at-it-again kind of thing. Instead this is intended to lay the groundwork to continuously run MIRI in CI to catch any soundness issues. This feels, to me, overdue given the amount of
unsafecode inside of Wasmtime. My hope is that over time we can figure out how to run Wasm in MIRI but that may take quite some time. Regardless this will be adding nontrivial maintenance work to contributors to Wasmtime. MIRI will be run on CI for merges, MIRI will have test failures when everything else passes, MIRI's errors will need to be deciphered by those who have probably never run MIRI before, things like that. Despite all this to me it seems worth the cost at this time. Just getting this running caught two possible soundness bugs in the component implementation that could have had a real-world impact in the future![stacked]: https://github.com/rust-lang/unsafe-code-guidelines/blob/master/wip/stacked-borrows.md
[tree]: https://perso.crans.org/vanille/treebor/
[discuss]: https://rust-lang.zulipchat.com/#narrow/stream/269128-miri/topic/Tree.20vs.20Stacked.20Borrows.20.26.20a.20debugging.20question<!--
Please make sure you include the following information:
If this work has been discussed elsewhere, please include a link to that
conversation. If it was discussed in an issue, just mention "issue #...".Explain why this change is needed. If the details are in an issue already,
this can be brief.Our development process is documented in the Wasmtime book:
https://docs.wasmtime.dev/contributing-development-process.htmlPlease ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->
Wasmtime GitHub notifications bot (May 02 2023 at 22:01):alexcrichton requested wasmtime-core-reviewers for a review on PR #6332.
Wasmtime GitHub notifications bot (May 02 2023 at 22:01):alexcrichton requested wasmtime-default-reviewers for a review on PR #6332.
Wasmtime GitHub notifications bot (May 03 2023 at 16:14):fitzgen submitted PR review:
Ship it! :rocket:
Wasmtime GitHub notifications bot (May 03 2023 at 16:14):fitzgen submitted PR review:
Ship it! :rocket:
Wasmtime GitHub notifications bot (May 03 2023 at 16:14):fitzgen created PR review comment:
lol nice
Wasmtime GitHub notifications bot (May 03 2023 at 16:14):fitzgen created PR review comment:
Why this change? Does the comment just above need updating to explain it?
Wasmtime GitHub notifications bot (May 03 2023 at 16:51):alexcrichton created PR review comment:
It turns out that without this the
VMContextwasn't properly aligned so MIRI was indicating that under-aligned writes were happening (oh dear!). The comments and annotations here predate all that but yeah I should update things here.
Wasmtime GitHub notifications bot (May 03 2023 at 16:52):alexcrichton updated PR #6332.
Wasmtime GitHub notifications bot (May 03 2023 at 16:52):alexcrichton has enabled auto merge for PR #6332.
Wasmtime GitHub notifications bot (May 03 2023 at 21:45):alexcrichton merged PR #6332.
Last updated: Nov 22 2024 at 16:03 UTC