Bounds-check string transcoder buffers before calling the host

[ilyas-mallah]

The host string transcoders in vm/component/libcalls.rs build raw slices from the pointers they get, and only the adapter's earlier checks keep those pointers in bounds. This path had three advisories this year (GHSA-394w-hwhg-8vgm, GHSA-hx6p-xpx3-jvvv, GHSA-jxhv-7h78-9775), each a gap in those checks.

This adds a second check in the adapter right before every transcoder call: a plain wasm load of the last byte or 16-bit unit of each buffer the host will access, in that buffer's memory. Empty buffers skip the load. Before it, the adapter traps on a length over the maximum string size or an address that wraps, and 16-bit buffers keep the alignment check, now a helper shared with validate_guest_pointer.

If one of these loads fails, the trap is a regular out-of-bounds trap instead of StringOutOfBounds. That only happens when an earlier check has a bug.

Tested with a new disas test and the component-model wast tests on Cranelift, Winch and Pulley. With validate_guest_pointer disabled locally, the out-of-bounds cases in component_model::strings still trap, and without the new loads as well they crash the test process.

[alexcrichton]

Thanks for this! Out of curiosity, would you be interested in helping to try something with a slightly different tact instead? Bounds-checks are notoriously tricky and hard to get right, so instead of that one possibility would be to use a normal wasm load/store to determine if the string is valid. For example if before calling transcoding the adapter could perform a wasm load of the last byte/16-bit codepoint in the string, and if that succeeds then everything is guaranteed to be in-bounds. That'd keep the translation and handling on the wasm-side as well which I think would be nice to keep the Cranelift side smaller

[ilyas-mallah]

Sure, happy to try that. I'll rework it so the adapter does a wasm load of the last byte (or the last 16-bit unit for UTF-16) of each buffer before calling the transcoder, and drop the Cranelift side. For zero-length strings I'd skip the load, since the host only builds an empty slice there. Does that match what you had in mind?

[alexcrichton]

Yeah that's what I was thinking too, zero-length is just skipped

[ilyas-mallah]

Yeah that's what I was thinking too, zero-length is just skipped

Just reworked and pushed it as a new commit that replaces the Cranelift version entirely. The full PR diff is easier to read than the commit on its own. Two things that might be worth a look: the 16-bit alignment check moved out of validate_guest_pointer into a helper so both can use it, and a failed load is now a plain out-of-bounds trap instead of StringOutOfBounds. I also swapped the disas test for one showing the loads, can drop it if it's not useful.