cr0sh opened PR #5342 from run-piped-modules
to main
:
Previously, wasmtime-cli checked the module to be loaded is
precompiled or not, by pre-opening the given file path to
check if the "\x7FELF" header exists.
This commit moves this branch into theModule::from_trusted_file
,
which is only invoked with--allow-precompiled
flag on CLI.The initial motivation of the commit is, feeding a module to wasmtime
from piped inputs, is blocked by the pre-opening of the module.
TheModule::from_trusted_file
, assumes the --allow-precompiled flag
so there is no piped inputs, happily mmap-ing the module to test
if the header exists.
If --allow-precompiled is not supplied, the existingModule::from_file
will be used, without the additional header check as the precompiled
modules are intentionally not allowed on piped inputs for security measures.One caveat of this approach is that the user may be confused if
he or she tries to execute a precompiled module without
--allow-precompiled, as wasmtime shows an 'input bytes aren't valid
utf-8' error, not directly getting what's going wrong.
So this commit includes a hack-ish workaround for this: If the error
onModule::new
ends with "input bytes aren't valid utf-8" strings,
show a simple note to the standard error stream.This might be a small hiccup on preparing i18n or changing error
format on thewat
crate, but I feel comfortable (yet) this strategy
because thewat
crate includes tests for the error messages, so one
would notice the breakage if the error message have changed.Thanks to @jameysharp for suggesting this idea with a detailed guidance.
Note: this supercedes #5035.
jameysharp submitted PR review.
jameysharp created PR review comment:
As a matter of style, let's avoid suggesting anything about the mental health of our users. Here's an alternative, with more context copied from the documentation for
deserialize_file
, which has the same sources of unsafety. Can you think of anything this is missing that callers need to know?/// Creates a new WebAssembly `Module` from the contents of the given `file` /// on disk, but with assumptions that the file is from a trusted source. /// The file should be a binary- or text-format WebAssembly module, or a /// precompiled artifact generated by the same version of Wasmtime. /// /// # Unsafety /// /// All of the reasons that [`deserialize`] is `unsafe` apply to this /// function as well. Arbitrary data loaded from a file may trick Wasmtime /// into arbitrary code execution since the contents of the file are not /// validated to be a valid precompiled module. /// /// [`deserialize`]: Module::deserialize /// /// Additionally though this function is also `unsafe` because the file /// referenced must remain unchanged and a valid precompiled module for the /// entire lifetime of the [`Module`] returned. Any changes to the file on /// disk may change future instantiations of the module to be incorrect. /// This is because the file is mapped into memory and lazily loaded pages /// reflect the current state of the file, not necessarily the origianl /// state of the file.
jameysharp created PR review comment:
I think you meant
--allow-precompiled
here:eprintln!("note: wasmtime might be trying to load a precompiled binary without --allow-precompiled.");
jameysharp created PR review comment:
Unfortunately, things changed underneath you (in #5153), so
SerializedModule
doesn't exist any more. Fortunately, I think this is an easy fix. Incrates/wasmtime/src/engine.rs
, makefn load_code
bepub(crate)
. Then here you can callengine.load_code(mmap, ObjectKind::Module)
. Seedeserialize
anddeserialize_file
for examples of the current pattern.
jameysharp submitted PR review.
jameysharp created PR review comment:
Out of curiosity, is the
*
necessary here? I think if you write&mmap
, Rust will still figure out that you want the dereferenced type. But I think the&
is necessary either way.
bjorn3 submitted PR review.
bjorn3 created PR review comment:
I don't think the wasmtime library should print to stderr. There is no way for a binary to suppress this.
bjorn3 created PR review comment:
It needs a type implementing
AsRef<[u8]>
as argument.&MmapVec
doesn't implement this.MmapVec
only implementsDeref
andDerefMut
.
bjorn3 submitted PR review.
cr0sh submitted PR review.
cr0sh created PR review comment:
Oops, I forgot to
cargo check
again after rebasing. I'll fix eod today
cr0sh edited PR review comment.
cr0sh submitted PR review.
cr0sh created PR review comment:
You're right, wasmtime is a library so eprintln should be avoided if possible. How about deferring this eprintln to the wasmtime-cli? This would make wasmtime-cli add
wat
dependency to downcast error.
Also the#[cfg(feature = "wat")]
branch condition cannot be used on wasmtime-cli, so this eprintln would be fired unconditionally.
cr0sh submitted PR review.
cr0sh created PR review comment:
Great! Didn't think the scenario that the file would be modified after mmaping. Nice catch.
cr0sh updated PR #5342 from run-piped-modules
to main
.
jameysharp submitted PR review.
jameysharp created PR review comment:
Let's keep the error reporting simple here. If somebody has a more user-friendly way to handle this, we can do that in a later PR.
Module::from_file(engine, path).context("if you're trying to run a precompiled module, pass --allow-precompiled")
jameysharp submitted PR review.
jameysharp created PR review comment:
I've just suggested a different way to report this error, so let's delete this
eprintln
in favor of that alternative.
cr0sh submitted PR review.
cr0sh created PR review comment:
Awesome.
cr0sh updated PR #5342 from run-piped-modules
to main
.
cr0sh updated PR #5342 from run-piped-modules
to main
.
cr0sh submitted PR review.
cr0sh created PR review comment:
This is done and I found a small typo near load_code so fixed it too ;)
jameysharp created PR review comment:
Maybe GitHub is confusing me here. It looks like you applied this suggestion, then force-pushed a version that didn't have this suggestion any more. Could you add it again?
jameysharp submitted PR review.
cr0sh updated PR #5342 from run-piped-modules
to main
.
cr0sh submitted PR review.
cr0sh created PR review comment:
Might be a mistake while rebasing. Thanks for pointing out.
cr0sh updated PR #5342 from run-piped-modules
to main
.
jameysharp submitted PR review.
jameysharp merged PR #5342.
Last updated: Dec 23 2024 at 12:05 UTC