diff --git a/include/xrpl/shamap/SHAMap.h b/include/xrpl/shamap/SHAMap.h index 42d37cce93..4652cfdba6 100644 --- a/include/xrpl/shamap/SHAMap.h +++ b/include/xrpl/shamap/SHAMap.h @@ -444,20 +444,46 @@ private: return stack_.size(); } + /** + * The node at the end of the path, paired with its ID. + * + * Reading an empty stack would be undefined, and the assert alone is stripped in release, + * so an empty path yields a null node the caller can test instead. + */ [[nodiscard]] std::pair const& top() const { - XRPL_ASSERT(!stack_.empty(), "xrpl::SHAMap::NodePathStack::top : non-empty stack"); + if (stack_.empty()) + { + // LCOV_EXCL_START + UNREACHABLE("xrpl::SHAMap::NodePathStack::top : empty stack"); + static std::pair const kEmpty; + return kEmpty; + // LCOV_EXCL_STOP + } return stack_.top(); } void pop() { - XRPL_ASSERT(!stack_.empty(), "xrpl::SHAMap::NodePathStack::pop : non-empty stack"); + if (stack_.empty()) + { + // LCOV_EXCL_START + UNREACHABLE("xrpl::SHAMap::NodePathStack::pop : empty stack"); + return; + // LCOV_EXCL_STOP + } stack_.pop(); } + /** + * Discard the whole path. + * + * For a walk that pushed a node it then found unusable: the node never became a + * meaningful path entry, so it must not be mistaken for one by whatever the caller + * does next with an empty-vs-nonempty check. + */ void clear() { @@ -466,12 +492,22 @@ private: /** * Start a path at the root of the map, whose ID is the zero-depth ID by definition. + * + * @return false, leaving the path unchanged, if a path was already started. A malformed + * call must not abort a release build, so callers stop rather than overwrite it. */ - void + [[nodiscard]] bool pushRoot(SHAMapTreeNodePtr node) { - XRPL_ASSERT(stack_.empty(), "xrpl::SHAMap::NodePathStack::pushRoot : empty stack"); + if (!stack_.empty()) + { + // LCOV_EXCL_START + UNREACHABLE("xrpl::SHAMap::NodePathStack::pushRoot : non-empty stack"); + return false; + // LCOV_EXCL_STOP + } stack_.emplace(std::move(node), SHAMapNodeID{}); + return true; } /** @@ -479,23 +515,40 @@ private: * * A node keeps the depth it was reached at, never a normalized kLeafDepth. Only a leaf may * sit at kLeafDepth, since an inner node there would have no branch left to select. + * + * @return false, leaving the path unchanged, if the current node can have no child. A + * malformed map must not abort a release build, so callers stop walking instead. */ - void + [[nodiscard]] bool pushChild(SHAMapTreeNodePtr node, unsigned int branch) { - XRPL_ASSERT(node, "xrpl::SHAMap::NodePathStack::pushChild : non-null node input"); - XRPL_ASSERT( - !stack_.empty(), "xrpl::SHAMap::NodePathStack::pushChild : non-empty stack"); - auto childID = stack_.top().second.getChildNodeID(branch); - XRPL_ASSERT_IF( - node->isInner(), - childID.getDepth() < kLeafDepth, - "xrpl::SHAMap::NodePathStack::pushChild : inner node above leaf depth"); + if (stack_.empty() || !node || branch >= kBranchFactor) + { + // LCOV_EXCL_START + UNREACHABLE("xrpl::SHAMap::NodePathStack::pushChild : no child to push"); + return false; + // LCOV_EXCL_STOP + } + + // Only a leaf may sit at kLeafDepth, so an inner child must land one level short of + // it, tighter than the plain depth bound a leaf child needs. + auto const& parentID = stack_.top().second; + auto const parentDepth = parentID.getDepth(); + if (node->isInner() ? parentDepth + 1u >= kLeafDepth : parentDepth >= kLeafDepth) + { + // LCOV_EXCL_START + UNREACHABLE("xrpl::SHAMap::NodePathStack::pushChild : no child to push"); + return false; + // LCOV_EXCL_STOP + } + + auto childID = parentID.getChildNodeID(branch); XRPL_ASSERT_IF( node->isLeaf(), childID.isPrefixOf(leafKey(*node)), "xrpl::SHAMap::NodePathStack::pushChild : leaf key below branch"); stack_.emplace(std::move(node), std::move(childID)); + return true; } /** @@ -504,13 +557,12 @@ private: * For nodes not reached by descending a known branch: the walk tracks only the key it is * heading for, or the node is newly created. Either way `target` selects the branch. */ - void + [[nodiscard]] bool pushNode(SHAMapTreeNodePtr node, uint256 const& target) { if (stack_.empty()) - pushRoot(std::move(node)); - else - pushChild(std::move(node), selectBranch(stack_.top().second, target)); + return pushRoot(std::move(node)); + return pushChild(std::move(node), selectBranch(stack_.top().second, target)); } private: diff --git a/src/libxrpl/shamap/SHAMap.cpp b/src/libxrpl/shamap/SHAMap.cpp index 137f9d32a2..03b6e9b6be 100644 --- a/src/libxrpl/shamap/SHAMap.cpp +++ b/src/libxrpl/shamap/SHAMap.cpp @@ -133,27 +133,42 @@ SHAMap::walkTowardsKey(uint256 const& id, NodePathStack* stack) const auto inNode = root_; SHAMapNodeID nodeID; - // Every node on this walk lies on the path to `id`, so the stack can derive each ID from the - // branch `id` selects at the node above it. - auto pushCurrent = [&] { - if (stack != nullptr) - stack->pushNode(inNode, id); + // Without a caller-supplied stack, `nodeID` is the only record of position, so it is derived + // directly here instead of read back from a push. A failure below means the map is malformed + // (an inner node one level too deep), not that `id` is merely absent; the stack is cleared + // rather than left holding a node that never became a real path entry. + auto pushCurrent = [&]() -> bool { + if (stack == nullptr || stack->pushNode(inNode, id)) + return true; + stack->clear(); + return false; }; while (inNode->isInner()) { - pushCurrent(); + if (!pushCurrent()) + return nullptr; auto& inner = safeDowncast(*inNode); - auto const branch = selectBranch(nodeID, id); + auto const branch = selectBranch(stack != nullptr ? stack->top().second : nodeID, id); if (inner.isEmptyBranch(branch)) return nullptr; inNode = descendThrow(inner, branch); - nodeID = nodeID.getChildNodeID(branch); + if (stack == nullptr) + { + // Only a leaf may sit at kLeafDepth, so an inner child needs the tighter bound: this + // must mirror pushChild's guard exactly, or a malformed map fails one mode earlier + // than the other and stack/no-stack callers disagree on the outcome. + auto const depth = nodeID.getDepth(); + if (inNode->isInner() ? depth + 1u >= kLeafDepth : depth >= kLeafDepth) + return nullptr; + nodeID = nodeID.getChildNodeID(branch); + } } - pushCurrent(); + if (!pushCurrent()) + return nullptr; return safeDowncast(inNode.get()); } @@ -436,6 +451,8 @@ SHAMapLeafNode* SHAMap::belowHelper(NodePathStack& stack, BelowDirection direction) const { XRPL_ASSERT(!stack.empty(), "xrpl::SHAMap::belowHelper : non-empty stack input"); + if (stack.empty()) + return nullptr; if (auto const& top = stack.top().first; top->isLeaf()) return safeDowncast(top.get()); @@ -455,7 +472,9 @@ SHAMap::belowHelper(NodePathStack& stack, BelowDirection direction) const continue; } - stack.pushChild(descendThrow(*inner, childBranch), childBranch); + auto descended = descendThrow(*inner, childBranch); + if (!stack.pushChild(std::move(descended), childBranch)) + return nullptr; auto const& child = stack.top().first; if (child->isLeaf()) @@ -512,10 +531,12 @@ SHAMapLeafNode const* SHAMap::peekFirstItem(NodePathStack& stack) const { XRPL_ASSERT(stack.empty(), "xrpl::SHAMap::peekFirstItem : empty stack input"); - stack.pushRoot(root_); + if (!stack.pushRoot(root_)) + return nullptr; SHAMapLeafNode const* node = belowHelper(stack, BelowDirection::First); if (node == nullptr) { + // An empty map leaves only the root behind; a failed walk leaves the path it got to. stack.clear(); return nullptr; } @@ -526,6 +547,8 @@ SHAMapLeafNode const* SHAMap::peekNextItem(uint256 const& id, NodePathStack& stack) const { XRPL_ASSERT(!stack.empty(), "xrpl::SHAMap::peekNextItem : non-empty stack input"); + if (stack.empty()) + return nullptr; XRPL_ASSERT(stack.top().first->isLeaf(), "xrpl::SHAMap::peekNextItem : stack starts with leaf"); stack.pop(); while (!stack.empty()) @@ -537,7 +560,9 @@ SHAMap::peekNextItem(uint256 const& id, NodePathStack& stack) const { if (!inner.isEmptyBranch(i)) { - stack.pushChild(descendThrow(inner, i), i); + auto child = descendThrow(inner, i); + if (!stack.pushChild(std::move(child), i)) + Throw(type_, id); auto leaf = belowHelper(stack, BelowDirection::First); if (leaf == nullptr) Throw(type_, id); @@ -606,7 +631,9 @@ SHAMap::boundHelper(uint256 const& id, BelowDirection direction) const if (inner.isEmptyBranch(branch)) continue; - stack.pushChild(descendThrow(inner, branch), branch); + auto child = descendThrow(inner, branch); + if (!stack.pushChild(std::move(child), branch)) + Throw(type_, id); auto const leaf = belowHelper(stack, direction); if (leaf == nullptr) Throw(type_, id); @@ -768,7 +795,8 @@ SHAMap::addGiveItem(SHAMapNodeType type, boost::intrusive_ptr while ((b1 = selectBranch(nodeID, tag)) == (b2 = selectBranch(nodeID, otherItem->key()))) { - stack.pushNode(node, tag); + if (!stack.pushNode(node, tag)) + Throw(type_, tag); // we need a new inner node, since both go on same branch at this // level