Fix JS<->WASM string marshaling crash#42486
Conversation
Fix passing strings across the boundary Fix JS strings being truncated at the first null when passed to mono
|
This likely fixes #41604 along with another non-github-tracked issue involving large strings. |
kg
added
arch-wasm
CoffeeFlux
left a comment
CoffeeFlux
left a comment
There was a problem hiding this comment.
Mostly LGTM. Thanks a lot!
| var buffer = Module._malloc ((string.length + 1) * 2); | ||
| var buffer16 = (buffer / 2) | 0; | ||
| for (var i = 0; i < string.length; i++) | ||
| Module.HEAP16[buffer16 + i] = string.charCodeAt (i); |
There was a problem hiding this comment.
Nit: Is this file intended to follow Mono conventions? If so, space before the [ here and the next line.
There was a problem hiding this comment.
It is, though I don't know if we follow that convention for the JS. I can make it match.
There was a problem hiding this comment.
I'll correct this in the larger bindings optimization PR, didn't want to let a formatting change delay the merge on this one
There was a problem hiding this comment.
Isn't string[i] access generally faster than charCodeAt ?
There was a problem hiding this comment.
string[i] in JS returns a single-character string
|
/backport to release/5.0-rc2 |
|
Started backporting to release/5.0-rc2: https://github.com/dotnet/runtime/actions/runs/262527073 |
|
@kg backporting to release/5.0-rc2 failed, the patch most likely resulted in conflicts: $ git am --3way --ignore-whitespace --keep-non-patch changes.patch
Applying: Fix passing mono object ptrs to bound functions Fix passing strings across the boundary Fix JS strings being truncated at the first null when passed to mono
Applying: Add new string conv wrapper
error: sha1 information is lacking or useless (src/mono/wasm/runtime/binding_support.js).
error: could not build fake ancestor
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0002 Add new string conv wrapper
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
Error: The process '/usr/bin/git' failed with exit code 128Please backport manually! |
|
This adds another allocation+copy for that marshal case, what was the failure case before? |
|
Random out-of-bounds memory accesses / memory corruptions, and truncation at the first null |
Fix JS strings being truncated at the first null when passed to mono Fix crashes when moving large strings across the JS<->WASM boundary
Fix JS strings being truncated at the first null when passed to mono Fix crashes when moving large strings across the JS<->WASM boundary
ghost
locked as resolved and limited conversation to collaborators
5 participants
Under some circumstances passing strings across the JS<->WASM boundary will crash or corrupt memory. We also currently truncate strings at the first embedded null, which is wrong. This PR fixes both.