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
thatModule::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 thedeserialize
part ofModule::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 aModule
. 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 fuzzModule::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.
-->
alexcrichton requested peterhuene for a review on PR #2858.
alexcrichton updated PR #2858 from bring-back-deserialize
to main
.
fitzgen submitted PR Review.
fitzgen submitted PR Review.
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 ofunsafe
is encapsulating such external-to-the-type-system safety invariants.
alexcrichton submitted PR Review.
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 theunsafe
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.
peterhuene submitted PR Review.
peterhuene submitted PR Review.
peterhuene created PR Review Comment:
/// It's important to note that this is somewhat `unsafe` but is not marked
peterhuene submitted PR Review.
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".
fitzgen submitted PR Review.
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 withModule::deserialize
regular old files can trip us up if they have the right shape.
alexcrichton submitted PR Review.
alexcrichton created PR Review Comment:
Hm ok I'll go with
unsafe
and we can see how it goes.
alexcrichton updated PR #2858 from bring-back-deserialize
to main
.
alexcrichton merged PR #2858.
Last updated: Dec 23 2024 at 12:05 UTC