Memcheck for Wasm guests in Wasmtime #6820

ssunkin-fastly · 2023-08-08T17:54:04Z

This PR introduces wmemcheck, a memory checker for a Wasm guest running inside Wasmtime.

wmemcheck operates by recognizing calls to malloc and free inside the Wasm module, and tracking which bytes are valid. It then instruments loads and stores and flags errors when accesses are invalid. This is inspired by the memcheck tool in Valgrind, hence the name.

The functionality is off by default and is enabled with wasmtime run --wmemcheck.

Co-authored-by: Chris Fallin [email protected]
Co-authored-by: iximeow [email protected]

it works!!!!!

cfallin · 2023-08-08T18:18:17Z

For some context for others: Sophia (@ssunkin-fastly) is our summer intern who has written a "Valgrind for Wasm" feature: we want to hook loads, stores, mallocs and frees inside a Wasm guest (presumably written in an unsafe language like C or C++) and catch memory safety violations (use-after-frees, double-frees, out-of-bounds indexing, etc.). The feature works at an MVP level with tests written in C/C++; it's not yet fully general (multi-memories, arbitrary annotations for custom allocators, etc) but we want to merge the MVP first and then file followup issues.

@ssunkin-fastly: would you mind either rebasing or merging main to fix the merge conflicts here? (Happy to help if needed.)

cfallin

This is looking really great! I have a whole bunch of minor comments but the overall shape of the code is good and we should be able to refine things fairly quickly, I think.

I know you're working on doc-comments but will note in particular that our CI checks will fail until there's a doc-comment on every public type, enum, function; it might feel a little tedious but it's good practice :-)

It'd be good to file some followup GitHub issues and refer to issue numbers where appropriate in comments as well. Off the top of my head:

Support for multiple memories
Support for instrumenting SIMD loads/stores
Support for handling different memory layouts (assume whole initial memory size is valid, for example)
Option to print valgrind errors and continue (as the "real Valgrind" does) rather than trap

and anything else on your to-do list you want to capture.

Happy to pair on any of the requests here if unclear!

Cargo.toml

cranelift/wasm/src/code_translator.rs

cranelift/wasm/src/environ/spec.rs

cranelift/wasm/src/sections_translator.rs

crates/cranelift/Cargo.toml

crates/valgrind/fuzz/fuzz_targets/buggy_accesses.rs

cfallin · 2023-08-08T18:47:50Z

crates/valgrind/src/lib.rs

+    metadata: Vec<MemState>,
+    mallocs: HashMap<usize, usize>,
+    pub stack_pointer: usize,
+    max_stack_size: usize,


To reduce ambiguity I think this should be called stack_end or something similar? ("Stack size" makes me think of the actual size, so a stack from 1MiB down to 1MiB - 4KiB is 4KiB large; but the "end" is 1MiB)

cfallin · 2023-08-08T18:49:33Z

crates/valgrind/src/lib.rs

@@ -0,0 +1,417 @@
+use std::cmp::*;


Let's add a doc-comment at the top of this module like

//! add some docs here //! //! in markdown format, with multiple paragraphs as desired //! //! like this

documenting the assumptions about memory layout here.

cfallin · 2023-08-08T18:50:23Z

crates/valgrind/src/lib.rs

