Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move precompiled module detection into wasmtime #5342

Merged
merged 10 commits into from
Dec 1, 2022
Prev Previous commit
Next Next commit
Move precompiled module detection into wasmtime
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 the `Module::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.
The `Module::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 existing `Module::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
on `Module::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 the `wat` crate, but I feel comfortable (yet) this strategy
because the `wat` 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.
cr0sh committed Nov 30, 2022
commit 666988a6bd92f488a280f90f55a0561f6730b07d
28 changes: 26 additions & 2 deletions crates/wasmtime/src/module.rs
Original file line number Diff line number Diff line change
@@ -241,6 +241,10 @@ impl Module {
if #[cfg(feature = "wat")] {
let mut e = e.downcast::<wat::Error>()?;
e.set_path(file);
if e.to_string().ends_with("input bytes aren't valid utf-8") {
eprintln!("note: wasmtime might be trying to load a precompiled binary without --allow-compiled.");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you meant --allow-precompiled here:

Suggested change
eprintln!("note: wasmtime might be trying to load a precompiled binary without --allow-compiled.");
eprintln!("note: wasmtime might be trying to load a precompiled binary without --allow-precompiled.");

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think the wasmtime library should print to stderr. There is no way for a binary to suppress this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've just suggested a different way to report this error, so let's delete this eprintln in favor of that alternative.

}

bail!(e)
} else {
Err(e)
@@ -332,8 +336,28 @@ impl Module {
}
}

/// Compiles a binary-encoded WebAssembly module to an artifact usable by
/// Wasmtime.
/// Creates a new WebAssembly `Module` from the contents of the given
/// `file` on disk, but with assumptions that the file is 'sane'.
/// In other words, the file should be a binary- or text-format WebAssembly
/// module or a precompiled version of it generated by wasmtime.
///
/// # Unsafety
///
/// This function is marked as `unsafe` as it relies on [`Module::deserialize`].
cr0sh marked this conversation as resolved.
Show resolved Hide resolved
#[cfg(compiler)]
#[cfg_attr(nightlydoc, doc(cfg(feature = "cranelift")))] // see build.rs
pub unsafe fn from_trusted_file(engine: &Engine, file: impl AsRef<Path>) -> Result<Module> {
let mmap = MmapVec::from_file(file.as_ref())?;
if &mmap[0..4] == b"\x7fELF" {
return SerializedModule::from_mmap(mmap, &engine.config().module_version)?
.into_module(engine);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately, things changed underneath you (in #5153), so SerializedModule doesn't exist any more. Fortunately, I think this is an easy fix. In crates/wasmtime/src/engine.rs, make fn load_code be pub(crate). Then here you can call engine.load_code(mmap, ObjectKind::Module). See deserialize and deserialize_file for examples of the current pattern.

Copy link
Contributor Author

@cr0sh cr0sh Nov 30, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, I forgot to cargo check again after rebasing. I'll fix at the eod today

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is done and I found a small typo near load_code so fixed it too ;)

}

Module::new(engine, &*mmap)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It needs a type implementing AsRef<[u8]> as argument. &MmapVec doesn't implement this. MmapVec only implements Deref and DerefMut.

}

/// Converts an input binary-encoded WebAssembly module to compilation
/// artifacts and type information.
///
/// This is where compilation actually happens of WebAssembly modules and
/// translation/parsing/validation of the binary input occurs. The binary
26 changes: 4 additions & 22 deletions src/commands/run.rs
Original file line number Diff line number Diff line change
@@ -3,8 +3,6 @@
use anyhow::{anyhow, bail, Context as _, Result};
use clap::Parser;
use once_cell::sync::Lazy;
use std::fs::File;
use std::io::Read;
use std::thread;
use std::time::Duration;
use std::{
@@ -425,27 +423,11 @@ impl RunCommand {
}

fn load_module(&self, engine: &Engine, path: &Path) -> Result<Module> {
// Peek at the first few bytes of the file to figure out if this is
// something we can pass off to `deserialize_file` which is fastest if
// we don't actually read the whole file into memory. Note that this
// behavior is disabled by default, though, because it's not safe to
// pass arbitrary user input to this command with `--allow-precompiled`
let mut file =
File::open(path).with_context(|| format!("failed to open: {}", path.display()))?;
let mut magic = [0; 4];
if let Ok(()) = file.read_exact(&mut magic) {
if &magic == b"\x7fELF" {
if self.allow_precompiled {
return unsafe { Module::deserialize_file(engine, path) };
}
bail!(
"cannot load precompiled module `{}` unless --allow-precompiled is passed",
path.display()
)
}
if self.allow_precompiled {
unsafe { Module::from_trusted_file(engine, path) }
} else {
Module::from_file(engine, path)
cr0sh marked this conversation as resolved.
Show resolved Hide resolved
}

Module::from_file(engine, path)
}
}