Skip to content
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

Merged
merged 4 commits into from May 14, 2024

Conversation

alexcrichton
Copy link
Member

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 #8556

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
@alexcrichton
Copy link
Member Author

I should mention as well that the downside to this approach is that previously we'd call get-arguments once, 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.

Copy link
Member

@elliottt elliottt left a 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!

crates/wasi-preview1-component-adapter/src/descriptors.rs Outdated Show resolved Hide resolved
Co-authored-by: Trevor Elliott <awesomelyawesome@gmail.com>
@alexcrichton alexcrichton added this pull request to the merge queue May 14, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks May 14, 2024
Looks like a 1M env block is a bit too large.
@alexcrichton alexcrichton added this pull request to the merge queue May 14, 2024
Merged via the queue into bytecodealliance:main with commit 2f0354f May 14, 2024
22 checks passed
@alexcrichton alexcrichton deleted the repro-64k-env-bug branch May 14, 2024 16:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

wasi-preview1-component-adapter: A 64KiB arena is too small for environment variables
2 participants