From 19d8ff8ff5b8a3fb296e6f6bd924d8c8f2ca7133 Mon Sep 17 00:00:00 2001 From: Bart <11445373+bthomee@users.noreply.github.com> Date: Mon, 21 Sep 2026 17:26:34 +0200 Subject: [PATCH] fix: Reject a misplaced leaf at entry, fail closed if one slips past `NodePathStack`'s position and depth checks were `XRPL_ASSERT_IF`s, which are stripped under `NDEBUG`, so a release build walked on with a node sitting where it did not belong. Each is now a live test that refuses the push and lets the caller stop, and each reports `SOMETIMES` rather than `UNREACHABLE`: a node resolved from the local store reaches a walk through `descend(parent, branch)`, which fetches by the parent's recorded child hash and judges neither position nor type, so external data can reach either case and neither may abort a build. Every path that does know the position now judges a node before hooking it: the two filter descents and the deferred-read hook, each marking the map invalid the way `addKnownNode` already did. `getMissingNodes` no longer calls `clearSynching()` on a map it has condemned, at either of its two returns, since that would move the state to `Modifying` and erase the verdict. `gmnProcessDeferredReads` became non-static so it can record one. `boundHelper` now throws where it used to answer `end()`. An empty map still leaves its root on the path, so an empty path means only that a node was refused, while `end()` is the positive claim that no key lies on the requested side of the key asked for. New `SHAMapMisplacedLeaf` tests build a tree whose hashes agree but whose leaf sits under the wrong branch, drive it in through both acquisition routes, and check that iteration and both bounds refuse it. --- include/xrpl/shamap/SHAMap.h | 195 ++++++++++++--- include/xrpl/shamap/SHAMapLeafNode.h | 18 ++ src/libxrpl/shamap/SHAMap.cpp | 155 ++++++++++-- src/libxrpl/shamap/SHAMapSync.cpp | 86 ++++++- src/tests/libxrpl/shamap/SHAMap.cpp | 346 ++++++++++++++++++++++++++- 5 files changed, 736 insertions(+), 64 deletions(-) diff --git a/include/xrpl/shamap/SHAMap.h b/include/xrpl/shamap/SHAMap.h index a29f8fb679..b1ed1177d8 100644 --- a/include/xrpl/shamap/SHAMap.h +++ b/include/xrpl/shamap/SHAMap.h @@ -420,6 +420,28 @@ public: invariants() const; private: + /** + * Whether placing `node` one level below `parentDepth` leaves it no room. + * + * Only a leaf may sit at kLeafDepth, since an inner node there would have + * no branch left to select. Both of the places that bound a descent call + * this, so a walk with a caller-supplied path and one without cannot drift + * apart and refuse at different nodes. + * + * The depth is tested before the node's type so the virtual call runs only + * where the bound can bite, which is the last level of a 65-level walk. + * + * @param parentDepth the depth of the node being descended from. + * @param node the node about to be placed one level below it. + * @return whether that placement is past the deepest level this kind of + * node may occupy. + */ + [[nodiscard]] static bool + pastLeafDepth(unsigned int parentDepth, SHAMapTreeNode const& node) + { + return parentDepth + 1u >= kLeafDepth && (node.isInner() || parentDepth >= kLeafDepth); + } + /** * A path from the root of the map down to some node, pairing each node with the ID naming its * position. @@ -444,20 +466,53 @@ 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(); } + /** + * Shorten the path by one node. + * + * Popping an empty path would be undefined, and the assert alone is + * stripped in release, so an empty path is left alone instead. + */ 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 +521,23 @@ 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 +545,65 @@ 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. + * + * @param node the child to append. + * @param branch the branch of the current node that `node` was + * reached through. + * @return false, leaving the path unchanged, if there is no node to + * descend from, no node to push, no branch of that number, no + * room left below for the kind of node offered, or a leaf + * whose own key does not lie under `branch`. A malformed call + * or 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"); - XRPL_ASSERT_IF( - node->isLeaf(), - childID.isPrefixOf(leafKey(*node)), - "xrpl::SHAMap::NodePathStack::pushChild : leaf key below branch"); + 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. + // + // Reachable, for the same reason the misplaced-leaf case below is: a node resolved from + // the local store has had neither its position nor its type judged. The two-argument + // SHAMap::descend fetches by the parent's recorded child hash and hooks what comes + // back, and a parsed node adopts that hash rather than recomputing it, so an inner node + // can arrive one level too deep. So this refuses rather than aborting an instrumented + // build. + // + auto const& parentID = stack_.top().second; + auto const parentDepth = parentID.getDepth(); + bool const tooDeep = pastLeafDepth(parentDepth, *node); + SOMETIMES(tooDeep, "xrpl::SHAMap::NodePathStack::pushChild : child past leaf depth"); + if (tooDeep) + { + return false; + } + + // A leaf's own key names its position, so a leaf reached by this branch must agree with + // the ID that branch derives. Where the two disagree the pair is not a path entry at + // all, and keeping it would make every later walk read the ID rather than the key. + // + // Not UNREACHABLE, for the reason given above: the paths that hook a node from a peer + // reject a misplaced one first (see SHAMap::descend and SHAMap::gmnProcessNodes), but a + // map read lazily from the local store never passes through them. + auto childID = parentID.getChildNodeID(branch); + bool const misplaced = !belongsAt(childID, *node); + SOMETIMES( + misplaced, "xrpl::SHAMap::NodePathStack::pushChild : leaf key outside branch"); + if (misplaced) + { + return false; + } + stack_.emplace(std::move(node), std::move(childID)); + return true; } /** @@ -504,17 +612,14 @@ 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: @@ -588,12 +693,26 @@ private: /** * Returns the first or last item at or below the node already on top of `stack`, extending * `stack` with the path walked to reach it. + * + * @param stack the path to extend, whose last node the search starts from. + * @param direction whether to take the lowest or the highest branch at + * each level. + * @return the leaf found, or nullptr if no leaf lies below that node. */ SHAMapLeafNode* belowHelper(NodePathStack& stack, BelowDirection direction) const; - // helper function for upperBound and lowerBound - ConstIterator + /** + * The nearest item on one side of `id`, which upperBound and lowerBound + * both answer. + * + * @param id the key to search around, which need not be in the map. + * @param direction First for the nearest key greater than `id`, Last for + * the nearest lesser. + * @return an iterator at that item, or end() if the map holds no key on + * that side. + */ + [[nodiscard]] ConstIterator boundHelper(uint256 const& id, BelowDirection direction) const; // Simple descent @@ -718,10 +837,30 @@ private: }; // getMissingNodes helper functions + + /** + * Examine the remaining branches of one inner node, recording or + * requesting what is missing. + * + * @param mn the walk's shared state, which collects the missing nodes. + * @param node the walk's current position, updated to the node to process + * next. + */ void - gmnProcessNodes(MissingNodes&, MissingNodes::StackEntry& node); - static void - gmnProcessDeferredReads(MissingNodes&); + gmnProcessNodes(MissingNodes& mn, MissingNodes::StackEntry& node); + + /** + * Wait for every read this pass posted, then hook up or record what each + * one resolved. + * + * Drains all of them even after judging the map, since an outstanding + * read holds a pointer to `mn` and this is the only thing that waits for + * it. + * + * @param mn the walk's shared state, holding the posted reads. + */ + void + gmnProcessDeferredReads(MissingNodes& mn); // fetch from DB helper function SHAMapTreeNodePtr diff --git a/include/xrpl/shamap/SHAMapLeafNode.h b/include/xrpl/shamap/SHAMapLeafNode.h index ab5bd574ed..e9c7e6557d 100644 --- a/include/xrpl/shamap/SHAMapLeafNode.h +++ b/include/xrpl/shamap/SHAMapLeafNode.h @@ -75,4 +75,22 @@ leafKey(SHAMapTreeNode const& node) return safeDowncast(node).peekItem()->key(); } +/** + * Whether a node may occupy a position in a SHAMap. + * + * A leaf's own key names its position, so an ID that is not a prefix of that + * key names a different subtree than the one the leaf belongs to. An inner + * node carries no key, so every position is consistent with it and the + * caller's own depth rules are what bound it. + * + * @param nodeID the position the node is claimed to occupy. + * @param node the node to judge. + * @return whether the node's own key agrees with that position. + */ +[[nodiscard]] inline bool +belongsAt(SHAMapNodeID const& nodeID, SHAMapTreeNode const& node) +{ + return !node.isLeaf() || nodeID.isPrefixOf(leafKey(node)); +} + } // namespace xrpl diff --git a/src/libxrpl/shamap/SHAMap.cpp b/src/libxrpl/shamap/SHAMap.cpp index 137f9d32a2..1307edaac9 100644 --- a/src/libxrpl/shamap/SHAMap.cpp +++ b/src/libxrpl/shamap/SHAMap.cpp @@ -128,32 +128,69 @@ SHAMap::dirtyUp(NodePathStack& stack, uint256 const& target, SHAMapTreeNodePtr c SHAMapLeafNode* SHAMap::walkTowardsKey(uint256 const& id, NodePathStack* stack) const { - XRPL_ASSERT( - stack == nullptr || stack->empty(), "xrpl::SHAMap::walkTowardsKey : empty stack input"); + if (stack != nullptr && !stack->empty()) + { + // A plain XRPL_ASSERT here is a no-op under NDEBUG; without this guard a non-empty stack + // would be appended to below, leaving the caller with a path that starts mid-walk instead + // of at the root. + // LCOV_EXCL_START + UNREACHABLE("xrpl::SHAMap::walkTowardsKey : non-empty stack input"); + stack->clear(); + return nullptr; + // LCOV_EXCL_STOP + } + 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 push fails when the map is malformed, by + // holding a leaf outside the branch it was reached through or a node with no room left below + // it, not because `id` is merely absent; the stack is cleared rather than left holding a node + // that never became a real path entry. Callers tell the two apart by the path, which is empty + // only in the first case. + 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) + { + // Shares pastLeafDepth with pushChild, so this mode and the one with a + // caller-supplied path refuse at the same node. Reachable for the reason that helper + // gives, so it refuses rather than aborts. + auto const depth = nodeID.getDepth(); + bool const tooDeep = pastLeafDepth(depth, *inNode); + SOMETIMES(tooDeep, "xrpl::SHAMap::walkTowardsKey : child too deep"); + if (tooDeep) + { + return nullptr; + } + nodeID = nodeID.getChildNodeID(branch); + } } - pushCurrent(); + if (!pushCurrent()) + { + return nullptr; + } return safeDowncast(inNode.get()); } @@ -357,12 +394,29 @@ SHAMap::descend( !parent->isEmptyBranch(branch), "xrpl::SHAMap::descend : parent branch is non-empty"); SHAMapTreeNode* child = parent->getChildPointer(branch); // NOLINT(misc-const-correctness) + auto childID = parentID.getChildNodeID(branch); if (child == nullptr) { auto const& childHash = parent->getChildHash(branch); SHAMapTreeNodePtr childNode = fetchNodeNT(childHash, filter); + if (childNode && !belongsAt(childID, *childNode)) + { + // A node arriving through the filter is judged by hash, and a hash covers a node's + // contents rather than its position, so this is where a leaf that belongs elsewhere + // enters the map. Judged before canonicalizeChild, after which every later walk would + // see it as part of the tree. + // + // The map is the verdict rather than the node, because refusing one node would only + // make the walk fetch the same thing again: the filter answers from a local cache, so + // the next attempt resolves the same blob to the same place. + JLOG(journal_.warn()) << "Leaf " << childHash << " does not belong at " << childID + << ", map is invalid"; + state_ = SHAMapState::Invalid; + return std::make_pair(nullptr, std::move(childID)); + } + if (childNode) { childNode = parent->canonicalizeChild(branch, std::move(childNode)); @@ -370,7 +424,7 @@ SHAMap::descend( } } - return std::make_pair(child, parentID.getChildNodeID(branch)); + return std::make_pair(child, std::move(childID)); } SHAMapTreeNode* @@ -436,6 +490,12 @@ SHAMapLeafNode* SHAMap::belowHelper(NodePathStack& stack, BelowDirection direction) const { XRPL_ASSERT(!stack.empty(), "xrpl::SHAMap::belowHelper : non-empty stack input"); + if (stack.empty()) + { + // LCOV_EXCL_START + return nullptr; + // LCOV_EXCL_STOP + } if (auto const& top = stack.top().first; top->isLeaf()) return safeDowncast(top.get()); @@ -455,7 +515,25 @@ 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)) + { + // A refused push means the map holds a node that cannot be walked, which is not the + // same as a subtree with no leaf below it. Throwing keeps nullptr meaning only the + // latter, so begin() cannot report such a map as empty while an iterator increment + // throws on the same condition. SHAMapMissingNode describes a resident node poorly, + // but descendThrow above throws it too, so every caller already handles it. + // + // The map is deliberately NOT condemned here. Every caller of belowHelper is a const + // read on an immutable snapshot, called from several RPC threads at once, and no + // reader checks isValid(); the callers that do are on the acquisition path. So the + // write would buy nothing, would race those readers, and would make a later compare() + // trip its own isValid() assertion. A map from peer data is judged where it is + // assembled (see SHAMap::descend and gmnProcessNodes). + JLOG(journal_.warn()) << "Cannot walk below " << stack.top().second << " at branch " + << childBranch; + Throw(type_, inner->getChildHash(childBranch)); + } auto const& child = stack.top().first; if (child->isLeaf()) @@ -512,10 +590,17 @@ SHAMapLeafNode const* SHAMap::peekFirstItem(NodePathStack& stack) const { XRPL_ASSERT(stack.empty(), "xrpl::SHAMap::peekFirstItem : empty stack input"); - stack.pushRoot(root_); + if (!stack.pushRoot(root_)) + { + // LCOV_EXCL_START + return nullptr; + // LCOV_EXCL_STOP + } SHAMapLeafNode const* node = belowHelper(stack, BelowDirection::First); if (node == nullptr) { + // Whether the map was empty or belowHelper's walk otherwise failed to find a leaf, the + // stack is cleared rather than left holding a partial path the caller cannot use. stack.clear(); return nullptr; } @@ -526,6 +611,12 @@ SHAMapLeafNode const* SHAMap::peekNextItem(uint256 const& id, NodePathStack& stack) const { XRPL_ASSERT(!stack.empty(), "xrpl::SHAMap::peekNextItem : non-empty stack input"); + if (stack.empty()) + { + // LCOV_EXCL_START + return nullptr; + // LCOV_EXCL_STOP + } XRPL_ASSERT(stack.top().first->isLeaf(), "xrpl::SHAMap::peekNextItem : stack starts with leaf"); stack.pop(); while (!stack.empty()) @@ -537,7 +628,11 @@ 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); @@ -578,12 +673,21 @@ SHAMap::ConstIterator SHAMap::boundHelper(uint256 const& id, BelowDirection direction) const { // Walk back up the path to `id` looking for the nearest leaf on the requested side. At each - // inner node the branches beyond the one `id` takes hold the candidates; the first non-empty - // one is the closest, and the extreme leaf below it is the answer. + // inner node the candidates are the branches on that side of the one `id` takes: the higher + // ones searching forward, the lower ones searching back. The nearest non-empty candidate holds + // the answer, which is its lowest leaf searching forward and its highest searching back. auto const searchingForward = direction == BelowDirection::First; NodePathStack stack; walkTowardsKey(id, &stack); + + // An empty path means the walk refused a node, not that the map is empty: an empty map still + // leaves its root on the path. end() is the positive claim that no key lies on the requested + // side of `id`, so it must not stand in for "cannot answer", which is what every other entry + // point reports by throwing. + if (stack.empty()) + Throw(type_, id); + while (!stack.empty()) { auto const [node, nodeID] = stack.top(); @@ -606,7 +710,11 @@ 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 +876,16 @@ 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)) + { + // The node pushed here is freshly made and inner, so only the depth bound could + // refuse it, and the loop cannot reach that bound: it advances only while the two + // keys agree at the current nibble, and keys agreeing at all 64 nibbles are equal, + // which the caller already returned false for. + // LCOV_EXCL_START + Throw(type_, tag); + // LCOV_EXCL_STOP + } // we need a new inner node, since both go on same branch at this // level diff --git a/src/libxrpl/shamap/SHAMapSync.cpp b/src/libxrpl/shamap/SHAMapSync.cpp index 602d8e629c..6f1feb5204 100644 --- a/src/libxrpl/shamap/SHAMapSync.cpp +++ b/src/libxrpl/shamap/SHAMapSync.cpp @@ -238,6 +238,28 @@ SHAMap::gmnProcessNodes(MissingNodes& mn, MissingNodes::StackEntry& se) if (--mn.max <= 0) return; } + // Only a leaf has a position of its own to judge, so the type is tested first: that + // also keeps getChildNodeID, which builds a SHAMapNodeID, off every inner child on the + // walk. The depth is tested next so the ID is only asked for a child that can exist. + else if ( + d->isLeaf() && nodeID.getDepth() < kLeafDepth && + !belongsAt(nodeID.getChildNodeID(branch), *d)) + { + // The same judgment SHAMap::descend makes, for the path that consults the filter + // through descendAsync instead. descendAsync hooks what it resolves, so the node is + // already part of the tree and refusing it here would not remove it. + // + // `fullBelow` is cleared first, as on the missing-node path above. It is a + // reference into the caller's stack entry, and this node is left on that stack, so + // a later pass over its remaining branches would otherwise reach the full-below + // test with it still set and record this subtree's hash as complete in the + // family-wide cache, where another map would trust it. + JLOG(journal_.warn()) << "Leaf " << childHash << " does not belong below " << nodeID + << " at branch " << branch << ", map is invalid"; + fullBelow = false; + state_ = SHAMapState::Invalid; + return; + } else if (d->isInner() && !safeDowncast(d)->isFullBelow(mn.generation)) { mn.stack.push(se); @@ -291,6 +313,29 @@ SHAMap::gmnProcessDeferredReads(MissingNodes& mn) auto nodePtr = std::get<3>(deferredNode); auto const& nodeHash = parent->getChildHash(branch); + // Guarded on depth for the same reason as the sibling test in gmnProcessNodes: a deferred + // entry carries the position the walk held when it posted the read, and the `pending` + // branch there records that position without building a child ID from it. So a child ID is + // asked for here only where the tree has room for one, which is the bound getChildNodeID + // keeps for itself. + if (nodePtr && nodePtr->isLeaf() && parentID.getDepth() < kLeafDepth && + !belongsAt(parentID.getChildNodeID(branch), *nodePtr)) + { + // The same judgment the two synchronous paths make (see SHAMap::descend and the + // descendAsync case in gmnProcessNodes), for a node an async read resolved. Every site + // that knows the position a node is about to take judges it here, which is what lets + // the traversal treat a misplaced leaf as a rarity rather than a routine case. + // + // Skips this node rather than returning: the reads still outstanding hold a pointer to + // `mn`, which lives in getMissingNodes' frame, and this loop is the only thing that + // waits for them. Returning early would let that frame go while a read was still due + // to write through it. + JLOG(journal_.warn()) << "Leaf " << nodeHash << " does not belong below " << parentID + << " at branch " << branch << ", map is invalid"; + state_ = SHAMapState::Invalid; + continue; + } + if (nodePtr) { // Got the node nodePtr = parent->canonicalizeChild(branch, std::move(nodePtr)); @@ -328,10 +373,15 @@ SHAMap::getMissingNodes(int max, SHAMapSyncFilter const* filter) 512, // number of async reads per pass f_.getFullBelowCache()->getGeneration()); + // Guarded with isValid() for the same reason the late return below is: clearSynching() moves + // the state to Modifying, which would erase a verdict an earlier walk already reached. No path + // to that was found, since every site that condemns the map also clears the fullBelow flag this + // return reads, but the rule holds either way and one conjunct is what it costs. if (!root_->isInner() || intr_ptr::staticPointerCast(root_)->isFullBelow(mn.generation)) { - clearSynching(); + if (isValid()) + clearSynching(); return std::move(mn.missingNodes); } @@ -416,7 +466,11 @@ SHAMap::getMissingNodes(int max, SHAMapSyncFilter const* filter) } while (node != nullptr); - if (mn.missingNodes.empty()) + // An empty result does not mean the map is complete when the walk judged it impossible on the + // way down: clearSynching() moves the state to Modifying, which would erase that verdict and + // report the map as satisfied. Asking nothing is the only part this has to get right, since + // clearSynching() is what a later walk would read. + if (mn.missingNodes.empty() && isValid()) clearSynching(); return std::move(mn.missingNodes); @@ -569,10 +623,6 @@ SHAMap::addKnownNode( { XRPL_ASSERT(!nodeID.isRoot(), "xrpl::SHAMap::addKnownNode : valid node"); XRPL_ASSERT(treeNode, "xrpl::SHAMap::addKnownNode : non-null tree node"); - XRPL_ASSERT_IF( - treeNode->isLeaf(), - nodeID.isPrefixOf(leafKey(*treeNode)), - "xrpl::SHAMap::addKnownNode : leaf position consistent with node ID"); if (!isSynching()) { @@ -606,6 +656,17 @@ SHAMap::addKnownNode( auto prevNode = inner; std::tie(currNode, currNodeID) = descend(inner, currNodeID, branch, filter); + if (!isValid()) + { + // descend judged a node on the way down and condemned the map. Stops here rather than + // falling through, for two reasons: `childHash` was read before that descent, so the + // hash comparison below would report a corrupt node against a sender that sent nothing + // wrong, and if the node descend refused is the one offered here, that comparison would + // instead succeed and hook it after all. + JLOG(journal_.warn()) << "Node " << nodeID << " cannot be hooked into an invalid map"; + return SHAMapAddNode::invalid(); + } + if (currNode != nullptr) continue; @@ -637,6 +698,19 @@ SHAMap::addKnownNode( return SHAMapAddNode::useful(); } + // A leaf's own key names its position, so a leaf offered for this slot has to agree with + // the ID it was offered under. The hash test above already proves the parent records this + // exact leaf here, so a disagreement is a property of the map rather than of the sender. + // This was an entry assertion, which is stripped under NDEBUG, and the node is hooked + // immediately below. + if (!belongsAt(nodeID, *treeNode)) + { + JLOG(journal_.warn()) << "Leaf " << treeNode->getHash() << " does not belong at " + << nodeID << ", map is invalid"; + state_ = SHAMapState::Invalid; + return SHAMapAddNode::invalid(); + } + if (backed_) canonicalize(childHash, treeNode); diff --git a/src/tests/libxrpl/shamap/SHAMap.cpp b/src/tests/libxrpl/shamap/SHAMap.cpp index abceade814..1f33902f0a 100644 --- a/src/tests/libxrpl/shamap/SHAMap.cpp +++ b/src/tests/libxrpl/shamap/SHAMap.cpp @@ -8,11 +8,13 @@ #include #include #include +#include #include #include #include #include #include +#include #include #include @@ -23,7 +25,9 @@ #include #include #include +#include #include +#include #include #include #include @@ -273,8 +277,8 @@ INSTANTIATE_TEST_SUITE_P( shamapBackingModeName); // Exercises the traversal stacks built by belowHelper. Each stack entry pairs a node with the ID -// naming its position, and SHAMap asserts that pairing on every push, so these traversals fail -// loudly in a Debug build if a node ID is ever derived from the wrong branch. +// naming its position, and every push refuses a leaf whose own key does not lie under the branch it +// was reached through, in Release builds as well as Debug ones. class SHAMapTraversal : public ::testing::Test { protected: @@ -461,9 +465,8 @@ TEST_F(SHAMapTraversal, bounds_on_empty_map_return_end) SHAMap map{SHAMapType::FREE, f}; map.setUnbacked(); - // The root is a childless inner node, so boundHelper's inner-node branch scans every branch on - // the requested side of the one id selects, finds them all empty, and falls through to end() - // rather than dereference a child. + // An empty map still leaves its root on the path, so end() here is an answer rather than a + // refusal. This is what stops boundHelper from reading an empty path as an empty map. EXPECT_EQ(map.upperBound(uint256{}), map.end()); EXPECT_EQ(map.lowerBound(uint256{}), map.end()); @@ -481,12 +484,10 @@ TEST_F(SHAMapTraversal, bounds_on_single_item_map_use_the_leaf_below_the_root) auto const key = deepFanOutKeys().front(); fillMap(map, {key}); - // root_ can be a leaf, but only after syncing a single-item map from a peer (addRootNode); - // fillMap builds this map in-process via addItem, which always leaves root_ as the inner node - // it was constructed with, with the single leaf one level below it. So the stack holds that - // inner root plus the leaf, and boundHelper examines the leaf first. Only a probe the leaf - // qualifies against is answered there; for the rest the leaf is popped and root_'s own - // inner-node scan runs, finds nothing on the requested side, and falls through to end(). + // fillMap uses addItem, which leaves root_ the inner node the map was constructed with and the + // single leaf one level below it. So the path holds both, and boundHelper judges the leaf + // first; for a probe the leaf does not qualify against it pops back to the root, whose scan + // finds nothing on the requested side. uint256 below = key; --below; uint256 above = key; @@ -947,4 +948,327 @@ TEST_F(SHAMapPathProof, substituted_leaf_for_other_key_is_rejected) EXPECT_FALSE(SHAMap::verifyProofPath(badRoot, kKey, badPath)); } +/** + * A filter that resolves exactly one node, by hash. + * + * Stands in for the real sync filters, which serve a node from a local cache + * keyed on its hash and so say nothing about where in a tree it belongs. + */ +class OneNodeFilter : public SHAMapSyncFilter +{ + std::map nodes_; + +public: + OneNodeFilter(SHAMapHash const& hash, Blob blob) + { + nodes_.emplace(hash, std::move(blob)); + } + + explicit OneNodeFilter(std::vector> nodes) + { + for (auto& [hash, blob] : nodes) + nodes_.emplace(hash, std::move(blob)); + } + + void + gotNode( + bool, + SHAMapHash const&, + std::uint32_t, + Blob&&, // NOLINT(cppcoreguidelines-rvalue-reference-param-not-moved) + SHAMapNodeType) const override + { + } + + [[nodiscard]] std::optional + getNode(SHAMapHash const& hash) const override + { + if (auto const it = nodes_.find(hash); it != nodes_.end()) + return it->second; + return std::nullopt; + } +}; + +// A tree whose hashes all agree can still put a leaf where its key does not belong, because a hash +// covers a node's contents rather than its position. Such a tree is what a proposer builds, and it +// is accepted node by node, so the paths that hook a node are where the position has to be judged. +class SHAMapMisplacedLeaf : public ::testing::Test +{ +protected: + beast::Journal const j_{TestSink::instance()}; + + // An arbitrary key whose first nibble is 1, so its leaf belongs under branch 1 of the root. + static constexpr uint256 kKey{ + "1c8cec8e5e9b0e5e0e0f5b3e2c9f7a1d6b4e8c2a0d7f3b9e5c1a8d4f2b6e0c93"}; + + // Any branch other than the one kKey selects at depth 0. + static constexpr unsigned int kWrongBranch = 5; + + /** + * A genuine leaf holding kKey, in the form a sync filter serves, with its + * hash. + * + * Taken from a map that placed the leaf correctly, so only its position is + * ever wrong below. Serialized with its prefix rather than in wire form, + * since that is what checkFilter parses. + * + * @param f the family the throwaway source map belongs to. + * @return the leaf's prefixed form and its hash, or an empty blob if the + * map rejected the item. + */ + static std::pair + genuineLeaf(Family& f) + { + SHAMap source{SHAMapType::FREE, f}; + source.setUnbacked(); + if (!source.addItem( + SHAMapNodeType::TnAccountState, + makeShamapitem(kKey, Slice{kKey.data(), kKey.size()}))) + { + return {}; + } + + auto const path = source.getProofPath(kKey); + if (!path.has_value() || path->empty()) + return {}; + + auto leaf = SHAMapTreeNode::makeFromWire(makeSlice(path->front())); + if (!leaf || !leaf->isLeaf()) + return {}; + leaf->updateHash(); + + Serializer s; + leaf->serializeWithPrefix(s); + return {s.getData(), leaf->getHash()}; + } + + /** + * Assemble `map` as a root inner node holding a leaf's hash under the + * wrong branch. + * + * The root is installed directly, as a peer's would be, so the leaf itself + * stays unresolved until a walk consults the filter for it. + * + * @param map the map to assemble, which must be synching and empty. + * @param leafHash the hash the forged root records under kWrongBranch. + * @return whether the root was accepted. + */ + static bool + forgeRoot(SHAMap& map, SHAMapHash const& leafHash) + { + Serializer s; + for (auto i = 0u; i < SHAMap::kBranchFactor; ++i) + s.addBitString(i == kWrongBranch ? leafHash.asUInt256() : uint256{}); + s.add8(kWireTypeInner); + + auto root = SHAMapTreeNode::makeFromWire(makeSlice(s.peekData())); + if (!root) + return false; + root->updateHash(); + + auto const rootHash = root->getHash(); + return map.addRootNode(rootHash, std::move(root), nullptr).isGood(); + } +}; + +// getMissingNodes reaches a filter through descendAsync, which hooks whatever it resolves. The +// verdict lands on the map, since every node from the root down hash-verified to get here. +TEST_F(SHAMapMisplacedLeaf, walking_for_missing_nodes_invalidates_the_map) +{ + tests::TestNodeFamily sourceFamily{j_}; + auto const [leafBlob, leafHash] = genuineLeaf(sourceFamily); + ASSERT_FALSE(leafBlob.empty()); + + // Its own family, so the leaf is reachable only through the filter rather than from a cache the + // source map warmed. + tests::TestNodeFamily targetFamily{j_}; + SHAMap map{SHAMapType::FREE, uint256{}, targetFamily}; + map.setUnbacked(); + ASSERT_TRUE(forgeRoot(map, leafHash)); + ASSERT_TRUE(map.isValid()); + + OneNodeFilter const filter{leafHash, leafBlob}; + map.getMissingNodes(1, &filter); + + EXPECT_FALSE(map.isValid()); +} + +// addKnownNode reaches a filter through the synchronous descend on its way to the position it was +// given, which is the other route a node takes into a tree during acquisition. +TEST_F(SHAMapMisplacedLeaf, hooking_a_known_node_invalidates_the_map) +{ + tests::TestNodeFamily sourceFamily{j_}; + auto const [leafBlob, leafHash] = genuineLeaf(sourceFamily); + ASSERT_FALSE(leafBlob.empty()); + + tests::TestNodeFamily targetFamily{j_}; + SHAMap map{SHAMapType::FREE, uint256{}, targetFamily}; + map.setUnbacked(); + ASSERT_TRUE(forgeRoot(map, leafHash)); + ASSERT_TRUE(map.isValid()); + + // A key whose first nibble is kWrongBranch, so the walk descends the branch holding the leaf. + // An inner node is offered rather than a leaf, since a leaf would have to agree with this + // position and the point here is to reach the descent, not to hook what is offered. + auto const target = SHAMapNodeID::createID( + 2, uint256{"5000000000000000000000000000000000000000000000000000000000000000"}); + + Serializer s; + for (auto i = 0u; i < SHAMap::kBranchFactor; ++i) + s.addBitString(i == 0u ? uint256{1} : uint256{}); + s.add8(kWireTypeInner); + auto offered = SHAMapTreeNode::makeFromWire(makeSlice(s.peekData())); + ASSERT_TRUE(offered); + offered->updateHash(); + + OneNodeFilter const filter{leafHash, leafBlob}; + auto const result = map.addKnownNode(target, std::move(offered), &filter); + + EXPECT_FALSE(map.isValid()); + + // The verdict matters as much as the state: it is what the acquisition paths charge a peer on, + // so a later change to it should fail here rather than pass quietly. + EXPECT_TRUE(result.isInvalid()); + EXPECT_FALSE(result.isGood()); +} + +// addKnownNode also hooks the very node it was handed, on the path where the local store has +// nothing to resolve for that slot. Such a node's position is known only from the ID the caller +// supplied, so it is judged against the leaf's own key before it is hooked. +TEST_F(SHAMapMisplacedLeaf, hooking_an_offered_misplaced_leaf_invalidates_the_map) +{ + tests::TestNodeFamily sourceFamily{j_}; + auto const [leafBlob, leafHash] = genuineLeaf(sourceFamily); + ASSERT_FALSE(leafBlob.empty()); + + tests::TestNodeFamily targetFamily{j_}; + SHAMap map{SHAMapType::FREE, uint256{}, targetFamily}; + map.setUnbacked(); + ASSERT_TRUE(forgeRoot(map, leafHash)); + ASSERT_TRUE(map.isValid()); + + // The branch the forged root files the leaf under, which is not the one kKey selects. + uint256 wrongPrefix; + wrongPrefix.begin()[0] = static_cast(kWrongBranch << 4); + auto const target = SHAMapNodeID::createID(1, wrongPrefix); + + auto offered = SHAMapTreeNode::makeFromPrefix(makeSlice(leafBlob), leafHash); + ASSERT_TRUE(offered); + ASSERT_TRUE(offered->isLeaf()); + + // No filter, so the walk resolves nothing locally and the node offered here is the one that + // would be hooked. + auto const result = map.addKnownNode(target, std::move(offered), nullptr); + + EXPECT_FALSE(map.isValid()); + EXPECT_TRUE(result.isInvalid()); + EXPECT_FALSE(result.isGood()); +} + +// A whole subtree can sit under the wrong branch through a single wrong child pointer, and that is +// cheaper to produce than one misplaced leaf. Every leaf below such a subtree agrees with its own +// final branch, because the subtree is internally well formed, and disagrees only at the level the +// pointer is wrong. So judging a leaf against the last branch alone accepts all of them, and only +// judging it against every branch above it refuses them. +TEST_F(SHAMapMisplacedLeaf, iterating_a_misplaced_subtree_throws) +{ + // Two keys sharing their first nibble, so they hang off one inner node at depth 1. + constexpr uint256 kFirst{"a100000000000000000000000000000000000000000000000000000000000000"}; + constexpr uint256 kSecond{"a200000000000000000000000000000000000000000000000000000000000000"}; + + tests::TestNodeFamily sourceFamily{j_}; + SHAMap source{SHAMapType::FREE, sourceFamily}; + source.setUnbacked(); + for (auto const& k : {kFirst, kSecond}) + { + ASSERT_TRUE(source.addItem( + SHAMapNodeType::TnAccountState, makeShamapitem(k, Slice{k.data(), k.size()}))); + } + source.invariants(); + + // The inner node holding both leaves, as the filter will serve it. It belongs under branch 10, + // the nibble the two keys share, and the forged root below files it under kWrongBranch instead. + auto const subtree = source.getProofPath(kFirst); + ASSERT_TRUE(subtree.has_value()); + // NOLINTBEGIN(bugprone-unchecked-optional-access) has_value() checked above + ASSERT_GE(subtree->size(), 2u); + + // getProofPath returns the path deepest element first, so the element above the leaf is the + // inner node the two keys share. + auto inner = SHAMapTreeNode::makeFromWire(makeSlice((*subtree)[1])); + // NOLINTEND(bugprone-unchecked-optional-access) + ASSERT_TRUE(inner); + ASSERT_TRUE(inner->isInner()); + inner->updateHash(); + + Serializer innerPrefixed; + inner->serializeWithPrefix(innerPrefixed); + + // Both leaves are served as well. Without them the walk would stop on a node it genuinely does + // not have, and the throw below would say nothing about position. + std::vector> served; + served.emplace_back(inner->getHash(), innerPrefixed.getData()); + for (auto const& k : {kFirst, kSecond}) + { + auto const leafPath = source.getProofPath(k); + ASSERT_TRUE(leafPath.has_value()); + // NOLINTBEGIN(bugprone-unchecked-optional-access) has_value() checked above + ASSERT_FALSE(leafPath->empty()); + auto leaf = SHAMapTreeNode::makeFromWire(makeSlice(leafPath->front())); + // NOLINTEND(bugprone-unchecked-optional-access) + ASSERT_TRUE(leaf); + ASSERT_TRUE(leaf->isLeaf()); + leaf->updateHash(); + + Serializer leafPrefixed; + leaf->serializeWithPrefix(leafPrefixed); + served.emplace_back(leaf->getHash(), leafPrefixed.getData()); + } + + tests::TestNodeFamily targetFamily{j_}; + SHAMap map{SHAMapType::FREE, uint256{}, targetFamily}; + map.setUnbacked(); + ASSERT_TRUE(forgeRoot(map, inner->getHash())); + + // The inner node itself carries no key, so nothing about it is out of place. Only a leaf below + // it can show that the branch it was reached through disagrees with the keys underneath. + OneNodeFilter const filter{std::move(served)}; + map.getMissingNodes(4, &filter); + + EXPECT_THROW(map.begin(), SHAMapMissingNode); + + // The bounds have to refuse the same map, and refusing is not the same as answering end(). + // This probe selects kWrongBranch at depth 0 and then the branch holding kFirst, so the walk + // reaches the misplaced leaf and clears the path. Both keys in the map are greater than the + // probe, so end() here would be the positive and wrong claim that no greater key exists. + uint256 probe; + probe.begin()[0] = static_cast((kWrongBranch << 4) | 0x1u); + ASSERT_GT(kFirst, probe); + ASSERT_GT(kSecond, probe); + + EXPECT_THROW(map.upperBound(probe), SHAMapMissingNode); + EXPECT_THROW(map.lowerBound(probe), SHAMapMissingNode); +} + +// The descendAsync walk leaves the leaf hooked, since it resolved the node before the position +// could be judged. Iterating it must not abort an instrumented build, and must not report the map +// as empty either, which is what a plain nullptr from belowHelper would have meant. +TEST_F(SHAMapMisplacedLeaf, iterating_a_hooked_misplaced_leaf_throws) +{ + tests::TestNodeFamily sourceFamily{j_}; + auto const [leafBlob, leafHash] = genuineLeaf(sourceFamily); + ASSERT_FALSE(leafBlob.empty()); + + tests::TestNodeFamily targetFamily{j_}; + SHAMap map{SHAMapType::FREE, uint256{}, targetFamily}; + map.setUnbacked(); + ASSERT_TRUE(forgeRoot(map, leafHash)); + + OneNodeFilter const filter{leafHash, leafBlob}; + map.getMissingNodes(1, &filter); + ASSERT_FALSE(map.isValid()); + + EXPECT_THROW(map.begin(), SHAMapMissingNode); +} + } // namespace xrpl::tests