From 41f59c4231199d545743dfbc227a79ab993afe7d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dawid=20Ma=C5=82ecki?= Date: Tue, 3 Mar 2026 06:18:20 -0800 Subject: [PATCH] Fix unsafe rawPointer access in cloneMultiple (#55613) Summary: Pull Request resolved: https://github.com/facebook/react-native/pull/55613 The cloneMultiple method was written in a way to accept a list of families that are presumed to be owned by the api caller. This designe was mostly aimed at reanimated, that holds refernces to ShadowNodes (that own these families). In the case of AnimationBackend this didn't work properly, as when the view is unmounted we would lose the ShadowNodeFamily shared_ptr that we hold and it could get deallocated. Since now we can get an owning reference to ShadowNodeFamily from ShadowNode::getFamilyShared, we don't have to keep this old unsafe api. Instead we require the caller to have an owning reference with the api itself. This `cloneMultiple` method isn't really adopted in the community, so the breaking change shouldn't be a big problem. Reviewed By: zeyap, javache Differential Revision: D93596770 --- .../AnimatedPropsRegistry.cpp | 4 ++- .../animationbackend/AnimatedPropsRegistry.h | 6 ++--- .../animationbackend/AnimationBackend.cpp | 5 ++-- .../AnimationBackendCommitHook.cpp | 7 ++--- .../react/renderer/core/ShadowNode.cpp | 27 ++++++++++--------- .../react/renderer/core/ShadowNode.h | 2 +- .../renderer/core/tests/ShadowNodeTest.cpp | 8 +++--- 7 files changed, 32 insertions(+), 27 deletions(-) diff --git a/packages/react-native/ReactCommon/react/renderer/animationbackend/AnimatedPropsRegistry.cpp b/packages/react-native/ReactCommon/react/renderer/animationbackend/AnimatedPropsRegistry.cpp index 09ee3620c6fd..041eac315d5e 100644 --- a/packages/react-native/ReactCommon/react/renderer/animationbackend/AnimatedPropsRegistry.cpp +++ b/packages/react-native/ReactCommon/react/renderer/animationbackend/AnimatedPropsRegistry.cpp @@ -55,7 +55,9 @@ void AnimatedPropsRegistry::update( } } -std::pair&, SnapshotMap&> +std::pair< + std::unordered_set>&, + SnapshotMap&> AnimatedPropsRegistry::getMap(SurfaceId surfaceId) { auto lock = std::lock_guard(mutex_); auto& [pendingMap, map, pendingFamilies, families] = diff --git a/packages/react-native/ReactCommon/react/renderer/animationbackend/AnimatedPropsRegistry.h b/packages/react-native/ReactCommon/react/renderer/animationbackend/AnimatedPropsRegistry.h index ead82a635393..f577519762b8 100644 --- a/packages/react-native/ReactCommon/react/renderer/animationbackend/AnimatedPropsRegistry.h +++ b/packages/react-native/ReactCommon/react/renderer/animationbackend/AnimatedPropsRegistry.h @@ -24,11 +24,11 @@ struct PropsSnapshot { struct SurfaceContext { std::unordered_map> pendingMap, map; - std::unordered_set pendingFamilies, families; + std::unordered_set> pendingFamilies, families; }; struct SurfaceUpdates { - std::unordered_set families; + std::unordered_set> families; std::unordered_map propsMap; bool hasLayoutUpdates{false}; }; @@ -39,7 +39,7 @@ class AnimatedPropsRegistry { public: void update(const std::unordered_map &surfaceUpdates); void clear(SurfaceId surfaceId); - std::pair &, SnapshotMap &> getMap(SurfaceId surfaceId); + std::pair> &, SnapshotMap &> getMap(SurfaceId surfaceId); private: std::unordered_map surfaceContexts_; diff --git a/packages/react-native/ReactCommon/react/renderer/animationbackend/AnimationBackend.cpp b/packages/react-native/ReactCommon/react/renderer/animationbackend/AnimationBackend.cpp index 3ad43b8e0210..9fb2e5ecf94d 100644 --- a/packages/react-native/ReactCommon/react/renderer/animationbackend/AnimationBackend.cpp +++ b/packages/react-native/ReactCommon/react/renderer/animationbackend/AnimationBackend.cpp @@ -71,7 +71,7 @@ void AnimationBackend::onAnimationFrame(AnimationTimestamp timestamp) { auto& [families, updates, hasLayoutUpdates] = surfaceUpdates[family->getSurfaceId()]; hasLayoutUpdates |= mutation.hasLayoutUpdates; - families.insert(family.get()); + families.insert(family); updates[mutation.tag] = std::move(mutation.props); } } @@ -146,7 +146,8 @@ void AnimationBackend::commitUpdates( const ShadowNode& shadowNode, const ShadowNodeFragment& fragment) { auto newProps = ShadowNodeFragment::propsPlaceholder(); - if (surfaceFamilies.contains(&shadowNode.getFamily())) { + if (surfaceFamilies.contains( + shadowNode.getFamilyShared())) { auto& animatedProps = updates.at(shadowNode.getTag()); newProps = cloneProps(animatedProps, shadowNode); } diff --git a/packages/react-native/ReactCommon/react/renderer/animationbackend/AnimationBackendCommitHook.cpp b/packages/react-native/ReactCommon/react/renderer/animationbackend/AnimationBackendCommitHook.cpp index 4a00926b0e2d..cb547d9e367d 100644 --- a/packages/react-native/ReactCommon/react/renderer/animationbackend/AnimationBackendCommitHook.cpp +++ b/packages/react-native/ReactCommon/react/renderer/animationbackend/AnimationBackendCommitHook.cpp @@ -43,9 +43,10 @@ RootShadowNode::Unshared AnimationBackendCommitHook::shadowTreeWillCommit( const ShadowNodeFragment& fragment) { auto newProps = ShadowNodeFragment::propsPlaceholder(); std::shared_ptr viewProps = nullptr; - if (surfaceFamilies.contains(&shadowNode.getFamily()) && - updates.contains(shadowNode.getTag())) { - auto& snapshot = updates.at(shadowNode.getTag()); + if (auto updatesIter = updates.find(shadowNode.getTag()); + updatesIter != updates.end() && + surfaceFamilies.contains(shadowNode.getFamilyShared())) { + auto& snapshot = updatesIter->second; if (!snapshot->propNames.empty() || snapshot->rawProps) { PropsParserContext propsParserContext{ shadowNode.getSurfaceId(), diff --git a/packages/react-native/ReactCommon/react/renderer/core/ShadowNode.cpp b/packages/react-native/ReactCommon/react/renderer/core/ShadowNode.cpp index 168dd4079d4d..acbc5aea1cf2 100644 --- a/packages/react-native/ReactCommon/react/renderer/core/ShadowNode.cpp +++ b/packages/react-native/ReactCommon/react/renderer/core/ShadowNode.cpp @@ -413,17 +413,16 @@ namespace { std::shared_ptr cloneMultipleRecursive( const ShadowNode& shadowNode, - const std::unordered_map& childrenCount, + const std::unordered_map& childrenCount, const std::function(const ShadowNode&, const ShadowNodeFragment&)>& callback) { - const auto* family = &shadowNode.getFamily(); auto& children = shadowNode.getChildren(); std::shared_ptr>> newChildren; - auto count = childrenCount.at(family); + auto count = childrenCount.at(shadowNode.getTag()); for (size_t i = 0; count > 0 && i < children.size(); i++) { - const auto childFamily = &children[i]->getFamily(); - if (childrenCount.contains(childFamily)) { + const auto childTag = children[i]->getTag(); + if (childrenCount.contains(childTag)) { count--; if (!newChildren) { newChildren = @@ -441,37 +440,39 @@ std::shared_ptr cloneMultipleRecursive( } // namespace std::shared_ptr ShadowNode::cloneMultiple( - const std::unordered_set& familiesToUpdate, + const std::unordered_set>& + familiesToUpdate, const std::function( const ShadowNode& oldShadowNode, const ShadowNodeFragment& fragment)>& callback) const { - std::unordered_map childrenCount; + std::unordered_map childrenCount; for (const auto& family : familiesToUpdate) { - if (childrenCount.contains(family)) { + if (childrenCount.contains(family->getTag())) { continue; } - childrenCount[family] = 0; + childrenCount[family->getTag()] = 0; auto ancestor = family->parent_.lock(); while ((ancestor != nullptr) && ancestor != family_) { - auto ancestorIt = childrenCount.find(ancestor.get()); + auto ancestorTag = ancestor->getTag(); + auto ancestorIt = childrenCount.find(ancestorTag); if (ancestorIt != childrenCount.end()) { ancestorIt->second++; break; } - childrenCount[ancestor.get()] = 1; + childrenCount[ancestorTag] = 1; ancestor = ancestor->parent_.lock(); } if (ancestor == family_) { - childrenCount[ancestor.get()]++; + childrenCount[ancestor->getTag()]++; } } - if (!childrenCount.contains(&this->getFamily())) { + if (!childrenCount.contains(getTag())) { return nullptr; } diff --git a/packages/react-native/ReactCommon/react/renderer/core/ShadowNode.h b/packages/react-native/ReactCommon/react/renderer/core/ShadowNode.h index a27d70aae7b2..4a804c63dfbc 100644 --- a/packages/react-native/ReactCommon/react/renderer/core/ShadowNode.h +++ b/packages/react-native/ReactCommon/react/renderer/core/ShadowNode.h @@ -109,7 +109,7 @@ class ShadowNode : public Sealable, public DebugStringConvertible, public jsi::N * Returns `nullptr` if the operation cannot be performed successfully. */ std::shared_ptr cloneMultiple( - const std::unordered_set &familiesToUpdate, + const std::unordered_set> &familiesToUpdate, const std::function< std::shared_ptr(const ShadowNode &oldShadowNode, const ShadowNodeFragment &fragment)> &callback) const; diff --git a/packages/react-native/ReactCommon/react/renderer/core/tests/ShadowNodeTest.cpp b/packages/react-native/ReactCommon/react/renderer/core/tests/ShadowNodeTest.cpp index bc4c5eb2b54e..cc53b101e115 100644 --- a/packages/react-native/ReactCommon/react/renderer/core/tests/ShadowNodeTest.cpp +++ b/packages/react-native/ReactCommon/react/renderer/core/tests/ShadowNodeTest.cpp @@ -316,7 +316,7 @@ TEST_F(ShadowNodeTest, handleRuntimeReferenceTransferOnClone) { TEST_F(ShadowNodeTest, cloneMultiple) { auto newProps = std::make_shared(); auto newRoot = nodeA_->cloneMultiple( - {&nodeA_->getFamily(), &nodeAB_->getFamily()}, + {nodeA_->getFamilyShared(), nodeAB_->getFamilyShared()}, [&](const ShadowNode& oldShadowNode, const ShadowNodeFragment& fragment) { return oldShadowNode.clone({ .props = newProps, @@ -346,7 +346,7 @@ TEST_F(ShadowNodeTest, cloneMultiple) { TEST_F(ShadowNodeTest, cloneMultipleWithSingleFamily) { auto newProps = std::make_shared(); auto newRoot = nodeA_->cloneMultiple( - {&nodeAB_->getFamily()}, + {nodeAB_->getFamilyShared()}, [&](const ShadowNode& oldShadowNode, const ShadowNodeFragment& fragment) { return oldShadowNode.clone({ .props = newProps, @@ -379,7 +379,7 @@ TEST_F(ShadowNodeTest, cloneMultipleReturnsNullptrWhenFamilyHasNoPathToRoot) { auto newProps = std::make_shared(); // nodeZ_ is not part of nodeA_'s tree auto result = nodeA_->cloneMultiple( - {&nodeZ_->getFamily()}, + {nodeZ_->getFamilyShared()}, [&](const ShadowNode& oldShadowNode, const ShadowNodeFragment& fragment) { return oldShadowNode.clone({ .props = newProps, @@ -396,7 +396,7 @@ TEST_F(ShadowNodeTest, cloneMultipleWithMixOfValidAndInvalidFamilies) { auto newProps = std::make_shared(); // nodeAB_ is in the tree, nodeZ_ is not auto result = nodeA_->cloneMultiple( - {&nodeAB_->getFamily(), &nodeZ_->getFamily()}, + {nodeAB_->getFamilyShared(), nodeZ_->getFamilyShared()}, [&](const ShadowNode& oldShadowNode, const ShadowNodeFragment& fragment) { return oldShadowNode.clone({ .props = newProps,