diff --git a/packages/react-native/ReactCommon/react/renderer/core/RawPropsParser.cpp b/packages/react-native/ReactCommon/react/renderer/core/RawPropsParser.cpp index bef5aa2839b0..23ee075e2267 100644 --- a/packages/react-native/ReactCommon/react/renderer/core/RawPropsParser.cpp +++ b/packages/react-native/ReactCommon/react/renderer/core/RawPropsParser.cpp @@ -56,43 +56,32 @@ const RawValue* RawPropsParser::at( return nullptr; } -// Normally, keys are looked up in-order. For performance we can simply -// increment this key counter, and if the key is equal to the key at the next -// index, there's no need to do any lookups. However, it's possible for keys -// to be accessed out-of-order or multiple times, in which case we start -// searching again from index 0. -// To prevent infinite loops (which can occur if you look up a key that -// doesn't exist) we keep track of whether or not we've already looped around, -// and log and return nullptr if so. However, we ONLY do this in debug mode, -// where you're more likely to look up a nonexistent key as part of debugging. -// You can (and must) ensure infinite loops are not possible in production by: -// (1) constructing all props objects without conditionals, or (2) if there -// are conditionals, ensure that in the parsing setup case, the Props -// constructor will access _all_ possible props. To ensure this performance -// optimization is utilized, always access props in the same order every time. -// This is trivial if you have a simple Props constructor, but difficult or -// impossible if you have a shared sub-prop Struct that is used by multiple -// parent Props. -#ifdef REACT_NATIVE_DEBUG - bool resetLoop = false; -#endif - do { - rawProps.keyIndexCursor_++; + // Normally, keys are looked up in-order. For performance we can simply + // increment this key counter, and if the key is equal to the key at the next + // index, there's no need to do any lookups. However, it's possible for keys + // to be accessed out-of-order or multiple times, in which case we find the + // key's index in `nameToIndex_` and continue from there. + rawProps.keyIndexCursor_++; + + if (static_cast(rawProps.keyIndexCursor_) >= keys_.size()) + [[unlikely]] { + rawProps.keyIndexCursor_ = 0; + } + + if (!keysEqual(key, keys_[rawProps.keyIndexCursor_])) { + auto keyIndex = nameToIndex_.at(key); - if (static_cast(rawProps.keyIndexCursor_) >= keys_.size()) - [[unlikely]] { + if (keyIndex == kRawPropsValueIndexEmpty) [[unlikely]] { #ifdef REACT_NATIVE_DEBUG - if (resetLoop) { - LOG(ERROR) - << "Looked up property name which was not seen when preparing: " - << key; - return nullptr; - } - resetLoop = true; + LOG(ERROR) + << "Looked up property name which was not seen when preparing: " + << key; #endif - rawProps.keyIndexCursor_ = 0; + return nullptr; } - } while (!keysEqual(key, keys_[rawProps.keyIndexCursor_])); + + rawProps.keyIndexCursor_ = keyIndex; + } auto valueIndex = rawProps.keyIndexToValueIndex_[rawProps.keyIndexCursor_]; return valueIndex == kRawPropsValueIndexEmpty ? nullptr diff --git a/packages/react-native/ReactCommon/react/renderer/core/tests/RawPropsTest.cpp b/packages/react-native/ReactCommon/react/renderer/core/tests/RawPropsTest.cpp index b23760236f21..46774b3d1401 100644 --- a/packages/react-native/ReactCommon/react/renderer/core/tests/RawPropsTest.cpp +++ b/packages/react-native/ReactCommon/react/renderer/core/tests/RawPropsTest.cpp @@ -350,7 +350,6 @@ TEST(RawPropsTest, handleRawPropsPrimitiveTypesIncomplete) { EXPECT_EQ((int)*raw.at("intValue"), 42); } -#ifdef REACT_NATIVE_DEBUG TEST(RawPropsTest, handleRawPropsPrimitiveTypesIncorrectLookup) { ContextContainer contextContainer{}; PropsParserContext parserContext{-1, contextContainer}; @@ -361,13 +360,9 @@ TEST(RawPropsTest, handleRawPropsPrimitiveTypesIncorrectLookup) { parser.prepare(); raw.parse(parser); - // Before D18662135, looking up an invalid key would trigger - // an infinite loop. This is out of contract, so we should only - // test this in debug. EXPECT_EQ(raw.at("flurb"), nullptr); EXPECT_EQ((int)*raw.at("intValue"), 42); } -#endif TEST(RawPropsTest, handlePropsMultiLookup) { ContextContainer contextContainer{};