-
Notifications
You must be signed in to change notification settings - Fork 1.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Allow env/args/preopens to exceed 64k in size #8594
Allow env/args/preopens to exceed 64k in size #8594
Conversation
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.grow` or possibly `cabi_realloc` from 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_realloc` to 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 bytecodealliance#8556
I should mention as well that the downside to this approach is that previously we'd call |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This look good to me! Thank you for that big comment block on the ImportAlloc enum!
Co-authored-by: Trevor Elliott <[email protected]>
Looks like a 1M env block is a bit too large.
Integrate the adapter allocator changes from bytecodealliance/wasmtime#8594, which simplifies supporting cli args. Additionally, enable the cli args test for components, as it's now passing.
Integrate the adapter allocator changes from bytecodealliance/wasmtime#8594, which simplifies supporting cli args. Additionally, enable the cli args test for components, as it's now passing.
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.grow
or possiblycabi_realloc
from 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_realloc
to 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