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

[Strings] Avoid mishandling unicode in StringConcat. #6411

Merged
merged 2 commits into from
Mar 19, 2024

Conversation

rluble
Copy link
Contributor

@rluble rluble commented Mar 19, 2024

No description provided.

src/wasm-interpreter.h Outdated Show resolved Hide resolved
@kripken kripken enabled auto-merge (squash) March 19, 2024 21:45
@kripken kripken merged commit f9dc569 into WebAssembly:main Mar 19, 2024
14 checks passed
@tlively
Copy link
Member

tlively commented Mar 20, 2024

Is this PR necessary? I was under the impression that string.concat blindly concatenates the strings, no matter what their contents are.

@rluble
Copy link
Contributor Author

rluble commented Mar 20, 2024

Is this PR necessary?

Necessary, yes. If you have malformed UTF-8 the first byte(s) of the second string might become a valid escape; so in general you would need to backoff to avoid that. However my thinking was that if the UTF-8 string came from a Java wtf-16 then it should be ok to concat, since in Java it just concats the chars, but alas this was tripping our test with invalid WTF-16.

@tlively
Copy link
Member

tlively commented Mar 20, 2024

Can you share the test case that was failing? I just want to make sure this isn't masking another bug somewhere else. As far as I can tell, as long as the strings are valid WTF-8, the only problem concatenation can cause is an explicit surrogate sequence that should be a single WTF-8 code point. We'll warn about that in string lowering, but the resulting JSON should still be ok.

@gkdn gkdn mentioned this pull request Aug 31, 2024
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.

3 participants