Stream: git-wasmtime

Topic: wasmtime / PR #2858 Bring back `Module::deserialize`

Wasmtime GitHub notifications bot (Apr 26 2021 at 14:51):

alexcrichton opened PR #2858 from bring-back-deserialize to main:

I thought I was being clever suggesting that Module::deserialize was
removed from #2791 by funneling all module constructors into
Module::new. As our studious fuzzers have found, though, this means
that Module::new is not safe currently to pass arbitrary user-defined
input into. Now one might pretty reasonable expect to be able to do
that, however, being a WebAssembly engine and all. This PR as a result
separates the deserialize part of Module::new back into
Module::deserialize.

This means that binary blobs created with Module::serialize and
Engine::precompile_module will need to be passed to
Module::deserialize to "rehydrate" them back into a Module. This
restores the property that it should be safe to pass arbitrary input to
Module::new since it's always expected to be a wasm module. This also
means that fuzzing will no longer attempt to fuzz Module::deserialize
which isn't something we want to do anyway.

<!--

Please ensure that the following steps are all taken care of before submitting
the PR.

• [ ] This has been discussed in issue #..., or if not, please tell us why
  here.

• [ ] A short description of what this does, why it is needed; if the
  description becomes long, the matter should probably be discussed in an issue
  first.

• [ ] This PR contains test cases, if meaningful.

• [ ] A reviewer from the core maintainer team has been assigned for this PR.
  If you don't know who could review this, please indicate so. The list of
  suggested reviewers on the right can help you.

Please ensure all communication adheres to the code of conduct.
-->

Wasmtime GitHub notifications bot (Apr 26 2021 at 14:51):

alexcrichton requested peterhuene for a review on PR #2858.

Wasmtime GitHub notifications bot (Apr 26 2021 at 15:45):

alexcrichton updated PR #2858 from bring-back-deserialize to main.

Wasmtime GitHub notifications bot (Apr 26 2021 at 16:42):

fitzgen submitted PR Review.

Wasmtime GitHub notifications bot (Apr 26 2021 at 16:42):

fitzgen submitted PR Review.

Wasmtime GitHub notifications bot (Apr 26 2021 at 16:42):

fitzgen created PR Review Comment:

This is not really convincing me that this should _not_ be marked unsafe. It seems like this is describing the contract one must uphold for it to be safe, that it must have be some wasmtime-compiled blob, and the whole point of unsafe is encapsulating such external-to-the-type-system safety invariants.

Wasmtime GitHub notifications bot (Apr 26 2021 at 17:49):

alexcrichton submitted PR Review.

Wasmtime GitHub notifications bot (Apr 26 2021 at 17:49):

alexcrichton created PR Review Comment:

I was originally going to mark this unsafe, but the impact on other language bindings gave me hesitation. Languages like Python expect everything to be safe, so this sort of documentation would inevitably end up there as well. Or otherwise this would need to show up everywhere anyway (basically the unsafe in Rust doesn't help any users anywhere else).

In the end I thought of this along the lines of File::open and /dev/self/mem (or whatever it's called). Technically it's unsafe but practically it's never used that way.

Wasmtime GitHub notifications bot (Apr 26 2021 at 19:38):

peterhuene submitted PR Review.

Wasmtime GitHub notifications bot (Apr 26 2021 at 19:38):

peterhuene submitted PR Review.

Wasmtime GitHub notifications bot (Apr 26 2021 at 19:38):

peterhuene created PR Review Comment:

    /// It's important to note that this is somewhat `unsafe` but is not marked

Wasmtime GitHub notifications bot (Apr 26 2021 at 19:44):

peterhuene submitted PR Review.

Wasmtime GitHub notifications bot (Apr 26 2021 at 19:44):

peterhuene created PR Review Comment:

I could go either way on this issue; it's unsafe for arbitrary input but safe for all desired use cases (i.e. rehydrating precompiled/serialized output). Either way (marked unsafe or not) we'll want the big, bold caveat of "it's the caller's responsibility to ensure integrity of the serialized data before calling this method".

Wasmtime GitHub notifications bot (Apr 26 2021 at 20:02):

fitzgen submitted PR Review.

Wasmtime GitHub notifications bot (Apr 26 2021 at 20:02):

fitzgen created PR Review Comment:

I guess I would prefer to mark it as unsafe and also propagate that warning about "only use this with files you created via wasmtime directly" out to the language embeddings too.

In the end I thought of this along the lines of File::open and /dev/self/mem (or whatever it's called). Technically it's unsafe but practically it's never used that way.

I see the argument, but I think this is a different category of thing: with /dev/self/mem we have a :sparkles: magical :sparkles: file, but with Module::deserialize regular old files can trip us up if they have the right shape.

Wasmtime GitHub notifications bot (Apr 27 2021 at 14:34):

alexcrichton submitted PR Review.

Wasmtime GitHub notifications bot (Apr 27 2021 at 14:34):

alexcrichton created PR Review Comment:

Hm ok I'll go with unsafe and we can see how it goes.

Wasmtime GitHub notifications bot (Apr 27 2021 at 14:42):

alexcrichton updated PR #2858 from bring-back-deserialize to main.

Wasmtime GitHub notifications bot (Apr 27 2021 at 15:55):

alexcrichton merged PR #2858.

Last updated: Oct 23 2024 at 20:03 UTC