Skip to content

Commit

Permalink
Run some tests in MIRI on CI (bytecodealliance#6332)
Browse files Browse the repository at this point in the history
* 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
  • Loading branch information
alexcrichton authored May 3, 2023
1 parent 8620aa3 commit c0bb341
Show file tree
Hide file tree
Showing 78 changed files with 707 additions and 178 deletions.
46 changes: 46 additions & 0 deletions .github/workflows/main.yml
Original file line number Diff line number Diff line change
Expand Up @@ -588,6 +588,51 @@ jobs:
env:
GH_TOKEN: ${{ github.token }}

# Run a subset of tests under MIRI on CI to help check the `unsafe` code in
# Wasmtime to make sure it's at least not obviously incorrect for basic usage.
# Note that this doesn't run the full test suite since MIRI can't actually run
# WebAssembly itself at this time (aka it doesn't support a JIT). There are a
# number of annotations throughout the code which gates some tests on MIRI not
# being run.
#
# Note that `cargo nextest` is used here additionally to get parallel test
# execution by default to help cut down on the time in CI.
miri:
needs: determine
if: needs.determine.outputs.run-full
name: Miri
runs-on: ubuntu-latest
env:
CARGO_NEXTEST_VERSION: 0.9.51
steps:
- uses: actions/checkout@v3
with:
submodules: true
- uses: ./.github/actions/install-rust
with:
toolchain: nightly-2023-03-20
- run: rustup component add rust-src miri
- uses: actions/cache@v3
with:
path: ${{ runner.tool_cache }}/cargo-nextest
key: cargo-nextest-bin-${{ env.CARGO_NEXTEST_VERSION }}
- run: echo "${{ runner.tool_cache }}/cargo-nextest/bin" >> $GITHUB_PATH
- run: cargo install --root ${{ runner.tool_cache }}/cargo-nextest --version ${{ env.CARGO_NEXTEST_VERSION }} cargo-nextest
- run: |
cargo miri nextest run -j4 --no-fail-fast \
-p wasmtime \
-p wasmtime-cli \
-p wasmtime-runtime \
-p wasmtime-environ
env:
MIRIFLAGS: -Zmiri-tree-borrows
# common logic to cancel the entire run if this job fails
- run: gh run cancel ${{ github.run_id }}
if: failure() && github.event_name != 'pull_request'
env:
GH_TOKEN: ${{ github.token }}

# Perform release builds of `wasmtime` and `libwasmtime.so`. Builds a variety
# of platforms and architectures and then uploads the release artifacts to
# this workflow run's list of artifacts.
Expand Down Expand Up @@ -694,6 +739,7 @@ jobs:
- meta_deterministic_check
- verify-publish
- determine
- miri
if: always()
steps:
- name: Dump needs context
Expand Down
2 changes: 2 additions & 0 deletions build.rs
Original file line number Diff line number Diff line change
Expand Up @@ -155,6 +155,8 @@ fn write_testsuite_tests(
// Ignore when using QEMU for running tests (limited memory).
if ignore(testsuite, &testname, strategy) {
writeln!(out, "#[ignore]")?;
} else {
writeln!(out, "#[cfg_attr(miri, ignore)]")?;
}

writeln!(
Expand Down
2 changes: 1 addition & 1 deletion cranelift/codegen/src/isa/x64/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -214,7 +214,7 @@ fn isa_constructor(

// Check for compatibility between flags and ISA level
// requested. In particular, SIMD support requires SSE4.2.
if shared_flags.enable_simd() {
if !cfg!(miri) && shared_flags.enable_simd() {
if !isa_flags.has_sse3() || !isa_flags.has_ssse3() || !isa_flags.has_sse41() {
return Err(CodegenError::Unsupported(
"SIMD support requires SSE3, SSSE3, and SSE4.1 on x86_64.".into(),
Expand Down
56 changes: 33 additions & 23 deletions crates/environ/src/tunables.rs
Original file line number Diff line number Diff line change
Expand Up @@ -53,23 +53,26 @@ pub struct Tunables {

impl Default for Tunables {
fn default() -> Self {
let (static_memory_bound, static_memory_offset_guard_size) =
if cfg!(target_pointer_width = "64") {
// 64-bit has tons of address space to static memories can have 4gb
// address space reservations liberally by default, allowing us to
// help eliminate bounds checks.
//
// Coupled with a 2 GiB address space guard it lets us translate
// wasm offsets into x86 offsets as aggressively as we can.
(0x1_0000, 0x8000_0000)
} else if cfg!(target_pointer_width = "32") {
// For 32-bit we scale way down to 10MB of reserved memory. This
// impacts performance severely but allows us to have more than a
// few instances running around.
((10 * (1 << 20)) / crate::WASM_PAGE_SIZE as u64, 0x1_0000)
} else {
panic!("unsupported target_pointer_width");
};
let (static_memory_bound, static_memory_offset_guard_size) = if cfg!(miri) {
// No virtual memory tricks are available on miri so make these
// limits quite conservative.
((1 << 20) / crate::WASM_PAGE_SIZE as u64, 0)
} else if cfg!(target_pointer_width = "64") {
// 64-bit has tons of address space to static memories can have 4gb
// address space reservations liberally by default, allowing us to
// help eliminate bounds checks.
//
// Coupled with a 2 GiB address space guard it lets us translate
// wasm offsets into x86 offsets as aggressively as we can.
(0x1_0000, 0x8000_0000)
} else if cfg!(target_pointer_width = "32") {
// For 32-bit we scale way down to 10MB of reserved memory. This
// impacts performance severely but allows us to have more than a
// few instances running around.
((10 * (1 << 20)) / crate::WASM_PAGE_SIZE as u64, 0x1_0000)
} else {
panic!("unsupported target_pointer_width");
};
Self {
static_memory_bound,
static_memory_offset_guard_size,
Expand All @@ -78,14 +81,21 @@ impl Default for Tunables {
//
// Allocate a small guard to optimize common cases but without
// wasting too much memory.
dynamic_memory_offset_guard_size: 0x1_0000,
dynamic_memory_offset_guard_size: if cfg!(miri) { 0 } else { 0x1_0000 },

// We've got lots of address space on 64-bit so use a larger
// grow-into-this area, but on 32-bit we aren't as lucky.
#[cfg(target_pointer_width = "64")]
dynamic_memory_growth_reserve: 2 << 30, // 2GB
#[cfg(target_pointer_width = "32")]
dynamic_memory_growth_reserve: 1 << 20, // 1MB
// grow-into-this area, but on 32-bit we aren't as lucky. Miri is
// not exactly fast so reduce memory consumption instead of trying
// to avoid memory movement.
dynamic_memory_growth_reserve: if cfg!(miri) {
0
} else if cfg!(target_pointer_width = "64") {
2 << 30 // 2GB
} else if cfg!(target_pointer_width = "32") {
1 << 20 // 1MB
} else {
panic!("unsupported target_pointer_width");
},

generate_native_debuginfo: false,
parse_wasm_debuginfo: true,
Expand Down
3 changes: 3 additions & 0 deletions crates/jit-icache-coherence/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,9 @@ cfg_if::cfg_if! {
if #[cfg(target_os = "windows")] {
mod win;
use win as imp;
} else if #[cfg(miri)] {
mod miri;
use crate::miri as imp;
} else {
mod libc;
use crate::libc as imp;
Expand Down
10 changes: 10 additions & 0 deletions crates/jit-icache-coherence/src/miri.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
use std::ffi::c_void;
use std::io::Result;

pub(crate) fn pipeline_flush_mt() -> Result<()> {
Ok(())
}

pub(crate) fn clear_cache(_ptr: *const c_void, _len: usize) -> Result<()> {
Ok(())
}
3 changes: 3 additions & 0 deletions crates/jit/src/unwind.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,9 @@ cfg_if::cfg_if! {
if #[cfg(all(windows, any(target_arch = "x86_64", target_arch = "aarch64")))] {
mod winx64;
pub use self::winx64::*;
} else if #[cfg(miri)] {
mod miri;
pub use self::miri::*;
} else if #[cfg(unix)] {
mod systemv;
pub use self::systemv::*;
Expand Down
15 changes: 15 additions & 0 deletions crates/jit/src/unwind/miri.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
use anyhow::Result;

pub struct UnwindRegistration {}

impl UnwindRegistration {
pub const SECTION_NAME: &str = ".eh_frame";

pub unsafe fn new(
_base_address: *const u8,
_unwind_info: *const u8,
_unwind_len: usize,
) -> Result<UnwindRegistration> {
Ok(UnwindRegistration {})
}
}
Loading

0 comments on commit c0bb341

Please sign in to comment.