Conversation
…props updateEnteringAnimationTarget runs for every Update mutation without a layout config and cast the view's props to ViewProps before checking for an entering animation. On Android, a <Text> nested in another <Text> forms a view (TextShadowNode sets FormsView) whose props are TextProps, which does not derive from ViewProps and is smaller than the offset of ViewProps::opacity. The read went past the end of the TextProps allocation and crashed with SIGSEGV on allocators that guard it (GrapheneOS hardened_malloc, GWP-ASan). Only read opacity from ViewKind nodes, the same check React Native's LayoutAnimationKeyFrameManager uses before casting to ViewProps.
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (2)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review. 📝 WalkthroughWalkthroughupdateEnteringAnimationTarget now reads opacity from ViewProps only when the node has the ViewKind trait. For other node kinds, it leaves opacity unset. The changelog describes an Android native crash involving nested text opacity. Priority: ➖ Normal Change: Bug fix · Severity of issue fixed: Medium Merge Risk: ⚪ Minimal · up to The change avoids reading view opacity from non-view nodes, and the supplied downstream behavior safely ignores unset opacity. No concrete merge-blocking risk is evident. Architecture SummaryArchitecture risk: 🔵 Low · up to The change affects 1 system. Changed systems: Architecture concerns Review detailsSystems and components
Before / after behavior
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Note
This pull request was authored by AI on behalf of @hirvesh.
Summary
LayoutAnimationsProxyCommon::updateEnteringAnimationTargetreadsopacityby casting the view's props toViewProps. It does this before it checks for an entering animation:Both
LayoutAnimationsProxy::updateLightTreeandLayoutAnimationsProxy_Legacycall this function for everyUpdatemutation that has no layout config.USE_LEGACY_LAYOUT_ANIMATIONS_PROXYtherefore does not avoid it.On Android,
TextShadowNodesetsShadowNodeTraits::Trait::FormsView. A<Text>nested in another<Text>therefore gets mount mutations, and its props areTextProps.TextPropsderives fromProps, notViewProps. It is also smaller than the offset ofViewProps::opacity: in an arm64 release build on RN 0.88,sizeof(TextProps)is0x148andopacityis at offset0x2b0. So on every such update, the read lands 360 bytes past the end of theTextPropsallocation.With the default allocator this usually returns garbage and goes unnoticed. With allocators that guard the memory after an allocation, it crashes. We see this in production on 4.7.0 as a native
SIGSEGVatLayoutAnimationsProxyCommon.cpp:509:This PR reads
opacityonly fromViewKindnodes. That is the check React Native'sLayoutAnimationKeyFrameManagermakes before it casts props toViewProps. For other nodes,opacityisstd::nullopt. Both fields it is assigned to are alreadystd::optional<double>. The existing lookups and assignments are unchanged, so views behave as before.Other
ViewPropscasts on the entering and synchronous-props paths remain, for exampleLayoutAnimationsProxyCommon.cpp:207and:229. They only run for nodes that already have a layout animation, so they do not have this exposure. I left them unchanged, but I'm happy to guard them too if you prefer.Test plan
I reproduced the crash in a production app, not in a minimal example. GWP-ASan attributes the faulting
TextPropsallocation toreanimated::mergePropsinShadowTreeCloner.cpp, so the trigger is a nested<Text>whose props Reanimated updates. The snippet below should exercise the same path, but I have not verified it on its own:The default allocator does not fault on this read, so the crash needs GWP-ASan with every sampled allocation guarded. Run this on a rootable arm64 emulator (a
google_apisimage, API 36):Results, with react-native-reanimated 4.7.0, react-native-worklets 0.13.0 and RN 0.88.0-rc.1 (New Architecture, arm64 release builds):
Buffer Overflow, 360 bytes right of a 352-byte allocationinupdateEnteringAnimationTarget, called fromupdateLightTree. The 352-byte allocation is themake_shared<TextProps>block, and the reported offset is exactly whereViewProps::opacityis read.ViewKindbranch running 14–34 times per launch, always forcomponent=Text, where it previously read out of bounds.This file is byte-identical on
mainand in 4.7.0, so the change applies unchanged.Changelog
yarn changelog:addfor each changed package, or this PR does not changereact-native-reanimatedorreact-native-worklets.