Stream: git-wasmtime

Topic: wasmtime / PR #5342 Move precompiled module detection int...

view this post on Zulip Wasmtime GitHub notifications bot (Nov 29 2022 at 16:26):

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 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.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 29 2022 at 18:22):

jameysharp submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 29 2022 at 18:22):

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.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 29 2022 at 18:22):

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

view this post on Zulip Wasmtime GitHub notifications bot (Nov 29 2022 at 18:22):

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. 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.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 29 2022 at 18:22):

jameysharp submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 29 2022 at 18:22):

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.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 29 2022 at 20:07):

bjorn3 submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 29 2022 at 20:07):

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.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 29 2022 at 20:09):

bjorn3 created PR review comment:

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

view this post on Zulip Wasmtime GitHub notifications bot (Nov 29 2022 at 20:09):

bjorn3 submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 30 2022 at 01:22):

cr0sh submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 30 2022 at 01:22):

cr0sh created PR review comment:

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

view this post on Zulip Wasmtime GitHub notifications bot (Nov 30 2022 at 01:23):

cr0sh edited PR review comment.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 30 2022 at 01:41):

cr0sh submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 30 2022 at 01:41):

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.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 30 2022 at 01:43):

cr0sh submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 30 2022 at 01:43):

cr0sh created PR review comment:

Great! Didn't think the scenario that the file would be modified after mmaping. Nice catch.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 30 2022 at 01:44):

cr0sh updated PR #5342 from run-piped-modules to main.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 30 2022 at 02:00):

jameysharp submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 30 2022 at 02:00):

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

view this post on Zulip Wasmtime GitHub notifications bot (Nov 30 2022 at 02:02):

jameysharp submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 30 2022 at 02:02):

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.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 30 2022 at 12:07):

cr0sh submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 30 2022 at 12:07):

cr0sh created PR review comment:

Awesome.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 30 2022 at 12:08):

cr0sh updated PR #5342 from run-piped-modules to main.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 30 2022 at 12:20):

cr0sh updated PR #5342 from run-piped-modules to main.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 30 2022 at 12:20):

cr0sh submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Nov 30 2022 at 12:20):

cr0sh created PR review comment:

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

view this post on Zulip Wasmtime GitHub notifications bot (Dec 01 2022 at 01:30):

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?

view this post on Zulip Wasmtime GitHub notifications bot (Dec 01 2022 at 01:30):

jameysharp submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Dec 01 2022 at 04:57):

cr0sh updated PR #5342 from run-piped-modules to main.

view this post on Zulip Wasmtime GitHub notifications bot (Dec 01 2022 at 04:58):

cr0sh submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Dec 01 2022 at 04:58):

cr0sh created PR review comment:

Might be a mistake while rebasing. Thanks for pointing out.

view this post on Zulip Wasmtime GitHub notifications bot (Dec 01 2022 at 05:32):

cr0sh updated PR #5342 from run-piped-modules to main.

view this post on Zulip Wasmtime GitHub notifications bot (Dec 01 2022 at 17:12):

jameysharp submitted PR review.

view this post on Zulip Wasmtime GitHub notifications bot (Dec 01 2022 at 17:13):

jameysharp merged PR #5342.

Last updated: Nov 22 2024 at 17:03 UTC