From c2a089fddfe0706db468e54e11e28a8fa1b446d7 Mon Sep 17 00:00:00 2001 From: Nick Gerleman Date: Mon, 27 Feb 2023 10:07:51 -0800 Subject: [PATCH] Better ShadowNode memory safety (#36258) Summary: Pull Request resolved: https://github.com/facebook/react-native/pull/36258 This fixes a few instances where YogaLayoutableShadowNode (or general shadownode casting) could offer better memory safety. 1. The reference form of traitCast() now terminates on invalid cast, instead of debug assert, since it is better to crash in production than to corrupt memory (which will crash somewhere later, in a much more confusing way). 2. We use traitCast() in more places where we previously would static_cast. This means needing to formally add a mutable version. 3. We bounds-check yoga children access in a single place by using `std::vector` `at()` instead of `[]`. 4. Removed `Trait::UnreservedTrait1` API, since multiple libraries using it can collide and we lose the memory safety benefits of `traitCast`. This change is in response to a bug where `YogaLayoutableShadowNode` may perform an invalid `static_cast` of `RawTextShadowNode` if a text or number is rendered directly inside of a `` (instead of a `` element). This does not yet fix the underlying logic of YogaLayoutableShadowNode to act gracefully when a RawTextShadowNode makes its way into children. We just terminate, instead of corrupting memory. Changelog: [General][Breaking] - Better Fabric ShadowNode Memory Safety (Removes `Trait::UnreservedTrait` API) Reviewed By: javache Differential Revision: D43271779 fbshipit-source-id: 727c1230f72664bf4d261871c66ca61ddf0d5ffa --- .../components/text/BaseTextShadowNode.cpp | 1 + .../components/text/ParagraphShadowNode.cpp | 5 +- .../components/text/RawTextShadowNode.h | 29 +----- .../renderer/components/text/TextShadowNode.h | 28 ++---- .../view/YogaLayoutableShadowNode.cpp | 39 ++++---- .../view/YogaLayoutableShadowNode.h | 26 +----- .../renderer/core/LayoutableShadowNode.cpp | 7 +- .../renderer/core/LayoutableShadowNode.h | 25 +----- ReactCommon/react/renderer/core/ShadowNode.h | 16 ---- .../react/renderer/core/ShadowNodeTraits.h | 17 ++-- ReactCommon/react/renderer/core/TraitCast.h | 90 +++++++++++++++++++ .../renderer/core/tests/traitCastTest.cpp | 14 ++- .../react/renderer/mounting/ShadowView.cpp | 1 + .../react/renderer/uimanager/UIManager.cpp | 1 + .../renderer/uimanager/UIManagerBinding.cpp | 1 + 15 files changed, 156 insertions(+), 144 deletions(-) create mode 100644 ReactCommon/react/renderer/core/TraitCast.h diff --git a/ReactCommon/react/renderer/components/text/BaseTextShadowNode.cpp b/ReactCommon/react/renderer/components/text/BaseTextShadowNode.cpp index b0758dcd6e8..0be6cdbfb01 100644 --- a/ReactCommon/react/renderer/components/text/BaseTextShadowNode.cpp +++ b/ReactCommon/react/renderer/components/text/BaseTextShadowNode.cpp @@ -11,6 +11,7 @@ #include #include #include +#include #include namespace facebook::react { diff --git a/ReactCommon/react/renderer/components/text/ParagraphShadowNode.cpp b/ReactCommon/react/renderer/components/text/ParagraphShadowNode.cpp index 48b3a2128f7..7efe83c9e3e 100644 --- a/ReactCommon/react/renderer/components/text/ParagraphShadowNode.cpp +++ b/ReactCommon/react/renderer/components/text/ParagraphShadowNode.cpp @@ -13,6 +13,7 @@ #include #include #include +#include #include #include @@ -205,8 +206,8 @@ void ParagraphShadowNode::layout(LayoutContext layoutContext) { paragraphShadowNode = static_cast(paragraphOwningShadowNode.get()); - auto &layoutableShadowNode = const_cast( - traitCast(*clonedShadowNode)); + auto &layoutableShadowNode = + traitCast(*clonedShadowNode); auto attachmentFrame = measurement.attachments[i].frame; auto attachmentSize = roundToPixel<&ceil>( diff --git a/ReactCommon/react/renderer/components/text/RawTextShadowNode.h b/ReactCommon/react/renderer/components/text/RawTextShadowNode.h index 11cda560918..bd62a4af3f1 100644 --- a/ReactCommon/react/renderer/components/text/RawTextShadowNode.h +++ b/ReactCommon/react/renderer/components/text/RawTextShadowNode.h @@ -29,34 +29,13 @@ class RawTextShadowNode : public ConcreteShadowNode< using ConcreteShadowNode::ConcreteShadowNode; static ShadowNodeTraits BaseTraits() { auto traits = ConcreteShadowNode::BaseTraits(); - traits.set(ShadowNodeTraits::Trait::RawText); + traits.set(IdentifierTrait()); return traits; } + static ShadowNodeTraits::Trait IdentifierTrait() { + return ShadowNodeTraits::Trait::RawText; + } }; -template <> -inline RawTextShadowNode const &traitCast( - ShadowNode const &shadowNode) { - bool castable = - shadowNode.getTraits().check(ShadowNodeTraits::Trait::RawText); - react_native_assert(castable); - (void)castable; - return static_cast(shadowNode); -} - -template <> -inline RawTextShadowNode const *traitCast( - ShadowNode const *shadowNode) { - if (!shadowNode) { - return nullptr; - } - bool castable = - shadowNode->getTraits().check(ShadowNodeTraits::Trait::RawText); - if (!castable) { - return nullptr; - } - return static_cast(shadowNode); -} - } // namespace react } // namespace facebook diff --git a/ReactCommon/react/renderer/components/text/TextShadowNode.h b/ReactCommon/react/renderer/components/text/TextShadowNode.h index cefad906151..3c252a212f5 100644 --- a/ReactCommon/react/renderer/components/text/TextShadowNode.h +++ b/ReactCommon/react/renderer/components/text/TextShadowNode.h @@ -34,11 +34,15 @@ class TextShadowNode : public ConcreteShadowNode< #ifdef ANDROID traits.set(ShadowNodeTraits::Trait::FormsView); #endif - traits.set(ShadowNodeTraits::Trait::Text); + traits.set(IdentifierTrait()); return traits; } + static ShadowNodeTraits::Trait IdentifierTrait() { + return ShadowNodeTraits::Trait::Text; + } + using ConcreteShadowNode::ConcreteShadowNode; #ifdef ANDROID @@ -58,27 +62,5 @@ class TextShadowNode : public ConcreteShadowNode< #endif }; -template <> -inline TextShadowNode const &traitCast( - ShadowNode const &shadowNode) { - bool castable = shadowNode.getTraits().check(ShadowNodeTraits::Trait::Text); - react_native_assert(castable); - (void)castable; - return static_cast(shadowNode); -} - -template <> -inline TextShadowNode const *traitCast( - ShadowNode const *shadowNode) { - if (!shadowNode) { - return nullptr; - } - bool castable = shadowNode->getTraits().check(ShadowNodeTraits::Trait::Text); - if (!castable) { - return nullptr; - } - return static_cast(shadowNode); -} - } // namespace react } // namespace facebook diff --git a/ReactCommon/react/renderer/components/view/YogaLayoutableShadowNode.cpp b/ReactCommon/react/renderer/components/view/YogaLayoutableShadowNode.cpp index 306fd68cd10..57758be73c6 100644 --- a/ReactCommon/react/renderer/components/view/YogaLayoutableShadowNode.cpp +++ b/ReactCommon/react/renderer/components/view/YogaLayoutableShadowNode.cpp @@ -13,6 +13,7 @@ #include #include #include +#include #include #include #include @@ -61,10 +62,14 @@ thread_local LayoutContext threadLocalLayoutContext; ShadowNodeTraits YogaLayoutableShadowNode::BaseTraits() { auto traits = LayoutableShadowNode::BaseTraits(); - traits.set(ShadowNodeTraits::Trait::YogaLayoutableKind); + traits.set(IdentifierTrait()); return traits; } +ShadowNodeTraits::Trait YogaLayoutableShadowNode::IdentifierTrait() { + return ShadowNodeTraits::Trait::YogaLayoutableKind; +} + YogaLayoutableShadowNode::YogaLayoutableShadowNode( ShadowNodeFragment const &fragment, ShadowNodeFamily::Shared const &family, @@ -205,7 +210,7 @@ void YogaLayoutableShadowNode::adoptYogaChild(size_t index) { // TODO: At this point, React has wrong reference to the node. (T138668036) auto clonedChildNode = childNode.clone({}); auto &layoutableClonedChildNode = - traitCast(*clonedChildNode); + traitCast(*clonedChildNode); // The owner must be nullptr for a newly cloned node. react_native_assert( @@ -298,7 +303,7 @@ void YogaLayoutableShadowNode::updateYogaChildren() { adoptYogaChild(i); if (isClean) { - auto &oldYogaChildNode = *oldYogaChildren[i]; + auto &oldYogaChildNode = *oldYogaChildren.at(i); auto &newYogaChildNode = traitCast(*getChildren().at(i)) .yogaNode_; @@ -550,8 +555,7 @@ void YogaLayoutableShadowNode::layout(LayoutContext layoutContext) { auto contentFrame = Rect{}; for (auto childYogaNode : yogaNode_.getChildren()) { - auto &childNode = - *static_cast(childYogaNode->getContext()); + auto &childNode = shadowNodeFromContext(childYogaNode); // Verifying that the Yoga node belongs to the ShadowNode. react_native_assert(&childNode.yogaNode_ == childYogaNode); @@ -635,17 +639,15 @@ YGNode *YogaLayoutableShadowNode::yogaNodeCloneCallbackConnector( // At this point it is guaranteed that all shadow nodes associated with yoga // nodes are `YogaLayoutableShadowNode` subclasses. - auto parentNode = - static_cast(parentYogaNode->getContext()); - auto oldNode = - static_cast(oldYogaNode->getContext()); + auto &parentNode = shadowNodeFromContext(parentYogaNode); + auto &oldNode = shadowNodeFromContext(oldYogaNode); - auto clonedNode = oldNode->clone( + auto clonedNode = oldNode.clone( {ShadowNodeFragment::propsPlaceholder(), ShadowNodeFragment::childrenPlaceholder(), - oldNode->getState()}); - parentNode->replaceChild(*oldNode, clonedNode, childIndex); - return &static_cast(*clonedNode).yogaNode_; + oldNode.getState()}); + parentNode.replaceChild(oldNode, clonedNode, childIndex); + return &traitCast(*clonedNode).yogaNode_; } YGSize YogaLayoutableShadowNode::yogaNodeMeasureCallbackConnector( @@ -657,8 +659,7 @@ YGSize YogaLayoutableShadowNode::yogaNodeMeasureCallbackConnector( SystraceSection s( "YogaLayoutableShadowNode::yogaNodeMeasureCallbackConnector"); - auto shadowNodeRawPtr = - static_cast(yogaNode->getContext()); + auto &shadowNode = shadowNodeFromContext(yogaNode); auto minimumSize = Size{0, 0}; auto maximumSize = Size{ @@ -689,13 +690,19 @@ YGSize YogaLayoutableShadowNode::yogaNodeMeasureCallbackConnector( break; } - auto size = shadowNodeRawPtr->measureContent( + auto size = shadowNode.measureContent( threadLocalLayoutContext, {minimumSize, maximumSize}); return YGSize{ yogaFloatFromFloat(size.width), yogaFloatFromFloat(size.height)}; } +YogaLayoutableShadowNode &YogaLayoutableShadowNode::shadowNodeFromContext( + YGNode *yogaNode) { + return traitCast( + *static_cast(yogaNode->getContext())); +} + YGConfig &YogaLayoutableShadowNode::initializeYogaConfig(YGConfig &config) { config.setCloneNodeCallback( YogaLayoutableShadowNode::yogaNodeCloneCallbackConnector); diff --git a/ReactCommon/react/renderer/components/view/YogaLayoutableShadowNode.h b/ReactCommon/react/renderer/components/view/YogaLayoutableShadowNode.h index f836f1504e3..e76f593a162 100644 --- a/ReactCommon/react/renderer/components/view/YogaLayoutableShadowNode.h +++ b/ReactCommon/react/renderer/components/view/YogaLayoutableShadowNode.h @@ -31,6 +31,7 @@ class YogaLayoutableShadowNode : public LayoutableShadowNode { kShadowNodeChildrenSmallVectorSize>; static ShadowNodeTraits BaseTraits(); + static ShadowNodeTraits::Trait IdentifierTrait(); #pragma mark - Constructors @@ -139,6 +140,7 @@ class YogaLayoutableShadowNode : public LayoutableShadowNode { YGMeasureMode widthMode, float height, YGMeasureMode heightMode); + static YogaLayoutableShadowNode &shadowNodeFromContext(YGNode *yogaNode); #pragma mark - RTL Legacy Autoflip @@ -190,29 +192,5 @@ class YogaLayoutableShadowNode : public LayoutableShadowNode { void ensureYogaChildrenLookFine() const; }; -template <> -inline YogaLayoutableShadowNode const & -traitCast(ShadowNode const &shadowNode) { - bool castable = - shadowNode.getTraits().check(ShadowNodeTraits::Trait::YogaLayoutableKind); - react_native_assert(castable); - (void)castable; - return static_cast(shadowNode); -} - -template <> -inline YogaLayoutableShadowNode const * -traitCast(ShadowNode const *shadowNode) { - if (!shadowNode) { - return nullptr; - } - bool castable = shadowNode->getTraits().check( - ShadowNodeTraits::Trait::YogaLayoutableKind); - if (!castable) { - return nullptr; - } - return static_cast(shadowNode); -} - } // namespace react } // namespace facebook diff --git a/ReactCommon/react/renderer/core/LayoutableShadowNode.cpp b/ReactCommon/react/renderer/core/LayoutableShadowNode.cpp index 2c086283c0f..900a8c20f06 100644 --- a/ReactCommon/react/renderer/core/LayoutableShadowNode.cpp +++ b/ReactCommon/react/renderer/core/LayoutableShadowNode.cpp @@ -11,6 +11,7 @@ #include #include #include +#include #include #include @@ -221,10 +222,14 @@ LayoutMetrics LayoutableShadowNode::computeRelativeLayoutMetrics( ShadowNodeTraits LayoutableShadowNode::BaseTraits() { auto traits = ShadowNodeTraits{}; - traits.set(ShadowNodeTraits::Trait::LayoutableKind); + traits.set(IdentifierTrait()); return traits; } +ShadowNodeTraits::Trait LayoutableShadowNode::IdentifierTrait() { + return ShadowNodeTraits::Trait::LayoutableKind; +} + LayoutMetrics LayoutableShadowNode::getLayoutMetrics() const { return layoutMetrics_; } diff --git a/ReactCommon/react/renderer/core/LayoutableShadowNode.h b/ReactCommon/react/renderer/core/LayoutableShadowNode.h index e5b0fff4747..56b59c55644 100644 --- a/ReactCommon/react/renderer/core/LayoutableShadowNode.h +++ b/ReactCommon/react/renderer/core/LayoutableShadowNode.h @@ -42,6 +42,7 @@ class LayoutableShadowNode : public ShadowNode { ShadowNodeFragment const &fragment); static ShadowNodeTraits BaseTraits(); + static ShadowNodeTraits::Trait IdentifierTrait(); struct LayoutInspectingPolicy { bool includeTransform{true}; @@ -164,29 +165,5 @@ class LayoutableShadowNode : public ShadowNode { LayoutMetrics layoutMetrics_; }; -template <> -inline LayoutableShadowNode const &traitCast( - ShadowNode const &shadowNode) { - bool castable = - shadowNode.getTraits().check(ShadowNodeTraits::Trait::LayoutableKind); - react_native_assert(castable); - (void)castable; - return static_cast(shadowNode); -} - -template <> -inline LayoutableShadowNode const *traitCast( - ShadowNode const *shadowNode) { - if (!shadowNode) { - return nullptr; - } - bool castable = - shadowNode->getTraits().check(ShadowNodeTraits::Trait::LayoutableKind); - if (!castable) { - return nullptr; - } - return static_cast(shadowNode); -} - } // namespace react } // namespace facebook diff --git a/ReactCommon/react/renderer/core/ShadowNode.h b/ReactCommon/react/renderer/core/ShadowNode.h index a190a0906c6..c3bbe40140b 100644 --- a/ReactCommon/react/renderer/core/ShadowNode.h +++ b/ReactCommon/react/renderer/core/ShadowNode.h @@ -224,21 +224,5 @@ class ShadowNode : public Sealable, public DebugStringConvertible { ShadowNodeTraits traits_; }; -/* - * Template declarations for future specializations in concrete classes. - * `traitCast` checks for a trait that corresponds to the provided type and - * performs `static_cast`. Practically, the behavior is identical to - * `dynamic_cast` with very little runtime overhead. - */ -template -ShadowNodeReferenceT traitCast(ShadowNode const &shadowNode); - -template -ShadowNodePointerT traitCast(ShadowNode const *shadowNode); - -template -std::shared_ptr traitCast( - std::shared_ptr shadowNode); - } // namespace react } // namespace facebook diff --git a/ReactCommon/react/renderer/core/ShadowNodeTraits.h b/ReactCommon/react/renderer/core/ShadowNodeTraits.h index d4f78368cb9..11ca76c88b3 100644 --- a/ReactCommon/react/renderer/core/ShadowNodeTraits.h +++ b/ReactCommon/react/renderer/core/ShadowNodeTraits.h @@ -92,17 +92,8 @@ class ShadowNodeTraits { // Temporary (?) to indicate MapBuffer support on Android AndroidMapBufferPropsSupported = 1 << 15, - // Reserved - ReservedTrait0 = 1 << 16, - ReservedTrait1 = 1 << 17, - ReservedTrait2 = 1 << 18, - ReservedTrait3 = 1 << 19, - ReservedTrait4 = 1 << 20, - ReservedTrait5 = 1 << 21, - ReservedTrait6 = 1 << 22, - - // Unserved - alias these for local usage - UnreservedTrait1 = 1 << 23 + // Inherits 'ArtBaseShadowNode' (previously built into RN) + Art = 1 << 16, }; /* @@ -120,6 +111,10 @@ class ShadowNodeTraits { return ShadowNodeTraits::Trait(traits_ & traits) == traits; } + inline Trait get() const { + return traits_; + } + private: Trait traits_{Trait::None}; }; diff --git a/ReactCommon/react/renderer/core/TraitCast.h b/ReactCommon/react/renderer/core/TraitCast.h new file mode 100644 index 00000000000..d0a84debdfa --- /dev/null +++ b/ReactCommon/react/renderer/core/TraitCast.h @@ -0,0 +1,90 @@ +/* + * Copyright (c) Meta Platforms, Inc. and affiliates. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + */ + +#pragma once + +#include +#include +#include + +namespace facebook::react::details { +template +ShadowNodePointerT traitCastPointer(ParamT shadowNode) { + auto expectedIdentifier = + std::remove_pointer_t::IdentifierTrait(); + if (!shadowNode || !shadowNode->getTraits().check(expectedIdentifier)) { + return nullptr; + } + + return static_cast(shadowNode); +} + +template +ShadowNodeRefT traitCastRef(ParamT &&shadowNode) { + auto expectedIdentifier = + std::remove_reference_t::IdentifierTrait(); + if (!shadowNode.getTraits().check(expectedIdentifier)) { + LOG(FATAL) << "Invalid ShadowNode cast\n" + << "Expected identifier: " << std::hex + << static_cast(expectedIdentifier) << "\n" + << "Actual traits: " << std::hex + << static_cast(shadowNode.getTraits().get()) << "\n"; + } + + return static_cast(shadowNode); +} + +template +std::shared_ptr traitCastShared( + const std::shared_ptr &shadowNode) { + auto expectedIdentifier = ShadowNodeT::IdentifierTrait(); + if (!shadowNode || !shadowNode->getTraits().check(expectedIdentifier)) { + return nullptr; + } + + return std::static_pointer_cast(shadowNode); +} +} // namespace facebook::react::details + +namespace facebook::react { + +// Cast from one ShadowNode reference to another, terminating if the cast is +// invalid. +template +ShadowNodeReferenceT traitCast(ShadowNode const &shadowNode) { + return details::traitCastRef(shadowNode); +} +template +ShadowNodeReferenceT traitCast(ShadowNode &shadowNode) { + return details::traitCastRef(shadowNode); +} + +// Cast from one ShadowNode pointer to another, returning nullptr if the cast is +// invalid. +template +ShadowNodePointerT traitCast(ShadowNode const *shadowNode) { + return details::traitCastPointer(shadowNode); +} +template +ShadowNodePointerT traitCast(ShadowNode *shadowNode) { + return details::traitCastPointer(shadowNode); +} + +// Cast from one ShadowNode shared_ptr to another, returning nullptr if the +// cast is invalid. +template +std::shared_ptr traitCast( + const std::shared_ptr &shadowNode) { + return details::traitCastShared(shadowNode); +} +template +std::shared_ptr traitCast( + const std::shared_ptr &shadowNode) { + return details::traitCastShared(shadowNode); +} + +} // namespace facebook::react diff --git a/ReactCommon/react/renderer/core/tests/traitCastTest.cpp b/ReactCommon/react/renderer/core/tests/traitCastTest.cpp index 1154257792a..0f94fd1bf5b 100644 --- a/ReactCommon/react/renderer/core/tests/traitCastTest.cpp +++ b/ReactCommon/react/renderer/core/tests/traitCastTest.cpp @@ -12,6 +12,7 @@ #include #include #include +#include #include #include @@ -51,8 +52,13 @@ TEST(traitCastTest, testOne) { std::shared_ptr shadowNodeForTextShadowNode{textShadowNode}; // Casting `nullptr` returns `nullptrs`. - EXPECT_FALSE(traitCast(nullptr)); - EXPECT_FALSE(traitCast(nullptr)); + ShadowNode *nullShadowNode = nullptr; + EXPECT_FALSE(traitCast(nullShadowNode)); + EXPECT_FALSE(traitCast(nullShadowNode)); + EXPECT_FALSE(traitCast(nullShadowNode)); + EXPECT_FALSE(traitCast(nullShadowNode)); + EXPECT_FALSE(traitCast( + std::shared_ptr(nullShadowNode))); // `ViewShadowNode` is `LayoutableShadowNode` and `YogaLayoutableShadowNode`. EXPECT_TRUE(traitCast(viewShadowNode.get())); @@ -62,6 +68,10 @@ TEST(traitCastTest, testOne) { traitCast(*viewShadowNode)); EXPECT_NO_FATAL_FAILURE( traitCast(*viewShadowNode)); + EXPECT_NO_FATAL_FAILURE( + traitCast(*viewShadowNode)); + EXPECT_TRUE(traitCast(viewShadowNode.get())); + EXPECT_TRUE(traitCast(viewShadowNode)); // `ScrollViewShadowNode` is `LayoutableShadowNode` and // `YogaLayoutableShadowNode`. diff --git a/ReactCommon/react/renderer/mounting/ShadowView.cpp b/ReactCommon/react/renderer/mounting/ShadowView.cpp index c7769bd5d90..68a48d8e429 100644 --- a/ReactCommon/react/renderer/mounting/ShadowView.cpp +++ b/ReactCommon/react/renderer/mounting/ShadowView.cpp @@ -9,6 +9,7 @@ #include #include +#include namespace facebook::react { diff --git a/ReactCommon/react/renderer/uimanager/UIManager.cpp b/ReactCommon/react/renderer/uimanager/UIManager.cpp index 78d7bc912d5..67de51a9db6 100644 --- a/ReactCommon/react/renderer/uimanager/UIManager.cpp +++ b/ReactCommon/react/renderer/uimanager/UIManager.cpp @@ -11,6 +11,7 @@ #include #include #include +#include #include #include #include diff --git a/ReactCommon/react/renderer/uimanager/UIManagerBinding.cpp b/ReactCommon/react/renderer/uimanager/UIManagerBinding.cpp index 730668dcce6..27092d35f7c 100644 --- a/ReactCommon/react/renderer/uimanager/UIManagerBinding.cpp +++ b/ReactCommon/react/renderer/uimanager/UIManagerBinding.cpp @@ -11,6 +11,7 @@ #include #include #include +#include #include #include #include