Fix APP-KDN: preserve NUL bytes in iOS TurboModule strings and clamp blob ranges #101062
+175
−0
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -339,3 +339,20 @@ | |
| - Upstream PR/issue: https://github.com/react/react-native/pull/57546 (merged as `06eb1fe`) | ||
| - 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) | ||
|
Contributor
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
Contributor
Author
There was a problem hiding this comment. Both upstream links are now in
Contributor
There was a problem hiding this comment. @mountiny both are covered now.
|
||
|
|
||
| - Reason: 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 (`stringWithUTF8String:`, `stringWithCString:`, `NewStringUTF`) instead of using the byte length they already had. That is the producer behind the APP-KDN crash: `BlobManager.js` records the blob's `size` from the whole string, native stores the truncated bytes, and `subdataWithRange:` then overruns the stored data the next time that blob is used as a part of another blob. Backports upstream's fix, which replaces those calls with length-aware equivalents — `initWithBytes:length:encoding:` and `dataUsingEncoding:` on iOS, UTF-16 in both directions on Android. | ||
| - Blast radius: this covers **every** string argument crossing into a TurboModule on both platforms, plus Fabric text, not just blobs. Strings without NULs are unaffected; strings that previously arrived truncated now arrive whole. Two call sites (`convertJSIStringToNSString`, `RCTNSAttributedStringFragmentFromFragment`) now return `@""` where invalid UTF-8 previously gave `nil`, which is upstream's behaviour, not a deviation. | ||
| - Upstream PR/issue: [react/react-native#57906](https://github.com/react/react-native/pull/57906) (merged as [`5906cfb`](https://github.com/react/react-native/commit/5906cfb06085a4ae7ae2e5ac5bb190e0c1e90a42), fixes [#24129](https://github.com/react/react-native/issues/24129)) | ||
| - E/App issue: https://github.com/Expensify/App/issues/100843 | ||
| - PR introducing patch: this PR. | ||
| - 0.86.0 migration note: **drop this patch on the RN 0.88 upgrade** — `5906cfb` is absent from `v0.87.1` and ships in `v0.88.0`. Every hunk here is byte-identical to the 0.88 sources, so the drop needs no re-derivation. The commit's `React/Tests/Text/RCTAttributedTextUtilsTest.mm` hunks 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. | ||
|
|
||
| ### [react-native+0.86.0+043+clamp-blob-range-APP-KDN.patch](react-native+0.86.0+043+clamp-blob-range-APP-KDN.patch) | ||
|
|
||
| - Reason: Defence in depth for the same crash (APP-KDN, `NSRangeException: -[NSConcreteMutableData subdataWithRange:]: range {0, 321} exceeds data length 318`). `RCTBlobManager resolve:offset:size:` sliced the stored `NSData` with the JS-supplied offset and size without range-checking them, so any descriptor/data desync raised an `NSRangeException` on a background queue where no JS frame can catch it and the app dies. This clamps the range to the bytes actually stored and logs when it has to — an out-of-bounds offset returns `nil`, an oversized size is truncated — and adds the same logging to the silently-0-byte part paths in `createFromParts:withId:`. `size < 0` still means "the whole blob", preserving the existing `-1` contract. Android is already forgiving here ([`Arrays.copyOfRange`](https://github.com/facebook/react-native/blob/v0.86.0/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/modules/blob/BlobModule.kt)), which is why the crash is iOS-only. | ||
| - Why both patches: patch 042 fixes the producer; on its own this one would turn the crash into silent data loss (a leading NUL resolves to 0 bytes and uploads as an empty attachment as if it had succeeded). It is here so any *future* desync degrades and logs instead of killing the app. `RCTLogWarn` rather than `RCTLogError` deliberately: this runs on a background queue and `RCTLogError` red-boxes on every occurrence in dev. | ||
| - Upstream PR/issue: [react/react-native#58566](https://github.com/react/react-native/pull/58566) — open, not merged. Still unfixed in every release to date: `v0.88.0-rc.1` has the same unchecked `subdataWithRange:`. Re-check on each RN upgrade and drop this patch once the adopted release contains it. | ||
| - E/App issue: https://github.com/Expensify/App/issues/100843 | ||
| - PR introducing patch: this PR. | ||
|
Contributor
There was a problem hiding this comment. Looks like this issue was already handled upstream - react/react-native#57906
Contributor
Author
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 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:
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 — One behaviour change I did not have before. Upstream adds a Two things I deliberately did not do, happy to change either:
The patch was verified to apply cleanly against the 0.86.0 sources in |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,90 @@ | ||
| diff --git a/node_modules/react-native/React/Fabric/RCTConversions.h b/node_modules/react-native/React/Fabric/RCTConversions.h | ||
| --- a/node_modules/react-native/React/Fabric/RCTConversions.h | ||
| +++ b/node_modules/react-native/React/Fabric/RCTConversions.h | ||
| @@ -35,4 +35,5 @@ | ||
| const NSStringEncoding &encoding = NSUTF8StringEncoding) | ||
| { | ||
| - return [NSString stringWithCString:string.c_str() encoding:encoding] ?: @""; | ||
| + NSString *result = [[NSString alloc] initWithBytes:string.data() length:string.size() encoding:encoding]; | ||
| + return result != nil ? result : @""; | ||
| } | ||
| @@ -47,4 +48,11 @@ | ||
| inline std::string RCTStringFromNSString(NSString *string) | ||
| { | ||
| - return std::string{string.UTF8String ?: ""}; | ||
| + if (string == nil) { | ||
| + return ""; | ||
| + } | ||
| + NSData *data = [string dataUsingEncoding:NSUTF8StringEncoding]; | ||
| + if (data == nil) { | ||
| + return ""; | ||
| + } | ||
| + return std::string{static_cast<const char *>(data.bytes), data.length}; | ||
| } | ||
| diff --git a/node_modules/react-native/ReactCommon/react/nativemodule/core/platform/android/ReactCommon/JavaTurboModule.cpp b/node_modules/react-native/ReactCommon/react/nativemodule/core/platform/android/ReactCommon/JavaTurboModule.cpp | ||
| --- a/node_modules/react-native/ReactCommon/react/nativemodule/core/platform/android/ReactCommon/JavaTurboModule.cpp | ||
| +++ b/node_modules/react-native/ReactCommon/react/nativemodule/core/platform/android/ReactCommon/JavaTurboModule.cpp | ||
| @@ -403,5 +403,7 @@ | ||
| "string", argIndex, methodName, arg, &rt); | ||
| } | ||
| - jarg->l = makeGlobalIfNecessary( | ||
| - env->NewStringUTF(arg->getString(rt).utf8(rt).c_str())); | ||
| + auto utf16 = rt.utf16(arg->getString(rt)); | ||
| + jarg->l = makeGlobalIfNecessary(env->NewString( | ||
| + reinterpret_cast<const jchar*>(utf16.data()), | ||
| + static_cast<jsize>(utf16.size()))); | ||
| } else if (type == "Lcom/facebook/react/bridge/Callback;") { | ||
| @@ -751,8 +753,11 @@ | ||
| jsi::Value returnValue = jsi::Value::null(); | ||
| if (returnString != nullptr) { | ||
| - const char* js = env->GetStringUTFChars(returnString, nullptr); | ||
| - std::string result = js; | ||
| - env->ReleaseStringUTFChars(returnString, js); | ||
| - returnValue = | ||
| - jsi::Value(runtime, jsi::String::createFromUtf8(runtime, result)); | ||
| + jsize length = env->GetStringLength(returnString); | ||
| + const jchar* chars = env->GetStringChars(returnString, nullptr); | ||
| + auto jsiString = jsi::String::createFromUtf16( | ||
| + runtime, | ||
| + reinterpret_cast<const char16_t*>(chars), | ||
| + static_cast<size_t>(length)); | ||
| + env->ReleaseStringChars(returnString, chars); | ||
| + returnValue = jsi::Value(runtime, jsiString); | ||
| } | ||
| diff --git a/node_modules/react-native/ReactCommon/react/nativemodule/core/platform/ios/ReactCommon/RCTTurboModule.mm b/node_modules/react-native/ReactCommon/react/nativemodule/core/platform/ios/ReactCommon/RCTTurboModule.mm | ||
| --- a/node_modules/react-native/ReactCommon/react/nativemodule/core/platform/ios/ReactCommon/RCTTurboModule.mm | ||
| +++ b/node_modules/react-native/ReactCommon/react/nativemodule/core/platform/ios/ReactCommon/RCTTurboModule.mm | ||
| @@ -111,4 +111,6 @@ | ||
| static NSString *convertJSIStringToNSString(jsi::Runtime &runtime, const jsi::String &value) | ||
| { | ||
| - return [NSString stringWithUTF8String:value.utf8(runtime).c_str()]; | ||
| + auto utf8 = value.utf8(runtime); | ||
| + NSString *result = [[NSString alloc] initWithBytes:utf8.data() length:utf8.size() encoding:NSUTF8StringEncoding]; | ||
| + return result != nil ? result : @""; | ||
| } | ||
| diff --git a/node_modules/react-native/ReactCommon/react/renderer/textlayoutmanager/platform/ios/react/renderer/textlayoutmanager/RCTAttributedTextUtils.mm b/node_modules/react-native/ReactCommon/react/renderer/textlayoutmanager/platform/ios/react/renderer/textlayoutmanager/RCTAttributedTextUtils.mm | ||
| --- a/node_modules/react-native/ReactCommon/react/renderer/textlayoutmanager/platform/ios/react/renderer/textlayoutmanager/RCTAttributedTextUtils.mm | ||
| +++ b/node_modules/react-native/ReactCommon/react/renderer/textlayoutmanager/platform/ios/react/renderer/textlayoutmanager/RCTAttributedTextUtils.mm | ||
| @@ -431,5 +431,8 @@ | ||
| return [[NSMutableAttributedString attributedStringWithAttachment:attachment] mutableCopy]; | ||
| } else { | ||
| - NSString *string = [NSString stringWithUTF8String:fragment.string.c_str()]; | ||
| + NSString *decoded = [[NSString alloc] initWithBytes:fragment.string.data() | ||
| + length:fragment.string.size() | ||
| + encoding:NSUTF8StringEncoding]; | ||
| + NSString *string = decoded != nil ? decoded : @""; | ||
|
|
||
| if (fragment.textAttributes.textTransform.has_value()) { | ||
| diff --git a/node_modules/react-native/ReactCommon/react/renderer/textlayoutmanager/platform/ios/react/renderer/textlayoutmanager/RCTTextLayoutManager.mm b/node_modules/react-native/ReactCommon/react/renderer/textlayoutmanager/platform/ios/react/renderer/textlayoutmanager/RCTTextLayoutManager.mm | ||
| --- a/node_modules/react-native/ReactCommon/react/renderer/textlayoutmanager/platform/ios/react/renderer/textlayoutmanager/RCTTextLayoutManager.mm | ||
| +++ b/node_modules/react-native/ReactCommon/react/renderer/textlayoutmanager/platform/ios/react/renderer/textlayoutmanager/RCTTextLayoutManager.mm | ||
| @@ -211,5 +211,7 @@ | ||
| CGFloat baseline = [layoutManager locationForGlyphAtIndex:range.location].y; | ||
| - const char *renderedUTF8 = [renderedString UTF8String]; | ||
| + NSData *renderedData = [renderedString dataUsingEncoding:NSUTF8StringEncoding]; | ||
| auto line = LineMeasurement{ | ||
| - std::string(renderedUTF8 != nullptr ? renderedUTF8 : ""), | ||
| + std::string( | ||
| + renderedData != nil ? static_cast<const char *>(renderedData.bytes) : "", | ||
| + renderedData != nil ? renderedData.length : 0), | ||
| rect, |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,68 @@ | ||
| diff --git a/node_modules/react-native/Libraries/Blob/RCTBlobManager.mm b/node_modules/react-native/Libraries/Blob/RCTBlobManager.mm | ||
| --- a/node_modules/react-native/Libraries/Blob/RCTBlobManager.mm | ||
| +++ b/node_modules/react-native/Libraries/Blob/RCTBlobManager.mm | ||
| @@ -12,5 +12,6 @@ | ||
| #import <FBReactNativeSpec/FBReactNativeSpec.h> | ||
| #import <React/RCTConvert.h> | ||
| +#import <React/RCTLog.h> | ||
| #import <React/RCTMockDef.h> | ||
| #import <React/RCTNetworking.h> | ||
| #import <React/RCTUtils.h> | ||
| @@ -111,8 +112,33 @@ | ||
| if (!data) { | ||
| return nil; | ||
| } | ||
| - if (offset != 0 || (size != -1 && size != data.length)) { | ||
| - data = [data subdataWithRange:NSMakeRange(offset, size)]; | ||
| + // The descriptor is computed in JS and can disagree with the bytes that were actually stored. | ||
| + // Clamp the range to what is really here instead of letting `subdataWithRange:` raise an | ||
| + // NSRangeException, which is fatal on this background queue because no JS frame can catch it. | ||
| + const NSInteger length = (NSInteger)data.length; | ||
| + if (offset < 0 || offset > length) { | ||
| + RCTLogWarn( | ||
| + @"[BlobManager] blob %@: offset %ld out of bounds for %ld stored bytes", | ||
| + blobId, | ||
| + (long)offset, | ||
| + (long)length); | ||
| + return nil; | ||
| + } | ||
| + const NSInteger available = length - offset; | ||
| + // A negative size means "the rest of the blob", which is the existing contract for `-1`. | ||
| + if (size < 0 || size > available) { | ||
| + if (size > available) { | ||
| + RCTLogWarn( | ||
| + @"[BlobManager] blob %@: asked for %ld bytes at offset %ld, only %ld stored; truncating", | ||
| + blobId, | ||
| + (long)size, | ||
| + (long)offset, | ||
| + (long)available); | ||
| + } | ||
| + size = available; | ||
| + } | ||
| + if (offset != 0 || size != length) { | ||
| + data = [data subdataWithRange:NSMakeRange((NSUInteger)offset, (NSUInteger)size)]; | ||
| } | ||
| return data; | ||
| } | ||
| @@ -198,8 +224,21 @@ | ||
| if ([type isEqualToString:@"blob"]) { | ||
| NSData *partData = [self resolve:part[@"data"]]; | ||
| + if (!partData) { | ||
| + // A part that cannot be resolved contributes zero bytes, which is how a blob ends up | ||
| + // shorter than the size JS recorded for it. Say so instead of failing silently. | ||
| + RCTLogWarn( | ||
| + @"[BlobManager] blob %@: could not resolve blob part %@, it contributes 0 bytes", | ||
| + blobId, | ||
| + [RCTConvert NSString:part[@"data"][@"blobId"]]); | ||
| + continue; | ||
| + } | ||
| [data appendData:partData]; | ||
| } else if ([type isEqualToString:@"string"]) { | ||
| NSData *partData = [[RCTConvert NSString:part[@"data"]] dataUsingEncoding:NSUTF8StringEncoding]; | ||
| + if (!partData) { | ||
| + RCTLogWarn(@"[BlobManager] blob %@: string part could not be encoded, it contributes 0 bytes", blobId); | ||
| + continue; | ||
| + } | ||
| [data appendData:partData]; | ||
| } else { | ||
| [NSException raise:@"Invalid type for blob" format:@"%@ is invalid", type]; |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please simplify the
Reasons here. They should contain the most crucial information onlyThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Trimmed both in 085470a.
Patch 042's
Reasonis 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-1contract.What I kept and why, in case you'd cut further:
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.