Conversation
…ges (APP-KDN) RCTTurboModule converted every JSI string argument with stringWithUTF8String:, which re-derives the length with strlen and truncates at the first NUL byte. BlobManager.js counts the whole string, so the blob descriptor overran the stored bytes and subdataWithRange: raised a fatal NSRangeException on a background queue. Patch 042 builds the NSString from the known byte length, patch 043 clamps and logs the blob range instead of throwing. Co-authored-by: daledah <daledah@users.noreply.github.com>
|
@MelvinBot please open this PR |
|
🤖 Done — this PR is now out of draft and marked Ready for review.
view run · no recording available |
|
The failing checks are unrelated to this PR — no fix to push here.
The toolkit action pulled in by
Everything that actually exercises this PR passes, including view run · no recording available |
|
@codex review |
|
Codex Review: Didn't find any major issues. Already looking forward to the next diff. Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
Reviewer Checklist
Screenshots/Videosios-resize.mp4 |
|
@MelvinBot i've completed my checklist, please complete the author checklist and update the Tests section with:
|
|
🤖 Web test steps verification results: Plain-text and Unicode/emoji messages sent, persisted, and reopened correctly with the underlying text intact. The image attachment step could not be exercised — file upload is outside agent-device's supported web command surface.
No console errors were observed during the run. Important caveat: this run proves nothing about the fix itself. Both patches are iOS native ( PR body updatedTests section — replaced with your three steps verbatim. Author checklist — I checked off the 19 items I can actually attest to as the PR author: the linked issue, the local testing steps, the code-pattern and comment-quality items, and the blocks that are vacuously N/A for a patch-only PR (no CSS, no assets, no components, no Storybook stories, no deeplinks, no UI changes, no merge from I deliberately left these unchecked, because checking them would put a false testing attestation on a public PR:
Two sections still block "ready for review", and both are human-only:
You already have the iOS build and the recording, so the platform boxes and the two impact boxes are quick for you to tick. Once QA Steps and Offline tests are in, the whole author checklist closes. view run · no recording available |
|
@MelvinBot please update |
|
🤖 Done — QA Steps now reads One thing left before the author checklist closes: Offline tests is still empty, so The platform/testing boxes still need you, since Melvin has no iOS build — the five view run · no recording available |
|
Both failing checks are unrelated to this PR's changes, so I didn't push anything.
The The job failed before it ever read the diff, and its trigger is
Everything that actually exercises the change passed, including view run · no recording available |
|
🚧 mountiny has triggered a test Expensify/App build. You can view the workflow run here. |
| - E/App issue: https://github.com/Expensify/App/issues/97127 | ||
| - PR introducing patch: https://github.com/Expensify/App/pull/98095 | ||
|
|
||
| ### [react-native+0.86.0+042+fix-jsi-string-truncated-at-nul-APP-KDN.patch](react-native+0.86.0+042+fix-jsi-string-truncated-at-nul-APP-KDN.patch) |
There was a problem hiding this comment.
@daledah for both of these, we should create or link the upstream issues now, can you please take a look into that? if we dont do it now, we will never get back to it
There was a problem hiding this comment.
Both upstream links are now in details.md, so this should be covered:
- Patch 042 — Fix Text truncation at NULL character on iOS and Android (#24129), already merged upstream as
5906cfb. It's absent fromv0.87.1and ships inv0.88.0, so this one can be dropped on the 0.88 upgrade. The patch is now a verbatim backport of that commit rather than our own fix (085470a). - Patch 043 — Clamp blob range in RCTBlobManager instead of raising NSRangeException, opened by
daledahand still open (e76a649). The uncheckedsubdataWithRange:is still there as ofv0.88.0-rc.1, so this patch has to stay until the fix lands in a release we adopt.
There was a problem hiding this comment.
@mountiny both are covered now.
- 042 already had one: this turned out to be fixed upstream in react/react-native#57906
- 043 had none, so I created a new one: react/react-native#58566
|
The failing What failed: the Why: that run started at Evidence it isn't this PR: the PR changes only three files, all under How to clear it: The other red check, |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, and Web. Happy testing! 🧪🧪
|
|
Pushed What was actually broken: nothing in this PR. The That run started Why a merge was the fix. Verification
The one remaining red check, |
war-in
left a comment
war-in
left a comment
There was a problem hiding this comment.
Two comments, the rest LGTM
There was a problem hiding this comment.
Looks like this issue was already handled upstream - react/react-native#57906
I think we should implement it the same way and drop the patch when we migrate to 0.88
There was a problem hiding this comment.
Good catch — done in 085470a.
Confirmed it landed: Fix Text truncation at NULL character on iOS and Android (#24129) was imported and merged as 5906cfb. It's absent from v0.87.1 and present in v0.88.0-rc.1, so 0.88 is the right upgrade to drop this on — I verified both by reading RCTTurboModule.mm at each tag.
Patch 042 is now a verbatim backport of that commit instead of our own fix. Two consequences worth flagging:
It grew from 1 file to 5. Upstream fixed the shared converters rather than just the TurboModule one, so the patch now also covers:
| File | What it fixes |
|---|---|
React/Fabric/RCTConversions.h |
RCTNSStringFromString / RCTStringFromNSString, used app-wide by Fabric |
.../ios/.../RCTAttributedTextUtils.mm |
<Text> truncating at a NUL |
.../ios/.../RCTTextLayoutManager.mm |
line measurements reported back to C++ |
.../android/ReactCommon/JavaTurboModule.cpp |
the same truncation on Android, plus NewStringUTF's modified-UTF-8 mishandling of 4-byte sequences |
Every hunk is byte-identical to the 0.88 sources, so the drop needs no re-derivation. I checked the two JSI APIs the Android hunk needs — Runtime::utf16(const String&) and String::createFromUtf16 — and both already exist in 0.86.0's jsi.h, so the backport is API-compatible.
One behaviour change I did not have before. Upstream adds a ?: @"" fallback in convertJSIStringToNSString and RCTNSAttributedStringFragmentFromFragment, so invalid UTF-8 now yields @"" where stringWithUTF8String: returned nil. That's upstream's decision, not a deviation, but it is a real change and it's called out in details.md and the PR description.
Two things I deliberately did not do, happy to change either:
- Omitted the commit's
React/Tests/Text/RCTAttributedTextUtilsTest.mmhunks. That file does ship in the npm package, but it isn't part of any target the App builds, so patching it would add noise and test nothing. Say the word if you'd rather have the patch byte-exact including tests. - Left patch 043 as ours. I checked
v0.88.0-rc.1andRCTBlobManager resolve:offset:size:still has the uncheckedsubdataWithRange:, so there's nothing upstream to backport there. That one still needs an upstream PR filed — which also coversmountiny's request above, for 043 only. I can't open a PR againstreact-nativefrom here, so that one needs a human.
The patch was verified to apply cleanly against the 0.86.0 sources in node_modules, and I confirmed all five patched regions match v0.88.0-rc.1 byte for byte afterwards. I could not run scripts/validatePatches.sh in this environment, but its only check is that each changed patch is linked from details.md, and both still are.
There was a problem hiding this comment.
Please simplify the Reasons here. They should contain the most crucial information only
There was a problem hiding this comment.
Trimmed both in 085470a.
Patch 042's Reason is down from a paragraph with the full repro table to three sentences: what the bug is, why it causes APP-KDN, and what upstream's fix does. Patch 043's dropped the crash-thread and per-branch detail and kept the mechanism, the clamp, and the -1 contract.
What I kept and why, in case you'd cut further:
- The
Blast radiusbullet on 042. It now changes string conversion for every TurboModule argument on both platforms plus Fabric text, and it silently swapsnilfor@""at two call sites. That seemed like the one thing a future reader must not have to rediscover. Why both patcheson 043. Without it, 043 reads as redundant with 042, and the non-obvious part is that 043 alone would convert the crash into a silently empty attachment upload.
The detailed repro table still lives in the PR description and the linked issue, so nothing was lost — just moved out of details.md.
|
@daledah over to you |
|
I'm working on this PR now |
|
@MelvinBot please update this PR this comments above |
…reasons The NUL-truncation bug was already fixed upstream in react/react-native#57906 (merged as 5906cfb, ships in v0.88.0). Replace the hand-rolled single-file patch with a verbatim backport of that commit so it can be dropped on the 0.88 upgrade without re-deriving anything, and shorten the details.md Reason entries for patches 042 and 043 to the essentials. Co-authored-by: daledah <daledah@users.noreply.github.com>
|
Both of 1. Patch 042 is now a verbatim backport of the upstream fix. You were right that this was already handled upstream — Fix Text truncation at NULL character on iOS and Android (#24129) merged as Two things to weigh, since "implement it the same way" changed the shape of the patch:
I omitted the commit's 2. Simplified the Still needs a human: patch 043 has no upstream PR, and I confirmed Verification: the patch applies cleanly against the 0.86.0 sources in |
|
@MelvinBot the upstream PR for patch 043 is now open - react/react-native#58566. Please update |
react/react-native#58566 is now open, so replace the 'not filed' placeholder with the link and keep the note that the change is still absent from every released RN version. Co-authored-by: daledah <daledah@users.noreply.github.com>
|
Done in e76a649. Patch 043's entry in
I kept the "not merged / still unfixed in every release" note rather than just the bare link, so nobody reads the presence of a link as a signal the patch can be dropped yet. I also updated the PR description, which still claimed 043 had no upstream PR. Both patches now have an upstream link, which closes out One thing to flag: I diffed Clamp blob range in RCTBlobManager instead of raising NSRangeException against patch 043, and the logic is identical, but the two have drifted cosmetically — the comment and log-message wording differ, and upstream uses |
4 participants
Explanation of Change
Adds two
react-nativepatches that fix the fatal iOS crash in Sentry APP-KDN:Patch 042 — the cause. Strings crossing the JS/native boundary were truncated at the first NUL byte, because the converters passed
.c_str()to NUL-terminated C-string APIs instead of using the byte length they already had.BlobManager.jscounts the whole string when it records the blob'ssize, so the descriptor and the stored bytes end up disagreeing by exactly "the NUL plus everything after it". The next time that short blob is used as a part of another blob,subdataWithRange:overruns the stored data and raisesNSRangeExceptiononcom.meta.react.turbomodulemanager.queue, where no JS frame can catch it, so the app dies.This was already fixed upstream (merged as
5906cfb), so patch 042 is now a verbatim backport of that commit rather than an Expensify-authored fix. It replaces the C-string calls with length-aware equivalents —initWithBytes:length:encoding:anddataUsingEncoding:on iOS, UTF-16 in both directions on Android — across the five files upstream touched:ReactCommon/.../ios/ReactCommon/RCTTurboModule.mmReact/Fabric/RCTConversions.hRCTNSStringFromString/RCTStringFromNSString, used app-wide by FabricReactCommon/.../ios/.../RCTAttributedTextUtils.mm<Text>truncating at a NULReactCommon/.../ios/.../RCTTextLayoutManager.mmReactCommon/.../android/ReactCommon/JavaTurboModule.cppNewStringUTF's modified-UTF-8 mishandling of 4-byte sequences5906cfbis absent fromv0.87.1and ships inv0.88.0, so this patch can be dropped on the RN 0.88 upgrade. Every hunk is byte-identical to the 0.88 sources. The commit'sReact/Tests/Text/RCTAttributedTextUtilsTest.mmhunks are omitted — that file ships in the npm package but is not part of any target the App builds, so patching it would add noise and test nothing.The original reproduction still holds and is what pinned the mechanism. Reproduced deterministically on an iOS simulator (RN 0.86.0, New Arch) by
daledah— every case below builds to the same 318-byte JS count, so the only variable is what native stored:A×318A×315 +\0+BCA×317 +\0\0+A×317A×316/315/314 +é/中/😀The 3-byte case is the exact shortfall in the crash message. Multi-byte UTF-8 round-trips fine, emoji included — which rules out suspected cause 2 in the issue body.
Patch 043 — defence in depth.
RCTBlobManager resolve:offset:size:sliced the storedNSDatawith the JS-supplied offset and size without range-checking them. It now clamps the range to the bytes actually stored (out-of-bounds offset returnsnil; an oversized size is truncated to what is available) and logs both cases with the blob ID and the two lengths.size < 0still means "the whole blob", preserving the existing-1contract.createFromParts:withId:also gains nil guards with logs, where an unresolvable blob part or an unencodable string part previously contributed 0 bytes with no signal at all. This matches Android's forgivingArrays.copyOfRangebehaviour, which is why the crash is iOS-only.Both patches are needed: on its own, patch 043 converts the crash into silent data loss — with a leading NUL the blob resolves to 0 bytes and uploads as an empty attachment as if it had succeeded. Patch 042 fixes the producer; patch 043 makes any future desync degrade and log instead of killing the app. The logs use
RCTLogWarnrather thanRCTLogErrordeliberately: this runs on a background queue, andRCTLogErrorred-boxes on every occurrence in dev.Blast radius to weigh in review: patch 042 is now broader than the blob path — it changes string conversion for every TurboModule argument on both platforms, plus Fabric text. Behaviour is unchanged for strings without NULs, and strings that previously arrived silently truncated now arrive whole. Two call sites (
convertJSIStringToNSString,RCTNSAttributedStringFragmentFromFragment) now return@""where invalid UTF-8 previously gavenil; that is upstream's behaviour, not a deviation.Upstream: both patches now have an upstream link in
details.md. Patch 042 is react/react-native#57906, already merged and shipping in 0.88, so it can be dropped on that upgrade. Patch 043 is react/react-native#58566, open and not merged — the uncheckedsubdataWithRange:is still present as ofv0.88.0-rc.1, so that patch has to stay until the fix lands in a release we adopt.Still open (does not block this fix): what puts a NUL into an App string in production. The best lead is
whatwg-fetch'sResponse.blob()doingnew Blob([this._bodyText]), where binary content read as text would carry NULs.Fixed Issues
$ #100843
PROPOSAL: #100843 (comment)
Tests
Offline tests
QA Steps
Same as tests.
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectionAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari