diff --git a/include/xrpl/basics/IntrusivePointer.h b/include/xrpl/basics/IntrusivePointer.h index b978016860..2f9f1084a6 100644 --- a/include/xrpl/basics/IntrusivePointer.h +++ b/include/xrpl/basics/IntrusivePointer.h @@ -529,11 +529,30 @@ staticPointerCast(TT const& v) return SharedPtr(StaticCastTagSharedIntrusive{}, v); } +// A bare `TT&&` here would be a forwarding reference, since TT is deduced directly from this +// parameter, and would then also bind to lvalues in preference to the `const&` overload above +// (binding to a plain reference beats binding to a const one), silently moving out of a caller's +// live variable on what looks like a copy call. Naming the wrapped type keeps TT nested inside +// SharedIntrusive, so this only binds to an actual rvalue of that type. +template +SharedPtr +staticPointerCast(SharedIntrusive&& v) +{ + return SharedPtr(StaticCastTagSharedIntrusive{}, std::move(v)); +} + template SharedPtr dynamicPointerCast(TT const& v) { return SharedPtr(DynamicCastTagSharedIntrusive{}, v); } + +template +SharedPtr +dynamicPointerCast(SharedIntrusive&& v) +{ + return SharedPtr(DynamicCastTagSharedIntrusive{}, std::move(v)); +} } // namespace intr_ptr } // namespace xrpl diff --git a/include/xrpl/shamap/SHAMap.h b/include/xrpl/shamap/SHAMap.h index 4652cfdba6..226626f363 100644 --- a/include/xrpl/shamap/SHAMap.h +++ b/include/xrpl/shamap/SHAMap.h @@ -490,6 +490,28 @@ private: stack_ = {}; } + /** + * Remove and return the node at the end of the path. + * + * Copying an entry out before popping costs an atomic increment on the node's refcount, + * and callers that pop immediately do this per level, so on a 64-level path it is + * measurable. Moving avoids that increment. + */ + [[nodiscard]] std::pair + release() + { + if (stack_.empty()) + { + // LCOV_EXCL_START + UNREACHABLE("xrpl::SHAMap::NodePathStack::release : empty stack"); + return {}; + // LCOV_EXCL_STOP + } + auto entry = std::move(stack_.top()); + stack_.pop(); + return entry; + } + /** * Start a path at the root of the map, whose ID is the zero-depth ID by definition. * @@ -601,6 +623,10 @@ private: * Walk towards the specified id, returning the node. Caller must check * if the return is nullptr, and if not, if the node->peekItem()->key() == * id + * + * @param stack records the path walked, or nullptr to skip recording it. Lookups that only + * want the leaf (see findKey) omit it to avoid building a path they would + * immediately discard. */ SHAMapLeafNode* walkTowardsKey(uint256 const& id, NodePathStack* stack = nullptr) const; diff --git a/src/libxrpl/shamap/SHAMap.cpp b/src/libxrpl/shamap/SHAMap.cpp index 03b6e9b6be..1118588f5b 100644 --- a/src/libxrpl/shamap/SHAMap.cpp +++ b/src/libxrpl/shamap/SHAMap.cpp @@ -111,10 +111,15 @@ SHAMap::dirtyUp(NodePathStack& stack, uint256 const& target, SHAMapTreeNodePtr c while (!stack.empty()) { - auto node = intr_ptr::dynamicPointerCast(stack.top().first); - SHAMapNodeID const nodeID = stack.top().second; - stack.pop(); - XRPL_ASSERT(node, "xrpl::SHAMap::dirtyUp : non-null node"); + auto [top, nodeID] = stack.release(); + if (!top->isInner()) + { + // LCOV_EXCL_START + UNREACHABLE("xrpl::SHAMap::dirtyUp : node is not inner"); + Throw(type_, target); + // LCOV_EXCL_STOP + } + auto node = intr_ptr::staticPointerCast(std::move(top)); auto const branch = selectBranch(nodeID, target); @@ -553,7 +558,7 @@ SHAMap::peekNextItem(uint256 const& id, NodePathStack& stack) const stack.pop(); while (!stack.empty()) { - auto const [node, nodeID] = stack.top(); + auto const& [node, nodeID] = stack.top(); XRPL_ASSERT(!node->isLeaf(), "xrpl::SHAMap::peekNextItem : another node is not leaf"); auto& inner = safeDowncast(*node); for (auto i = selectBranch(nodeID, id) + 1; i < kBranchFactor; ++i) @@ -611,7 +616,7 @@ SHAMap::boundHelper(uint256 const& id, BelowDirection direction) const walkTowardsKey(id, &stack); while (!stack.empty()) { - auto const [node, nodeID] = stack.top(); + auto const& [node, nodeID] = stack.top(); if (node->isLeaf()) { auto const& item = safeDowncast(*node).peekItem(); @@ -675,8 +680,7 @@ SHAMap::delItem(uint256 const& id) if (stack.empty()) Throw(type_, id); - auto leaf = intr_ptr::dynamicPointerCast(stack.top().first); - stack.pop(); + auto leaf = intr_ptr::dynamicPointerCast(stack.release().first); if (!leaf || (leaf->peekItem()->key() != id)) return false; @@ -688,9 +692,15 @@ SHAMap::delItem(uint256 const& id) while (!stack.empty()) { - auto node = intr_ptr::staticPointerCast(stack.top().first); - SHAMapNodeID const nodeID = stack.top().second; - stack.pop(); + auto [top, nodeID] = stack.release(); + if (!top->isInner()) + { + // LCOV_EXCL_START + UNREACHABLE("xrpl::SHAMap::delItem : node is not inner"); + Throw(type_, id); + // LCOV_EXCL_STOP + } + auto node = intr_ptr::staticPointerCast(std::move(top)); node = unshareNode(std::move(node), nodeID); node->setChild( @@ -761,8 +771,7 @@ SHAMap::addGiveItem(SHAMapNodeType type, boost::intrusive_ptr if (stack.empty()) Throw(type_, tag); - auto [node, nodeID] = stack.top(); - stack.pop(); + auto [node, nodeID] = stack.release(); if (node->isLeaf()) { @@ -849,11 +858,17 @@ SHAMap::updateGiveItem(SHAMapNodeType type, boost::intrusive_ptr(type_, tag); - auto node = intr_ptr::dynamicPointerCast(stack.top().first); - auto nodeID = stack.top().second; - stack.pop(); + auto [top, nodeID] = stack.release(); - if (!node || (node->peekItem()->key() != tag)) + // walkTowardsKey pushes an inner node's own entry before testing whether the branch it needs + // is empty, so a tag absent from the map leaves that inner node on top rather than a leaf. + // Not reachable through updateGiveItem's current callers, all of which check the item exists + // before calling this, but the static cast below is safe only once this is confirmed. + if (!top->isLeaf()) + return false; + auto node = intr_ptr::staticPointerCast(std::move(top)); + + if (node->peekItem()->key() != tag) { // LCOV_EXCL_START UNREACHABLE("xrpl::SHAMap::updateGiveItem : invalid node"); diff --git a/src/tests/libxrpl/shamap/SHAMap.cpp b/src/tests/libxrpl/shamap/SHAMap.cpp index 349f7c6fd9..8aef2fa464 100644 --- a/src/tests/libxrpl/shamap/SHAMap.cpp +++ b/src/tests/libxrpl/shamap/SHAMap.cpp @@ -417,6 +417,21 @@ TEST_F(SHAMapTraversal, bounds_agree_with_iteration_for_absent_keys) } } +TEST_F(SHAMapTraversal, update_give_item_on_absent_key_returns_false) +{ + tests::TestNodeFamily f{j_}; + auto keys = deepFanOutKeys(); + SHAMap map{SHAMapType::FREE, f}; + fillMap(map, keys); + + // Absent key: walkTowardsKey stops on an inner node with an empty branch, not a leaf. + auto const absentKey = uint256{std::string_view{std::string(64, '0')}}; + Buffer vuc{32}; + std::fill_n(vuc.data(), vuc.size(), std::uint8_t{2}); + EXPECT_FALSE(map.updateGiveItem( + SHAMapNodeType::TnAccountState, makeShamapitem(absentKey, std::move(vuc)))); +} + TEST_F(SHAMapTraversal, bounds_on_empty_map_return_end) { tests::TestNodeFamily f{j_};