diff --git a/include/xrpl/basics/IntrusivePointer.h b/include/xrpl/basics/IntrusivePointer.h index b978016860..9f1617cc96 100644 --- a/include/xrpl/basics/IntrusivePointer.h +++ b/include/xrpl/basics/IntrusivePointer.h @@ -86,12 +86,16 @@ public: requires std::convertible_to SharedIntrusive(SharedIntrusive const& rhs); - SharedIntrusive(SharedIntrusive&& rhs); + // noexcept so that a std::vector of these relocates by moving. Without it, move_if_noexcept + // copies each element instead, since this type is also copy constructible, and every copy is an + // atomic increment on the pointee's refcount followed by a release on the original. The body is + // a std::exchange on a raw pointer, so it provably cannot throw. + SharedIntrusive(SharedIntrusive&& rhs) noexcept; template requires std::convertible_to - SharedIntrusive( - SharedIntrusive&& rhs); // NOLINT(cppcoreguidelines-rvalue-reference-param-not-moved) + // NOLINTNEXTLINE(cppcoreguidelines-rvalue-reference-param-not-moved) + SharedIntrusive(SharedIntrusive&& rhs) noexcept; SharedIntrusive& operator=(SharedIntrusive const& rhs); @@ -529,11 +533,48 @@ staticPointerCast(TT const& v) return SharedPtr(StaticCastTagSharedIntrusive{}, v); } +/** + * Statically cast an intrusive pointer the caller is giving up, moving out of + * it. + * + * The parameter names the wrapped type rather than taking a bare `TT&&`. A + * bare one would be a forwarding reference, so it would also bind to lvalues + * in preference to the `const&` overload above and move out of a caller's live + * variable on what looks like a copy call. + * + * @param v the pointer to cast, left empty afterwards. + * @return a pointer of the requested type to the same object. + */ +template +SharedPtr +staticPointerCast(SharedIntrusive&& v) +{ + return SharedPtr(StaticCastTagSharedIntrusive{}, std::move(v)); +} + template SharedPtr dynamicPointerCast(TT const& v) { return SharedPtr(DynamicCastTagSharedIntrusive{}, v); } + +/** + * Dynamically cast an intrusive pointer the caller is giving up, moving out of + * it. + * + * Tied to `SharedIntrusive&&` for the reason given above. + * + * @param v the pointer to cast, left empty afterwards if the cast succeeds and + * left owning the object if it does not. + * @return a pointer of the requested type, or an empty one if the object is + * not of that type. + */ +template +SharedPtr +dynamicPointerCast(SharedIntrusive&& v) +{ + return SharedPtr(DynamicCastTagSharedIntrusive{}, std::move(v)); +} } // namespace intr_ptr } // namespace xrpl diff --git a/include/xrpl/basics/IntrusivePointer.ipp b/include/xrpl/basics/IntrusivePointer.ipp index 6c2a71f7eb..6bf6af334d 100644 --- a/include/xrpl/basics/IntrusivePointer.ipp +++ b/include/xrpl/basics/IntrusivePointer.ipp @@ -43,7 +43,7 @@ SharedIntrusive::SharedIntrusive(SharedIntrusive const& rhs) } template -SharedIntrusive::SharedIntrusive(SharedIntrusive&& rhs) +SharedIntrusive::SharedIntrusive(SharedIntrusive&& rhs) noexcept : ptr_{std::move(rhs).unsafeExchange(nullptr)} { } @@ -51,7 +51,7 @@ SharedIntrusive::SharedIntrusive(SharedIntrusive&& rhs) template template requires std::convertible_to -SharedIntrusive::SharedIntrusive(SharedIntrusive&& rhs) +SharedIntrusive::SharedIntrusive(SharedIntrusive&& rhs) noexcept : ptr_{std::move(rhs).unsafeExchange(nullptr)} { } diff --git a/include/xrpl/shamap/SHAMap.h b/include/xrpl/shamap/SHAMap.h index b1ed1177d8..643602f8c9 100644 --- a/include/xrpl/shamap/SHAMap.h +++ b/include/xrpl/shamap/SHAMap.h @@ -519,6 +519,33 @@ private: stack_ = {}; } + /** + * Shorten the path by one node and hand that node to the caller, + * keeping its ID. + * + * Reading a node out and then popping copies it, which costs an atomic + * increment on its refcount. Moving it out does not. A caller that + * wants the ID as well reads `top().second` first, which costs the + * same either way: `SHAMapNodeID` declares no move constructor. + * + * @return the node that was at the end of the path, or an empty + * pointer if there was none. + */ + [[nodiscard]] SHAMapTreeNodePtr + releaseNode() + { + if (stack_.empty()) + { + // LCOV_EXCL_START + UNREACHABLE("xrpl::SHAMap::NodePathStack::releaseNode : empty stack"); + return {}; + // LCOV_EXCL_STOP + } + auto node = std::move(stack_.top().first); + stack_.pop(); + return node; + } + /** * Start a path at the root of the map, whose ID is the zero-depth ID by definition. * @@ -655,9 +682,15 @@ private: dirtyUp(NodePathStack& stack, uint256 const& target, SHAMapTreeNodePtr terminal); /** - * 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 + * Walk towards the specified id, returning the node. + * + * @param id the key to walk towards, which need not be in the map. + * @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. + * @return the leaf the walk ended on, or nullptr if it ended on an inner + * node or was refused. A returned leaf need not hold `id`, so + * callers compare its key themselves. */ SHAMapLeafNode* walkTowardsKey(uint256 const& id, NodePathStack* stack = nullptr) const; diff --git a/src/libxrpl/shamap/SHAMap.cpp b/src/libxrpl/shamap/SHAMap.cpp index 1307edaac9..f1c00af5cc 100644 --- a/src/libxrpl/shamap/SHAMap.cpp +++ b/src/libxrpl/shamap/SHAMap.cpp @@ -111,10 +111,16 @@ 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 const nodeID = stack.top().second; + auto top = stack.releaseNode(); + 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); @@ -621,7 +627,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) @@ -690,7 +696,7 @@ SHAMap::boundHelper(uint256 const& id, BelowDirection direction) const while (!stack.empty()) { - auto const [node, nodeID] = stack.top(); + auto const& [node, nodeID] = stack.top(); if (node->isLeaf()) { auto const& item = safeDowncast(*node).peekItem(); @@ -756,10 +762,15 @@ SHAMap::delItem(uint256 const& id) if (stack.empty()) Throw(type_, id); - auto leaf = intr_ptr::dynamicPointerCast(stack.top().first); - stack.pop(); + // An absent id leaves an inner node on top rather than a leaf, which is a "not found" answer + // and not a fault, so it is tested rather than cast through. Matches the three sibling sites in + // this commit, so no traversal path is left paying for a dynamic_cast. + auto top = stack.releaseNode(); + if (!top->isLeaf()) + return false; + auto leaf = intr_ptr::staticPointerCast(std::move(top)); - if (!leaf || (leaf->peekItem()->key() != id)) + if (leaf->peekItem()->key() != id) return false; SHAMapNodeType const type = leaf->getType(); @@ -769,9 +780,16 @@ 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 const nodeID = stack.top().second; + auto top = stack.releaseNode(); + 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( @@ -842,8 +860,8 @@ SHAMap::addGiveItem(SHAMapNodeType type, boost::intrusive_ptr if (stack.empty()) Throw(type_, tag); - auto [node, nodeID] = stack.top(); - stack.pop(); + auto nodeID = stack.top().second; + auto node = stack.releaseNode(); if (node->isLeaf()) { @@ -938,16 +956,26 @@ 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 const nodeID = stack.top().second; + auto top = stack.releaseNode(); - 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. + // No in-tree caller reaches this, since each checks the item exists first, but the API is + // public and permits the call, which is why it returns false rather than reporting UNREACHABLE. + // The static cast below is also safe only once this is confirmed. + if (!top->isLeaf()) + { + return false; + } + auto node = intr_ptr::staticPointerCast(std::move(top)); + + // The other shape an absent tag takes: the walk ends on the leaf it reached, whose key need not + // be `tag`, which is why findKey discards such a leaf. Both shapes mean the same thing to a + // caller, so both answer false rather than reporting UNREACHABLE. + if (node->peekItem()->key() != tag) { - // LCOV_EXCL_START - UNREACHABLE("xrpl::SHAMap::updateGiveItem : invalid node"); return false; - // LCOV_EXCL_STOP } if (node->getType() != type) diff --git a/src/tests/libxrpl/shamap/SHAMap.cpp b/src/tests/libxrpl/shamap/SHAMap.cpp index 1f33902f0a..b57cd0d093 100644 --- a/src/tests/libxrpl/shamap/SHAMap.cpp +++ b/src/tests/libxrpl/shamap/SHAMap.cpp @@ -459,6 +459,39 @@ 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, so + // updateGiveItem has to answer for a top of the wrong type. The public API permits the call, + // so it returns false rather than reporting UNREACHABLE and aborting an instrumented build. + 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)))); + + // The other shape an absent key takes: a single-item map queried with a key that selects the + // same root branch. The walk ends on the leaf it reached, whose key is not the one asked for, + // so the top is a leaf that does not hold the tag. Both shapes answer false. The difference + // only shows in an instrumented build, where reporting UNREACHABLE would abort. + SHAMap single{SHAMapType::FREE, f}; + fillMap(single, {keys.front()}); + + auto probe = keys.front(); + std::fill_n(probe.begin() + 1, probe.size() - 1, std::uint8_t{0}); + ASSERT_NE(probe, keys.front()); + + Buffer other{32}; + std::fill_n(other.data(), other.size(), std::uint8_t{3}); + EXPECT_FALSE(single.updateGiveItem( + SHAMapNodeType::TnAccountState, makeShamapitem(probe, std::move(other)))); +} + TEST_F(SHAMapTraversal, bounds_on_empty_map_return_end) { tests::TestNodeFamily f{j_};