Move precompiled module detection into wasmtime #5342

cr0sh · 2022-11-29T16:26:06Z

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.

Note: this supercedes #5035.

This applies to unix targets only, as Windows does not have an appropriate alternative.

github-actions · 2022-11-29T16:27:08Z

Subscribe to Label Action

cc @peterhuene

This issue or pull request has been labeled: "wasmtime:api"

Thus the following users have been cc'd because of the following labels:

peterhuene: wasmtime:api

To subscribe or unsubscribe from this label, edit the .github/subscribe-to-label.json configuration file.

Learn more.

jameysharp

Overall, this looks like exactly what I hoped for! There are a couple issues—most notably, this doesn't compile due to some other changes that happened while you were working on it. I hope the fixes will be straightforward, but do let me know if you run into any trouble!

jameysharp · 2022-11-29T17:31:45Z

crates/wasmtime/src/module.rs

@@ -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.");


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.");

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

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.

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

crates/wasmtime/src/module.rs

jameysharp · 2022-11-29T18:08:30Z

crates/wasmtime/src/module.rs

+            return SerializedModule::from_mmap(mmap, &engine.config().module_version)?
+                .into_module(engine);


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.

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

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

jameysharp · 2022-11-29T18:10:15Z

crates/wasmtime/src/module.rs

+                .into_module(engine);
+        }
+
+        Module::new(engine, &*mmap)


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.

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

src/commands/run.rs

This applies to unix targets only, as Windows does not have an appropriate alternative.

@jameysharp

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.

Co-authored-by: Jamey Sharp <jamey@minilop.net>

jameysharp

Awesome. Thank you for putting this together! You put a lot of work into this and I think it's turned out great.

This commit addresses a few items I saw after reading bytecodealliance#5342 such as: * `Module::from_trusted_file` no longer panics if the file length is less than 4 * `Module::from_trusted_file` with an empty file now returns a syntax error for `*.wat` instead of a "failed to map" error. * Usage of `-` for stdin no works on Windows instead of just Unix. * The filename for `-` reported to the executable is `<stdin>` rather than `stdin`.

Treat - as an alias to /dev/stdin

675dded

This applies to unix targets only, as Windows does not have an appropriate alternative.

github-actions bot added the wasmtime:api Related to the API of the `wasmtime` crate itself label Nov 29, 2022

jameysharp reviewed Nov 29, 2022

View reviewed changes

jameysharp reviewed Nov 30, 2022

View reviewed changes

src/commands/run.rs Show resolved Hide resolved

cr0sh and others added 7 commits November 30, 2022 21:19

Add tests for piped modules from stdin

910cbad

This applies to unix targets only, as Windows does not have an appropriate alternative.

Improve doc-comment of Module::from_trusted_file

63fcb6f

Co-authored-by: Jamey Sharp <jamey@minilop.net>

Make Engine::load_code pub(crate)

3112eb8

wasmtime: Fix typo on Engine::load_code_file doc-comment

8bf8518

Remove redundant eprintln! hint on Module::from_file

1ac8554

Fix Module::from_trusted_file using removed SerializedModule API

b30937e

cr0sh force-pushed the run-piped-modules branch from 9fab923 to b30937e Compare November 30, 2022 12:20

cr0sh and others added 2 commits December 1, 2022 13:57

Notify user about missing --allow-precompiled via anyhow::Context

61136ba

Co-authored-by: Jamey Sharp <jamey@minilop.net>

cargo fmt

c1ef3c7

jameysharp approved these changes Dec 1, 2022

View reviewed changes

jameysharp merged commit ebb693a into bytecodealliance:main Dec 1, 2022

alexcrichton mentioned this pull request Dec 1, 2022

Fix some minor issues with stdin-loaded modules #5368

Closed

cr0sh deleted the run-piped-modules branch December 1, 2022 23:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move precompiled module detection into wasmtime #5342

Move precompiled module detection into wasmtime #5342

cr0sh commented Nov 29, 2022

github-actions bot commented Nov 29, 2022

jameysharp left a comment

jameysharp Nov 29, 2022

bjorn3 Nov 29, 2022

cr0sh Nov 30, 2022

jameysharp Nov 30, 2022

jameysharp Nov 29, 2022

cr0sh Nov 30, 2022 •

edited

Loading

cr0sh Nov 30, 2022

jameysharp Nov 29, 2022

bjorn3 Nov 29, 2022

jameysharp left a comment

	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.");

		return SerializedModule::from_mmap(mmap, &engine.config().module_version)?
		.into_module(engine);

Move precompiled module detection into wasmtime #5342

Move precompiled module detection into wasmtime #5342

Conversation

cr0sh commented Nov 29, 2022

github-actions bot commented Nov 29, 2022

Subscribe to Label Action

jameysharp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cr0sh Nov 30, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jameysharp left a comment

Choose a reason for hiding this comment

cr0sh Nov 30, 2022 •

edited

Loading