Wasmtime GitHub notifications bot (May 10 2024 at 18:59): Wasmtime GitHub notifications bot (May 10 2024 at 18:59):alexcrichton opened PR #8594 from alexcrichton:repro-64k-env-bug to bytecodealliance:main:
This commit fixes an issue with the wasip1 adapter published with Wasmtime which current limits the size of environment variables, arguments, and preopens to not exceed 64k. This bug comes from the fact that we more-or-less forgot to account for this when designing the adapter initially. The adapter allocates a single WebAssembly page for itself but does not have a means of allocating much more than that. It's technically possible to continue to call
memory.growor possiblycabi_reallocfrom the original main module but it's pretty awkward.The solution in this commit is to take an alternative approach to how these properties are all processed. Previously arguments/env vars/preopens were all allocated once within the adapter and stored statically. This means that after startup they're copied from where they reside in-memory, but the downside is that we have to have enough memory to hold everything. This commit instead tries to "stream" the items so they're never held entirely within the adapter itself.
The general idea in this commit is to use the "align" parameter to
cabi_import_reallocto figure out what's being allocated and route the allocation to the destination. For example an allocation with alignment 1 is a string and can go directly into a user-supplied pointer where an allocation with alignment 4 is a pointer-based allocation which must be stored within the adapter, but only temporarily.With this redesign it's now possible to have the sum total of args/envs/preopens to exceed 64k. The new limitation is that the max-length string plus size of the max length of these arrays must be less than 64k. This should be a more reasonable limit than before where any one individual argument/env var is unlikely to exceed 64k (or get close).
Closes #8556
<!--
Please make sure you include the following information:
If this work has been discussed elsewhere, please include a link to that
conversation. If it was discussed in an issue, just mention "issue #...".Explain why this change is needed. If the details are in an issue already,
this can be brief.Our development process is documented in the Wasmtime book:
https://docs.wasmtime.dev/contributing-development-process.htmlPlease ensure all communication follows the code of conduct:
https://github.com/bytecodealliance/wasmtime/blob/main/CODE_OF_CONDUCT.md
-->
Wasmtime GitHub notifications bot (May 10 2024 at 18:59):alexcrichton requested wasmtime-core-reviewers for a review on PR #8594.
Wasmtime GitHub notifications bot (May 10 2024 at 18:59):alexcrichton requested elliottt for a review on PR #8594.
Wasmtime GitHub notifications bot (May 10 2024 at 19:01):alexcrichton commented on PR #8594:
I should mention as well that the downside to this approach is that previously we'd call
get-argumentsonce, for example, but now it's called twice. Once for when the argument sizes are requested and once when the argument data is requested. This runs the risk of slowing down startup a bit because there's more copying going on. My hope though is that this won't affect the majority of applications and if it's a problem for any one application then a different version of the adapter can be used. Perhaps even one day the adapter can provide customization options at build time to allocate, for example, two pages instead of 1 to be able to statically store more memory.
Wasmtime GitHub notifications bot (May 10 2024 at 19:02):alexcrichton updated PR #8594.
Wasmtime GitHub notifications bot (May 14 2024 at 00:47):elliottt submitted PR review:
This look good to me! Thank you for that big comment block on the ImportAlloc enum!
Wasmtime GitHub notifications bot (May 14 2024 at 00:47):elliottt submitted PR review:
This look good to me! Thank you for that big comment block on the ImportAlloc enum!
Wasmtime GitHub notifications bot (May 14 2024 at 00:47):elliottt created PR review comment:
// only interested in one individual string allocation, however, so
Wasmtime GitHub notifications bot (May 14 2024 at 14:47):alexcrichton updated PR #8594.
Wasmtime GitHub notifications bot (May 14 2024 at 14:47):alexcrichton has enabled auto merge for PR #8594.
Wasmtime GitHub notifications bot (May 14 2024 at 15:31):alexcrichton updated PR #8594.
Wasmtime GitHub notifications bot (May 14 2024 at 15:31):alexcrichton has enabled auto merge for PR #8594.
Wasmtime GitHub notifications bot (May 14 2024 at 16:10):alexcrichton merged PR #8594.
Last updated: Nov 22 2024 at 16:03 UTC