bytecodealliance / jco

JavaScript toolchain for working with WebAssembly Components
https://bytecodealliance.github.io/jco/
Apache License 2.0
618 stars 62 forks source link

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

Closed kateinoigakukun closed 3 months ago

kateinoigakukun commented 3 months ago

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.

kateinoigakukun commented 3 months ago

@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 https://github.com/bytecodealliance/jco/pull/444/commits/0968cbafc4e218a4face3da95b2dbc14ac859ce0

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

guybedford commented 3 months ago

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.