wasi-preview1-component-adapter: A 64KiB arena is too small for environment variables #8556
Comments
dfoxfranke
added
the
bug
|
Thanks for the bug report. I'm not sure how easy this will be to fix, the component-adapter is a very change resistant bit of software, but I believe our original reasons for designing the long-lived BumpArena to have a capped size are no longer valid, so I'm going to see if I can change the design so that that it can grow unbounded. |
|
Could https://crates.io/crates/wee_alloc be a good fit for this? |
|
@pchickey I was poking a bit at this today and wrote up a reproduction test case (ignore the commented out bits in the rest of Wasmtime). Otherwise I think it should be possible to call |
|
thanks! I made a reproducer as well today but i like yours better. i was going through trying to decode the history of why we didnt always call cabi_realloc directly and trying to figure out, when it became possible to do so, did we choose not to do it or did we just not think about it? anyway I think I'm going kayaking tomorrow but I will give it a try wednesday. |
|
Originally we assumed we couldn't call cabi_realloc at all and even now the import of cabi_realloc is a bit of a lie, sometimes it's hooked up to Otherwise though I think we just forgot to handle the case of >=64k env/arg blocks... |
|
I think I've got an idea, albeit a bit nasty in the implementation, of how to solve this so I'm going to give it a stab |
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
|
While it's very much a hack the idea ended up at least mostly working out |
* Allow env/args/preopens to exceed 64k in size 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 * Comment descriptors are closed * Update crates/wasi-preview1-component-adapter/src/descriptors.rs Co-authored-by: Trevor Elliott <awesomelyawesome@gmail.com> * Turn down process limits for macOS Looks like a 1M env block is a bit too large. --------- Co-authored-by: Trevor Elliott <awesomelyawesome@gmail.com>
The single 64KiB page available to
BumpArenainwasi-preview1-component-adapteris not reliably sufficient for allocating environment variables during initialization. This problem manifested for me as a crash injcowhen running it inside a NixOS shell with a lot of environment variables set.The text was updated successfully, but these errors were encountered: