Skip to content

Commit

Permalink
JSStringToSTLString: truncate string on conversion failure
Browse files Browse the repository at this point in the history
Summary:
[A recent change to JSStringToSTLString](#26955) causes a crash when the function is invoked with invalid UTF-16 data. The old behaviour, restored here, was to truncate the string before the first invalid character.

Here's how [the original code](https://github.com/facebook/react-native/blob/aee88b6843cea63d6aa0b5879ad6ef9da4701846/ReactCommon/jsi/JSCRuntime.cpp#L287) handled this case:
```
std::string JSStringToSTLString(JSStringRef str) {
  size_t maxBytes = JSStringGetMaximumUTF8CStringSize(str);
  // ^ maxBytes >= 1 regardless of str's contents
  std::vector<char> buffer(maxBytes);
  // ^ vector is zero initialised
  JSStringGetUTF8CString(str, buffer.data(), maxBytes);
  // ^ writes '\0' at the first invalid character and returns early (see JSC source code)
  return std::string(buffer.data());
  // ^ copies the string up to the first '\0'
}
```

See the JSC implementations of [`JSStringGetUTF8CString`](https://opensource.apple.com/source/JavaScriptCore/JavaScriptCore-7600.8.7/API/JSStringRef.cpp.auto.html) and [`convertUTF16ToUTF8`](https://opensource.apple.com/source/WTF/WTF-7600.7.2/wtf/unicode/UTF8.cpp.auto.html).

Based on the fact that `JSStringGetUTF8CString` *always* null-terminates the buffer - even when it bails out of converting an invalid string - here we're able to both

1. keep the fast path (not zero-initialising, not scanning for the null terminator) for the common case when the data is valid and JSStringGetUTF8CString returns a nonzero length; and
2. return the truncated string when JSStringGetUTF8CString returns an error code of 0, by scanning for the null terminator.

Changelog: [General] [Fixed] - Fix crash when passing invalid UTF-16 data from JSC into native code

Differential Revision: D19902751

fbshipit-source-id: 06bace2719800e921ec115ad6a29251eafd473f6
  • Loading branch information
motiz88 authored and facebook-github-bot committed Feb 14, 2020
1 parent 3611170 commit 011cf3f
Showing 1 changed file with 13 additions and 1 deletion.
14 changes: 13 additions & 1 deletion ReactCommon/jsi/JSCRuntime.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -294,6 +294,7 @@ std::string JSStringToSTLString(JSStringRef str) {
std::array<char, 20> stackBuffer;
std::unique_ptr<char[]> heapBuffer;
char *buffer;
// NOTE: By definition, maxBytes >= 1 since the null terminator is included.
size_t maxBytes = JSStringGetMaximumUTF8CStringSize(str);
if (maxBytes <= stackBuffer.size()) {
buffer = stackBuffer.data();
Expand All @@ -302,7 +303,18 @@ std::string JSStringToSTLString(JSStringRef str) {
buffer = heapBuffer.get();
}
size_t actualBytes = JSStringGetUTF8CString(str, buffer, maxBytes);
// NOTE: By definition, maxBytes >= actualBytes >= 1.
if (!actualBytes) {
// Happens if maxBytes == 0 (never the case here) or if str contains
// invalid UTF-16 data, since JSStringGetUTF8CString attempts a strict
// conversion.
// When converting an invalid string, JSStringGetUTF8CString writes a null
// terminator before returning. So we can reliably treat our buffer as a C
// string and return the truncated data to our caller. This is slightly
// slower than if we knew the length (like below) but better than crashing.
// TODO(T62295565): Perform a non-strict, best effort conversion of the
// full string instead, like we did before the JSI migration.
return std::string(buffer);
}
return std::string(buffer, actualBytes - 1);
}

Expand Down

0 comments on commit 011cf3f

Please sign in to comment.