-
Notifications
You must be signed in to change notification settings - Fork 24.5k
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: Early termination of strings with NULL values in-between for JavascriptCore #34300
base: main
Are you sure you want to change the base?
Conversation
`createStringFromUtf8` currently calls JavascriptCore's `JSStringCreateWithUTF8CString(const char *)` method when retrieving strings from the native side, causing strings with NULL values in-between to be early-truncated. Fixes facebook#24129
Base commit: 1af2bea |
Base commit: 1af2bea |
This PR is stale because it has been open 180 days with no activity. Remove stale label or comment or this will be closed in 7 days. |
not stale? |
Thanks for the fix. The approach makes sense, but I'd cautious this may cause performance regressions, given the existing fast path that exists in JSC for all-ASCII stings. I'd also prefer if we could defer a system implementation here, or have an implementation of these methods outside of this file that reference their original source (e.g. https://github.com/WebKit/WebKit/blob/main/Source/WTF/wtf/unicode/UTF8Conversion.cpp) |
Hi. Thanks for reviewing! (and sorry for the late reply) Just wondering; do you have any suggestions on how I might import that function (the one that converts UTF-8 to UTF-16 strings over from the WebKit/JSC side?) over? It's implemented in CPP and I'm not sure how I might define extern function declarations or whatnot to make it work. As for potential performance regressions, maybe we just have to benchmark in the end? Because of I understand correctly, eventually what the existing code path does is it needs to convert the strings over to UTF-16 internally anyway when on JSC. |
This PR is stale because it has been open 180 days with no activity. Remove stale label or comment or this will be closed in 7 days. |
... any potentials for this? |
This PR is stale because it has been open 180 days with no activity. Remove stale label or comment or this will be closed in 7 days. |
Is this basically stale because JavascriptCore is not a priority and Hermes is the future? Or is this still a valid issue? |
Summary
createStringFromUtf8
currently calls JavascriptCore'sJSStringCreateWithUTF8CString(const char *)
method when retrieving strings from the native side, causing strings with NULL values in-between to be early-truncated (since it stops reading with the first NULL value).Fixes #24129
Changelog
Changed
jsi::String JSCRuntime::createStringFromUtf8(const uint8_t *str, size_t length)
to use a parser to UTF-16 characters as appropriate for the JSChar * constructor,JSStringCreateWithCharacters(const JSChar* chars, size_t numChars)
, which allows specifying of length.This also should provide a minor speed-up for longer strings since
JSStringCreateWithUTF8CString
internally calls strlen(), then does UTF-8 to UTF-16 conversions.[General] [Fixed] - Early termination of strings with NULL values in JavascriptCore (#24129)
Test Plan
I executed calls to my
level
adapter, saving and loading strings.Without this fix it would result in only 'hi'.
With this fix, you get the whole string, 'hi\u0000world'.