Skip to content

[DRAFT] Fix JSCRuntime::createStringFromUtf8 to support strings with embedded null chars #1930

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

Closed
wants to merge 1 commit into from

Conversation

zagriswo
Copy link

@zagriswo zagriswo commented Sep 7, 2023

DRAFT - Do not merge

Please select one of the following

  • I am removing an existing difference between facebook/react-native and microsoft/react-native-macos 👍
  • I am cherry-picking a change from Facebook's react-native into microsoft/react-native-macos 👍
  • I am making a fix / change for the macOS implementation of react-native
  • I am making a change required for Microsoft usage of react-native

Summary

The current implementation of JSCRuntime::createStringFromUtf8 ignores the length parameter that it's given, and then blindly passes the char array to JSStringCreateWithUTF8CString which assumes the char array is a null terminated string. This means we can't pass strings with embedded null chars through the JSI boundary from C++ to JS. Instead, we can internally convert the string from UTF-8 to UTF-16, and then call JSStringCreateWithCharacters which allows us to pass an explicit length for our array of chars.

JSStringCreateWithUTF8CString internally has an optimization for strings that only contain ASCII characters, which we will miss out on by switching to JSStringCreateWithCharacters. The JSC API surface isn't rich enough for us to avoid that, so if there's a perf impact here, then so be it. We will trade perf for correctness.

This is based on PR 34300 upstream in the RN repo. I'm not assuming infinite flexibility in this version. We're only building for Apple platforms, so we know the endianness and the expected size of a JSChar.

The current impl ignores the length param and blindly assumes a null-terminated string. We can convert to UTF-16 internally and then call JSStringCreateWithCharacters to create the JSStringRef, enabling support for strings that have embedded null characters.
@zagriswo
Copy link
Author

zagriswo commented Sep 7, 2023

@Saadnajmi - draft from our conversation

@zagriswo zagriswo closed this by deleting the head repository Oct 5, 2023
@zagriswo
Copy link
Author

zagriswo commented Oct 5, 2023

Abandoned due to the perf impact of this change.

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.

1 participant