Fix some minor issues with stdin-loaded modules #5368
Conversation
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`.
| if self.allow_precompiled && module.starts_with(b"\x7fELF") { | ||
| unsafe { Module::deserialize(linker.engine(), &module)? } | ||
| } else { | ||
| Module::new(linker.engine(), &module)? | ||
| } |
There was a problem hiding this 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-precompileddoesn'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:
| if self.allow_precompiled && module.starts_with(b"\x7fELF") { | |
| unsafe { Module::deserialize(linker.engine(), &module)? } | |
| } else { | |
| Module::new(linker.engine(), &module)? | |
| } | |
| Module::new(linker.engine(), &module)? |
There was a problem hiding this 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.
There was a problem hiding this 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"?
There was a problem hiding this 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.
github-actions
bot
added
the
wasmtime:api
Subscribe to Label Actioncc @peterhuene
This issue or pull request has been labeled: "wasmtime:api"
Thus the following users have been cc'd because of the following labels:
To subscribe or unsubscribe from this label, edit the |
This commit addresses a few items I saw after reading #5342 such as:
Module::from_trusted_fileno longer panics if the file length is less than 4Module::from_trusted_filewith an empty file now returns a syntax error for*.watinstead of a "failed to map" error.-for stdin no works on Windows instead of just Unix.-reported to the executable is<stdin>rather thanstdin.