Stream: git-wasmtime

Topic: wasmtime / PR #5368 Fix some minor issues with stdin-load...

Wasmtime GitHub notifications bot (Dec 01 2022 at 19:54):

alexcrichton opened PR #5368 from minor-fixes to main:

This commit addresses a few items I saw after reading #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.

<!--

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

Wasmtime GitHub notifications bot (Dec 01 2022 at 19:54):

alexcrichton requested jameysharp for a review on PR #5368.

Wasmtime GitHub notifications bot (Dec 01 2022 at 20:16):

jameysharp submitted PR review.

Wasmtime GitHub notifications bot (Dec 01 2022 at 20:16):

jameysharp submitted PR review.

Wasmtime GitHub notifications bot (Dec 01 2022 at 20:16):

jameysharp created PR review comment:

I don't want to re-introduce the ELF-magic check in the CLI for two reasons:

• We considered it a feature that --allow-precompiled doesn't work on pipes. The motivating use case for this feature was curl | wasmtime, where the provided code really shouldn't be trusted.
• I think the details of which formats are supported should be fully encapsulated in the wasmtime crate, not duplicated in the CLI. Somebody (I forget who) suggested that the CLI should serve as an example of how to use the library, and I like that as a guiding principle here.

I think it'd be reasonable to declare that --allow-precompiled is incompatible with reading from stdin. With #5342, if stdin happens to be mmap-able because e.g. it's redirected from a file on Unix, then --allow-precompiled works, but I think it's okay to lose that "feature".

So I'd prefer to reject that combination while validating arguments, and replace this block with:

                Module::new(linker.engine(), &module)?

Wasmtime GitHub notifications bot (Dec 01 2022 at 21:00):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Dec 01 2022 at 21:00):

alexcrichton created PR review comment:

Can you elaborate more on why it's considered a good thing to disallow --allow-precompiled and stdin? It's already opt-in behavior which I would imagine suitably covers any misuse issues. It surely would be bad to curl | wasmtime --allow-precompiled but I don't feel like it's our place to prevent that if someone really wanted to.

As for the ELF detection, I don't mind moving it into the crate one way or another.

Wasmtime GitHub notifications bot (Jan 06 2023 at 02:23):

jameysharp submitted PR review.

Wasmtime GitHub notifications bot (Jan 06 2023 at 02:23):

jameysharp created PR review comment:

Sorry, I lost track of this. I guess I don't have a strong argument for preferring to disallow --allow-precompiled when reading from stdin on general principle.

I do think the API design is a little fiddly if you want to allow that case though. My suggestion, which we adopted in #5342, had been to combine "is this safe to mmap" with "is this safe to blindly execute as native code", so the same Module::from_trusted_file API is used to mean both things. We could introduce, I dunno, Module::from_trusted_bytes or something to mean only "safe to execute"?

Wasmtime GitHub notifications bot (Jan 17 2023 at 19:14):

alexcrichton submitted PR review.

Wasmtime GitHub notifications bot (Jan 17 2023 at 19:14):

alexcrichton created PR review comment:

Hm ok I've sort of lost context on this and don't feel strongly motivated for pushing it over the finish line, so I'll leave this to a future PR if it's necessary.

Wasmtime GitHub notifications bot (Jan 17 2023 at 19:15):

alexcrichton closed without merge PR #5368.

Last updated: Nov 22 2024 at 16:03 UTC