From 5ecac9dd4fee836a2138adc33c56cff35d141952 Mon Sep 17 00:00:00 2001 From: Bartlomiej Bloniarz Date: Wed, 18 Feb 2026 03:24:05 -0800 Subject: [PATCH] Fix unsafe rawPointer access in cloneMultiple Summary: 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. Differential Revision: D93596770 --- .../AnimatedPropsRegistry.cpp | 2 +- .../animationbackend/AnimatedPropsRegistry.h | 6 ++--- .../animationbackend/AnimationBackend.cpp | 5 ++-- .../AnimationBackendCommitHook.cpp | 2 +- .../react/renderer/core/ShadowNode.cpp | 26 +++++++++---------- .../react/renderer/core/ShadowNode.h | 2 +- .../renderer/core/tests/ShadowNodeTest.cpp | 8 +++--- 7 files changed, 26 insertions(+), 25 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..7c90628a0ee6 100644 --- a/packages/react-native/ReactCommon/react/renderer/animationbackend/AnimatedPropsRegistry.cpp +++ b/packages/react-native/ReactCommon/react/renderer/animationbackend/AnimatedPropsRegistry.cpp @@ -55,7 +55,7 @@ void AnimatedPropsRegistry::update( } } -std::pair&, SnapshotMap&> +std::pair&, 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..fcd71f2fb124 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 5bff2e5d119e..719acfdda1e7 100644 --- a/packages/react-native/ReactCommon/react/renderer/animationbackend/AnimationBackend.cpp +++ b/packages/react-native/ReactCommon/react/renderer/animationbackend/AnimationBackend.cpp @@ -70,7 +70,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); } } @@ -145,7 +145,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..9c217189b19d 100644 --- a/packages/react-native/ReactCommon/react/renderer/animationbackend/AnimationBackendCommitHook.cpp +++ b/packages/react-native/ReactCommon/react/renderer/animationbackend/AnimationBackendCommitHook.cpp @@ -43,7 +43,7 @@ RootShadowNode::Unshared AnimationBackendCommitHook::shadowTreeWillCommit( const ShadowNodeFragment& fragment) { auto newProps = ShadowNodeFragment::propsPlaceholder(); std::shared_ptr viewProps = nullptr; - if (surfaceFamilies.contains(&shadowNode.getFamily()) && + if (surfaceFamilies.contains(shadowNode.getFamilyShared()) && updates.contains(shadowNode.getTag())) { auto& snapshot = updates.at(shadowNode.getTag()); if (!snapshot->propNames.empty() || snapshot->rawProps) { diff --git a/packages/react-native/ReactCommon/react/renderer/core/ShadowNode.cpp b/packages/react-native/ReactCommon/react/renderer/core/ShadowNode.cpp index 9b704b1a3eb9..2a673a19475e 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,38 @@ 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 ca0c1be6762e..58bb2db128eb 100644 --- a/packages/react-native/ReactCommon/react/renderer/core/ShadowNode.h +++ b/packages/react-native/ReactCommon/react/renderer/core/ShadowNode.h @@ -116,7 +116,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,