Commit 5906cfb
Summary:
Fixes #24129
**Problem:** `<Text>{'Hello \u0000 World'}</Text>` renders as "Hello" (truncated at \u0000). Does NOT reproduce when Debug JS Remotely (JSON preserves \u0000), but reproduces in production Hermes+Fabric on iOS and Android.
**Root cause:** JS String preserves \u0000 (length includes it), C++ std::string from JSI utf8() also preserves via size. Truncation happens at platform bridge where C-string NUL-terminated APIs are used:
- iOS: RCTAttributedTextUtils.mm:420 stringWithUTF8String:fragment.string.c_str() stops at embedded \0
- iOS: RCTConversions.h RCTNSStringFromString via stringWithCString: and reverse std::string{UTF8String}
- iOS: RCTTurboModule.mm convertJSIStringToNSString same
- Android: JavaTurboModule.cpp NewStringUTF(c_str()) expects NUL-terminated
**Fix (no new API):** Replace the C-string APIs with length-aware equivalents.
On iOS, `[[NSString alloc] initWithBytes:length:encoding:]` for std::string to NSString, and `dataUsingEncoding:` for the reverse, matching the existing correct pattern in FollyConvert.mm:31 and MapBufferBuilder.cpp.
On Android the conversion now goes through UTF-16 in both directions rather than UTF-8:
- outbound: `rt.utf16(...)` then `NewString(jchar*, len)`
- inbound: `GetStringLength()` + `GetStringChars()` then `jsi::String::createFromUtf16(...)`, released with `ReleaseStringChars`
That is worth calling out because it fixes a second latent bug. `NewStringUTF` expects *modified* UTF-8 (CESU-8), so it already mishandled 4-byte sequences such as emoji and other supplementary-plane characters. Going UTF-16 to UTF-16 avoids both problems.
All surfaces (Text, TextInput, accessibility, TurboModule params) are fixed at once because the shared converters are fixed.
## NOTE: nil becomes empty string at two call sites
RCTTurboModule.mm and RCTAttributedTextUtils.mm gain a `?: @""` fallback. Previously `stringWithUTF8String:` returned `nil` for invalid UTF-8 and callers received nil; they now receive `@""`. This matches the fallback `RCTNSStringFromString` already had, and `@""` is safer than nil for the ObjC call sites involved, but it is a behaviour change rather than a pure refactor.
## Changelog:
[GENERAL] [FIXED] - Fix Text truncation when string contains NULL character \u0000 (#24129)
Pull Request resolved: #57906
Test Plan:
**Reproduction (RNTester):**
- Add screen Text > NullCharacter with <Text>{'Hello\u0000World'}</Text> and 'A\u0000B\u0000C'
- Before: "Hello" truncated
- After: "HelloWorld" full (invisible \0 zero-width but World visible, length preserved)
Closes #24129
## Added on import: regression tests
Four cases added to the existing `React/Tests/Text/RCTAttributedTextUtilsTest.mm`, which is owned by `TextTestsApple`:
```
buck2 test fbsource//xplat/js/react-native-github:TextTestsApple
→ Pass 31. Fail 0.
```
Restoring the pre-fix `RCTConversions.h` and `RCTAttributedTextUtils.mm` and re-running fails all four and nothing else:
```
✗ RCTAttributedTextUtilsTest/testNSStringFromStringPreservesEmbeddedNull
✗ RCTAttributedTextUtilsTest/testStringFromNSStringPreservesEmbeddedNull
✗ RCTAttributedTextUtilsTest/testStringConversionRoundTripsEmbeddedNull
✗ RCTAttributedTextUtilsTest/testAttributedStringFromFragmentPreservesEmbeddedNull
Tests finished: Pass 27. Fail 4.
```
They cover `RCTNSStringFromString`, `RCTStringFromNSString`, a round trip through both, and the real AttributedString to NSAttributedString path. The changed function in RCTAttributedTextUtils.mm (`RCTNSAttributedStringFragmentFromFragment`) is static, so that last one goes through the public `RCTNSAttributedStringFromAttributedString`.
This replaces the original `node -p "'a\u0000b'.length"` check, which exercised JavaScript string length rather than any of the changed code.
The Android hunks are not covered by these tests. They are iOS-only test targets and there is no equivalent JNI-level unit test in tree, so the Android side rests on code review.
## On the reverse-direction allocation
`RCTStringFromNSString` now allocates an `NSData` where it previously used `UTF8String` (an interior pointer). That converter has 6 callers. The hot one is `RCTNSStringFromString` with 43 callers, and it does not add an allocation: `stringWithCString:` and `initWithBytes:` both allocate an NSString. Keeping the `NSData` form deliberately, because the cheaper alternative relies on `UTF8String`'s buffer containing embedded NULs, which is exactly the ambiguity this diff removes.
`arc lint -e extra` on the test file reports only pre-existing warnings plus a NULLSAFECLANG infrastructure failure.
Reviewed By: cipolleschi
Differential Revision: D115707536
Pulled By: fabriziocucci
fbshipit-source-id: 4cde8d4b584e0305fcfc3a4520f3690b411500cb
1 parent 93284f5 commit 5906cfb
6 files changed
Lines changed: 87 additions & 13 deletions
File tree
- packages/react-native
- ReactCommon/react
- nativemodule/core/platform
- android/ReactCommon
- ios/ReactCommon
- renderer/textlayoutmanager/platform/ios/react/renderer/textlayoutmanager
- React
- Fabric
- Tests/Text
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
34 | 34 | | |
35 | 35 | | |
36 | 36 | | |
37 | | - | |
| 37 | + | |
| 38 | + | |
38 | 39 | | |
39 | 40 | | |
40 | 41 | | |
| |||
46 | 47 | | |
47 | 48 | | |
48 | 49 | | |
49 | | - | |
| 50 | + | |
| 51 | + | |
| 52 | + | |
| 53 | + | |
| 54 | + | |
| 55 | + | |
| 56 | + | |
| 57 | + | |
50 | 58 | | |
51 | 59 | | |
52 | 60 | | |
| |||
Lines changed: 54 additions & 0 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
9 | 9 | | |
10 | 10 | | |
11 | 11 | | |
| 12 | + | |
12 | 13 | | |
13 | 14 | | |
14 | 15 | | |
15 | 16 | | |
16 | 17 | | |
17 | 18 | | |
| 19 | + | |
18 | 20 | | |
19 | 21 | | |
20 | 22 | | |
| |||
384 | 386 | | |
385 | 387 | | |
386 | 388 | | |
| 389 | + | |
| 390 | + | |
| 391 | + | |
| 392 | + | |
| 393 | + | |
| 394 | + | |
| 395 | + | |
| 396 | + | |
| 397 | + | |
| 398 | + | |
| 399 | + | |
| 400 | + | |
| 401 | + | |
| 402 | + | |
| 403 | + | |
| 404 | + | |
| 405 | + | |
| 406 | + | |
| 407 | + | |
| 408 | + | |
| 409 | + | |
| 410 | + | |
| 411 | + | |
| 412 | + | |
| 413 | + | |
| 414 | + | |
| 415 | + | |
| 416 | + | |
| 417 | + | |
| 418 | + | |
| 419 | + | |
| 420 | + | |
| 421 | + | |
| 422 | + | |
| 423 | + | |
| 424 | + | |
| 425 | + | |
| 426 | + | |
| 427 | + | |
| 428 | + | |
| 429 | + | |
| 430 | + | |
| 431 | + | |
| 432 | + | |
| 433 | + | |
| 434 | + | |
| 435 | + | |
| 436 | + | |
| 437 | + | |
| 438 | + | |
| 439 | + | |
| 440 | + | |
387 | 441 | | |
Lines changed: 12 additions & 7 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
406 | 406 | | |
407 | 407 | | |
408 | 408 | | |
409 | | - | |
410 | | - | |
| 409 | + | |
| 410 | + | |
| 411 | + | |
| 412 | + | |
411 | 413 | | |
412 | 414 | | |
413 | 415 | | |
| |||
785 | 787 | | |
786 | 788 | | |
787 | 789 | | |
788 | | - | |
789 | | - | |
790 | | - | |
791 | | - | |
792 | | - | |
| 790 | + | |
| 791 | + | |
| 792 | + | |
| 793 | + | |
| 794 | + | |
| 795 | + | |
| 796 | + | |
| 797 | + | |
793 | 798 | | |
794 | 799 | | |
795 | 800 | | |
| |||
Lines changed: 3 additions & 1 deletion
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
143 | 143 | | |
144 | 144 | | |
145 | 145 | | |
146 | | - | |
| 146 | + | |
| 147 | + | |
| 148 | + | |
147 | 149 | | |
148 | 150 | | |
149 | 151 | | |
| |||
Lines changed: 4 additions & 1 deletion
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
417 | 417 | | |
418 | 418 | | |
419 | 419 | | |
420 | | - | |
| 420 | + | |
| 421 | + | |
| 422 | + | |
| 423 | + | |
421 | 424 | | |
422 | 425 | | |
423 | 426 | | |
| |||
Lines changed: 4 additions & 2 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
400 | 400 | | |
401 | 401 | | |
402 | 402 | | |
403 | | - | |
| 403 | + | |
404 | 404 | | |
405 | | - | |
| 405 | + | |
| 406 | + | |
| 407 | + | |
406 | 408 | | |
407 | 409 | | |
408 | 410 | | |
| |||
0 commit comments