+            });
+        } else if new_sp < self.stack_pointer {
+            for i in new_sp..self.stack_pointer + 1 {
+                // +1 to account for sp == max_stack_size (?)


I'm somewhat surprised this is needed -- could we document the case where it is if so (example addresses or whatnot)?

cfallin · 2023-08-08T18:51:25Z

src/commands/run.rs

@@ -253,6 +253,10 @@ pub struct RunCommand {
    /// memory, for example.
    #[clap(long)]
    trap_on_grow_failure: bool,
+
+    /// Run valgrind


We'll want a much longer doc-comment of course, probably with a pointer to documentation for more details on how to use!

cfallin · 2023-08-14T18:22:40Z

@uweigand we seem to be getting an s390x-specific qemu error

../tcg/tcg.c:3244: tcg fatal error
qemu: uncaught target signal 6 (Aborted) - core dumped
error: test failed, to rerun pass `-p wasmtime-cli --test all`

in CI -- any thoughts?

cfallin · 2023-08-15T01:31:07Z

(Trying again in case the qemu/tcg crash was spurious)

uweigand · 2023-08-15T13:06:40Z

This does indeed appear to be a qemu bug. Running the full testsuite natively with this patch works fine.

When running under 7.2.0 qemu on Intel, I can reproduce the same TCG abort. It seems this a known problem fixed by this patch by @iii-i :
https://lists.gnu.org/archive/html/qemu-devel/2023-03/msg04880.html
which went into qemu 8.0.

Backporting that fix makes the abort go away. Not sure why this particular patch exposed this bug - but that may just be some random effect of qemu register allocation ...

This should resolve a bug in qemu triggered by changes in bytecodealliance#6820 (see [here](bytecodealliance#6820 (comment))).

* CI: upgrade to qemu 8.0.4. This should resolve a bug in qemu triggered by changes in #6820 (see [here](#6820 (comment))). * ci: Update QEMU patch for 8.0.2 --------- Co-authored-by: Afonso Bordado <[email protected]>

…acking

* CI: upgrade to qemu 8.0.4. This should resolve a bug in qemu triggered by changes in bytecodealliance#6820 (see [here](bytecodealliance#6820 (comment))). * ci: Update QEMU patch for 8.0.2 --------- Co-authored-by: Afonso Bordado <[email protected]>

* attempt at inserting things where i think they might belong + questions * entry hook + questions * commented out all changes, doc comment errors * fix doc comment * libcalls build now!!!! * initial check_malloc_exit setup * WIP: load/store hooks * hooks added + building * added valgrind library * made wasm-valgrind accessible in wasmtime * check_malloc filled in... * move valgrind_state to an appropriate part of instance it works!!!!! * yay it's working! (?) i think?? * stack tracing in progress * errors + num bytes displayed * initial valgrind configuration * valgrind conditional some warnings fixed * conditional compilation + CLI flag finished * panic!() changed to bail!() * started adding doc comments * added memory grow hook + fixed access size handling * removed test.wasm * removed malloc_twice.wat * doc comments in spec.rs * pr feedback addressed * ran cargo fmt * addressing more feedback * Remove fuzz crate from wmemcheck. * Review feedback and test fix. * add wasmtime-wmemcheck crate to publish allowlist. * fix build without compiler features * reorder crates in publish list * Add trampolines for libcalls on s390x. * Make wasmtime-wmemcheck dep an exact version requirement. --------- Co-authored-by: iximeow <[email protected]> Co-authored-by: Chris Fallin <[email protected]> Co-authored-by: iximeow <[email protected]>

…time into feature/wasi-nn-preview-2 * 'feature/wasi-nn-preview-2' of github.com:geekbeast/wasmtime: Memcheck for Wasm guests in Wasmtime (bytecodealliance#6820) CI: upgrade to qemu 8.0.4. (bytecodealliance#6849) Sync wasi-cli with wit definitions in standards repo (bytecodealliance#6806) Rename `preview2::preview2` to `preview2::host` (bytecodealliance#6847) winch: Simplify the MacroAssembler and Assembler interfaces (bytecodealliance#6841) There are no files in `preview1` other than `mod.rs` (bytecodealliance#6845) Update stdio on Unix to fall back to worker threads (bytecodealliance#6833) Update RELEASES.md (bytecodealliance#6838) Minor documentation updates to docs/WASI-tutorial.md (bytecodealliance#6839) Add support for vector in DataValueExt::int() (bytecodealliance#6844)

* CI: upgrade to qemu 8.0.4. This should resolve a bug in qemu triggered by changes in bytecodealliance#6820 (see [here](bytecodealliance#6820 (comment))). * ci: Update QEMU patch for 8.0.2 --------- Co-authored-by: Afonso Bordado <[email protected]>

* Fix test expectations [automatically-tag-and-release-this-commit] * CI: upgrade to qemu 8.0.4. (#6849) * CI: upgrade to qemu 8.0.4. This should resolve a bug in qemu triggered by changes in #6820 (see [here](#6820 (comment))). * ci: Update QEMU patch for 8.0.2 --------- Co-authored-by: Afonso Bordado <[email protected]> --------- Co-authored-by: Chris Fallin <[email protected]> Co-authored-by: Afonso Bordado <[email protected]>

* Fix test expectations [automatically-tag-and-release-this-commit] * CI: upgrade to qemu 8.0.4. (bytecodealliance#6849) * CI: upgrade to qemu 8.0.4. This should resolve a bug in qemu triggered by changes in bytecodealliance#6820 (see [here](bytecodealliance#6820 (comment))). * ci: Update QEMU patch for 8.0.2 --------- Co-authored-by: Afonso Bordado <[email protected]> --------- Co-authored-by: Chris Fallin <[email protected]> Co-authored-by: Afonso Bordado <[email protected]>

ssunkin-fastly and others added 23 commits June 29, 2023 14:23

attempt at inserting things where i think they might belong + questions

7689549

entry hook + questions

3402c15

commented out all changes, doc comment errors

b999de6

fix doc comment

77d72b2

libcalls build now!!!!

ebda2a2

initial check_malloc_exit setup

f962078

WIP: load/store hooks

18389d7

hooks added + building

4e713b1

added valgrind library

2586e24

made wasm-valgrind accessible in wasmtime

e4d7a0c

check_malloc filled in...

adde43c

move valgrind_state to an appropriate part of instance

13035b1

it works!!!!!

yay it's working! (?) i think??

759b129

stack tracing in progress

efd9343

errors + num bytes displayed

c94c0f7

initial valgrind configuration

74990f2

valgrind conditional some warnings fixed

6e89201

conditional compilation + CLI flag finished

59ff0aa

panic!() changed to bail!()

8f5df30

started adding doc comments

e2d929c

added memory grow hook + fixed access size handling

3e9e823

removed test.wasm

0bb2290

removed malloc_twice.wat

fe3db2a

cfallin reviewed Aug 8, 2023

View reviewed changes

doc comments in spec.rs

c108c5f

github-actions bot added cranelift Issues related to the Cranelift code generator cranelift:wasm wasmtime:api Related to the API of the `wasmtime` crate itself wasmtime:config Issues related to the configuration of Wasmtime labels Aug 8, 2023

cfallin enabled auto-merge August 14, 2023 15:38

cfallin added this pull request to the merge queue Aug 14, 2023

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Aug 14, 2023

cfallin added this pull request to the merge queue Aug 14, 2023

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Aug 14, 2023

Make wasmtime-wmemcheck dep an exact version requirement.

7db34f6

cfallin enabled auto-merge August 14, 2023 16:32

cfallin added this pull request to the merge queue Aug 14, 2023

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Aug 14, 2023

cfallin added this pull request to the merge queue Aug 15, 2023

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Aug 15, 2023

cfallin added a commit to cfallin/wasmtime that referenced this pull request Aug 15, 2023

CI: upgrade to qemu 8.0.4.

d19115b

This should resolve a bug in qemu triggered by changes in bytecodealliance#6820 (see [here](bytecodealliance#6820 (comment))).

cfallin mentioned this pull request Aug 15, 2023

CI: upgrade to qemu 8.0.4. #6849

Merged

Merge remote-tracking branch 'upstream/main' into wasmtime-valgrind-h…

8a4a8d4

…acking

cfallin added this pull request to the merge queue Aug 16, 2023

Merged via the queue into bytecodealliance:main with commit ca5a9db Aug 16, 2023

cfallin deleted the wasmtime-valgrind-hacking branch August 16, 2023 04:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Memcheck for Wasm guests in Wasmtime #6820

Memcheck for Wasm guests in Wasmtime #6820

ssunkin-fastly commented Aug 8, 2023 •

edited by cfallin

Loading

cfallin commented Aug 8, 2023

cfallin left a comment

cfallin Aug 8, 2023

cfallin Aug 8, 2023

cfallin Aug 8, 2023

cfallin Aug 8, 2023

cfallin commented Aug 14, 2023

cfallin commented Aug 15, 2023

uweigand commented Aug 15, 2023

Memcheck for Wasm guests in Wasmtime #6820

Memcheck for Wasm guests in Wasmtime #6820

Conversation

ssunkin-fastly commented Aug 8, 2023 • edited by cfallin Loading

cfallin commented Aug 8, 2023

cfallin left a comment

Choose a reason for hiding this comment

cfallin Aug 8, 2023

Choose a reason for hiding this comment

cfallin Aug 8, 2023

Choose a reason for hiding this comment

cfallin Aug 8, 2023

Choose a reason for hiding this comment

cfallin Aug 8, 2023

Choose a reason for hiding this comment

cfallin commented Aug 14, 2023

cfallin commented Aug 15, 2023

uweigand commented Aug 15, 2023

ssunkin-fastly commented Aug 8, 2023 •

edited by cfallin

Loading