Run some tests in MIRI on CI #6332

alexcrichton · 2023-05-02T22:01:09Z

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 unsafe code in Wasmtime's runtime.

Under MIRI's default settings, which is to use the Stacked Borrows model, much of the code in Instance and VMContext is considered invalid. Under the optional Tree Borrows model, however, this same code is accepted. After some extremely helpful discussion 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 are unimplemented!() 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 of wasmtime_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 a Store<T> is now done with a Wasmtime-specific StoreBox<T> type. This is because, with MIRI's analyses, moving a Box<T> invalidates all pointers derived from this Box<T>. We don't want these semantics, so we effectively have a custom Box<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 unsafe code 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!

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 `unsafe` code in Wasmtime's runtime. Under MIRI's default settings, which is to use the [Stacked Borrows][stacked] model, much of the code in `Instance` and `VMContext` is 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 are `unimplemented!()` 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 of `wasmtime_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 a `Store<T>` is now done with a Wasmtime-specific `StoreBox<T>` type. This is because, with MIRI's analyses, moving a `Box<T>` invalidates all pointers derived from this `Box<T>`. We don't want these semantics, so we effectively have a custom `Box<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 `unsafe` code 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

github-actions · 2023-05-02T22:44:53Z

Subscribe to Label Action

cc @peterhuene

This issue or pull request has been labeled: "cranelift", "cranelift:area:x64", "wasmtime:api", "wasmtime:config"

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

peterhuene: wasmtime:api

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

Learn more.

github-actions · 2023-05-02T22:45:15Z

Label Messager: wasmtime:config

It looks like you are changing Wasmtime's configuration options. Make sure to
complete this check list:

If you added a new Config method, you wrote extensive documentation for
it.

Our documentation should be of the following form:

Short, simple summary sentence.

More details. These details can be multiple paragraphs. There should be
information about not just the method, but its parameters and results as
well.

Is this method fallible? If so, when can it return an error?

Can this method panic? If so, when does it panic?

# Example

Optional example here.

If you added a new Config method, or modified an existing one, you
ensured that this configuration is exercised by the fuzz targets.

For example, if you expose a new strategy for allocating the next instance
slot inside the pooling allocator, you should ensure that at least one of our
fuzz targets exercises that new strategy.

Often, all that is required of you is to ensure that there is a knob for this
configuration option in wasmtime_fuzzing::Config (or one
of its nested structs).

Rarely, this may require authoring a new fuzz target to specifically test this
configuration. See our docs on fuzzing for more details.
If you are enabling a configuration option by default, make sure that it
has been fuzzed for at least two weeks before turning it on by default.

To modify this label's message, edit the .github/label-messager/wasmtime-config.md file.

To add new label messages or remove existing label messages, edit the
.github/label-messager.json configuration file.

Learn more.

This pulls in Kerollmops/slice-group-by#20 which is necessary to get Cranelift "clean" in MIRI with Stacked Borrows. I plan on leveraging this in a subsequent commit to bytecodealliance#6332 which turns on Stacked Borrows for Wasmtime, but currently it fails due to this transitive dependency of Cranelift, hence the update.

This pulls in Kerollmops/slice-group-by#20 which is necessary to get Cranelift "clean" in MIRI with Stacked Borrows. I plan on leveraging this in a subsequent commit to #6332 which turns on Stacked Borrows for Wasmtime, but currently it fails due to this transitive dependency of Cranelift, hence the update.

fitzgen

Ship it! 🚀

fitzgen · 2023-05-03T16:01:45Z

crates/runtime/src/component.rs

-#[cfg_attr(target_pointer_width = "32", repr(align(4)))]
-#[cfg_attr(target_pointer_width = "64", repr(align(8)))]
+#[repr(align(16))]


Why this change? Does the comment just above need updating to explain it?

It turns out that without this the VMContext wasn'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.

fitzgen · 2023-05-03T16:09:00Z

crates/runtime/src/traphandlers.rs

+        struct WasmtimeLongjmp;
+
+        unsafe extern "C" fn wasmtime_setjmp(
+            _jmp_buf: *mut *const u8,
+            callback: extern "C" fn(*mut u8, *mut VMContext),
+            payload: *mut u8,
+            callee: *mut VMContext,
+        ) -> i32 {
+            use std::panic::{self, AssertUnwindSafe};
+            let result = panic::catch_unwind(AssertUnwindSafe(|| {
+                callback(payload, callee);
+            }));
+            match result {
+                Ok(()) => 1,
+                Err(e) => {
+                    if e.is::<WasmtimeLongjmp>() {
+                        0
+                    } else {
+                        panic::resume_unwind(e)
+                    }
+                }
+            }
+        }
+
+        unsafe extern "C" fn wasmtime_longjmp(_jmp_buf: *const u8) -> ! {
+            std::panic::panic_any(WasmtimeLongjmp)
+        }


* Run some tests in MIRI on CI 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 `unsafe` code in Wasmtime's runtime. Under MIRI's default settings, which is to use the [Stacked Borrows][stacked] model, much of the code in `Instance` and `VMContext` is 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 are `unimplemented!()` 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 of `wasmtime_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 a `Store<T>` is now done with a Wasmtime-specific `StoreBox<T>` type. This is because, with MIRI's analyses, moving a `Box<T>` invalidates all pointers derived from this `Box<T>`. We don't want these semantics, so we effectively have a custom `Box<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 `unsafe` code 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 * Update alignment comment

alexcrichton requested review from a team as code owners May 2, 2023 22:01

alexcrichton requested review from fitzgen and removed request for a team May 2, 2023 22:01

github-actions bot added cranelift Issues related to the Cranelift code generator cranelift:area:x64 Issues related to x64 codegen wasmtime:api Related to the API of the `wasmtime` crate itself wasmtime:config Issues related to the configuration of Wasmtime labels May 2, 2023

alexcrichton mentioned this pull request May 3, 2023

Update the slice-group-by dependency #6335

Merged

fitzgen approved these changes May 3, 2023

View reviewed changes

Update alignment comment

1c13b19

alexcrichton enabled auto-merge May 3, 2023 16:52

alexcrichton added this pull request to the merge queue May 3, 2023

github-merge-queue bot removed this pull request from the merge queue due to no response for status checks May 3, 2023

alexcrichton added this pull request to the merge queue May 3, 2023

Merged via the queue into bytecodealliance:main with commit c0bb341 May 3, 2023

alexcrichton deleted the miri branch May 3, 2023 21:45

alexcrichton mentioned this pull request May 3, 2023

Make Wasmtime compatible with Stacked Borrows in MIRI #6338

Merged

Dante-Broggi mentioned this pull request May 4, 2023

Fix typo 'oeprations' ⇒ 'operations' #6339

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Run some tests in MIRI on CI #6332

Run some tests in MIRI on CI #6332

alexcrichton commented May 2, 2023

github-actions bot commented May 2, 2023

github-actions bot commented May 2, 2023

fitzgen left a comment

fitzgen May 3, 2023

alexcrichton May 3, 2023

fitzgen May 3, 2023

Run some tests in MIRI on CI #6332

Run some tests in MIRI on CI #6332

Conversation

alexcrichton commented May 2, 2023

github-actions bot commented May 2, 2023

Subscribe to Label Action

github-actions bot commented May 2, 2023

Label Messager: wasmtime:config

fitzgen left a comment

Choose a reason for hiding this comment

fitzgen May 3, 2023

Choose a reason for hiding this comment

alexcrichton May 3, 2023

Choose a reason for hiding this comment

fitzgen May 3, 2023

Choose a reason for hiding this comment