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

fix: don't overallocate string buffer while encoding into utf8 #444

Merged

Conversation

kateinoigakukun
Copy link
Contributor

@kateinoigakukun kateinoigakukun commented Jun 4, 2024

Recent dlmalloc-rs started validating that deallocation size is same as the size that was allocated.
https://github.com/alexcrichton/dlmalloc-rs/blob/0.2.6/src/dlmalloc.rs#L1187

Since canonical abi string does not have "capacity" concept separate from "length", we don't have a way to tell how much memory was allocated to the buffer owner. So, canonical abi always assumes that "length" is the same as "capacity" and deallocates the buffer with the "length" size.

This means that if we overallocate the buffer and return the buffer with a length less than the allocated size, dlmalloc-rs validation will fail.

$ rustc --version
rustc 1.80.0-nightly (791adf759 2024-05-21)
$ cat main.rs
fn main() {
    let foo = std::env::var("FOO");
    println!("FOO: {:?}", foo);
}

$ rustc --target=wasm32-wasi main.rs
$ wasm-tools component new main.wasm -o main.component.wasm --adapt wasi_snapshot_preview1.command.wasm
$ wasi-virt --debug --allow-stdio --allow-all --out main.component.virt.wasm main.component.wasm
$ jco run main.component.virt.wasm
RuntimeError: unreachable
    at wasi_virt._ZN8dlmalloc8dlmalloc17Dlmalloc$LT$A$GT$13validate_size17h90a93ee998398de2E (wasm://wasm/wasi_virt-0006f322:wasm-function[243]:0x100cf)
    at wasi_virt.__rust_dealloc (wasm://wasm/wasi_virt-0006f322:wasm-function[348]:0x11055)
    at wasi_virt._ZN63_$LT$alloc..alloc..Global$u20$as$u20$core..alloc..Allocator$GT$10deallocate17ha2ddb58ddaa352d2E (wasm://wasm/wasi_virt-0006f322:wasm-function[340]:0x10fd9)
    at wasi_virt.cabi_post_wasi:cli/environment@0.2.0#get-environment (wasm://wasm/wasi_virt-0006f322:wasm-function[220]:0xf5f6)
    at wasm://wasm/9778e682:wasm-function[35]:0x1a1d
    at wit-component:shim.indirect-wasi:cli/environment@0.2.0-get-environment (wasm://wasm/wit-component:shim-49b6d142:wasm-function[10]:0x125)
    at wit-component:adapter:wasi_snapshot_preview1._ZN22wasi_snapshot_preview15State15get_environment17h50e6f92213a1e704E (wasm://wasm/wit-component:adapter:wasi_snapshot_preview1-00010d9e:wasm-function[28]:0x10c2)
    at wit-component:adapter:wasi_snapshot_preview1.environ_sizes_get (wasm://wasm/wit-component:adapter:wasi_snapshot_preview1-00010d9e:wasm-function[29]:0x12ca)
    at wit-component:shim.adapt-wasi_snapshot_preview1-environ_sizes_get (wasm://wasm/wit-component:shim-49b6d142:wasm-function[13]:0x14d)
    at main.wasm.__wasi_environ_sizes_get (wasm://wasm/main.wasm-00623926:wasm-function[139]:0x821f)

This commit fixes the issue by lopping off the extra allocated space by realloc with smaller size.

Recent dlmalloc-rs started validating that deallocation size is same as
the size that was allocated.
https://github.com/alexcrichton/dlmalloc-rs/blob/0.2.6/src/dlmalloc.rs#L1187

Since canonical abi string does not have "capacity" concept separate
from "length", we don't have a way to tell how much memory was
allocated to the buffer owner. So, canonical abi always assumes that
"length" is the same as "capacity" and deallocates the buffer with the
"length" size.

This means that if we overallocate the buffer and return the buffer with
a length less than the allocated size, dlmalloc-rs validation will fail.

This commit fixes the issue by lopping off the extra allocated space by
realloc with smaller size.
Previously, we would call realloc multiple times to grow the buffer and
shrink it back down to the correct size.
However, realloc provided by wasi-preview1-component-adapter is quite
restrictive:
* It can't always be called multiple times during a single imported
  function call.
* It does not accept non-null old pointers.

This commit changes the code to call realloc exactly once, which is
compatible with those restrictions. The use of temporary buffers with
`TextEncoder.encode` is less efficient than the previous `encodeInto`,
but it allows us to keep the allocator in the adapter simple.
We can optimize it after we will be able to get rid of the adapter or
decide to complicate the allocator in the adapter.
Copy link
Collaborator

@guybedford guybedford left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should still try to do the zero-copy approach if we can, unless we have benchmarks showing otherwise.

Rather than moving away from encodeInto, a shrinking relloc at the end of the encoding should be all that is needed to achieve that?

@kateinoigakukun
Copy link
Contributor Author

@guybedford Yes, I tried the zero-copy approach first, but it revealed the realloc provided by wasi-preview1-component-adapter does not support shrinking and multiple calls at this moment. So I turned it into copying approach in 0968cba

Do you think it's worth adding shrinking and multi-call support in realloc of wasi-preview1-component-adapter for jco?

@guybedford
Copy link
Collaborator

Okay thanks @kateinoigakukun for clarifying, at least until the adapter can support chained realloc calls then lets land this for now, and then come back to it if and when the adapter gets updated. Updating the adapter certainly to support it seems worthwhile though.

@guybedford guybedford merged commit c0efb2e into bytecodealliance:main Jun 5, 2024
16 checks passed
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.

2 participants