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 4Module::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 thanstdin
.<!--
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.
-->
alexcrichton requested jameysharp for a review on PR #5368.
jameysharp submitted PR review.
jameysharp submitted PR review.
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 wascurl | 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)?
alexcrichton submitted PR review.
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 tocurl | 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.
jameysharp submitted PR review.
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"?
alexcrichton submitted PR review.
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.
alexcrichton closed without merge PR #5368.
Last updated: Dec 23 2024 at 13:07 UTC