ARCHIVE/rippled
1
0
Fork 0
mirror of https://github.com/XRPLF/rippled.git synced 2026-08-21 22:30:57 +00:00
Code Issues Packages Projects Releases Wiki Activity

fix: Make NodePathStack fail closed when assertions are compiled out

Browse Source 
The stack asserted its preconditions and then went ahead regardless. Asserts
expand to `assert`, so in a release build every one of those was a no-op in
front of the operation it was guarding: reading or popping an empty
`std::stack` is undefined, `pushChild`'s out-of-range branch check was missing
entirely, and `getChildNodeID` throws `std::logic_error` at leaf depth. None
of these conditions are reachable through any of `SHAMap`'s public entry
points today, but the failure modes if they ever did happen would be
disproportionate: an out-of-range branch would silently corrupt a node ID
instead of failing loudly, and a `logic_error` reaching an unguarded call
chain would abort the process, since nothing in this codebase catches it.

Pushes now return false instead of throwing or silently corrupting the ID, and
reads degrade to a null node rather than undefined behavior. `[[nodiscard]]`
makes an unchecked push a compile error. Each new guard is marked
`UNREACHABLE` rather than left implicitly untested, since no test fixture in
this suite can reach these paths without building a deliberately corrupt map.
`pushRoot` gets the same conversion as every sibling method; it was the one
push still asserting instead of returning false.

`walkTowardsKey`'s two modes (with and without a caller-supplied stack) must
fail at the same node and leave the stack in a state every caller already
knows how to handle; on failure the stack is now cleared via a restored
`clear()`, and a restored `pushCurrent` lambda keeps the loop-entry and
post-loop push-and-clear logic from being duplicated. It also stops deriving
each node ID twice: a caller-supplied stack now reads the ID `pushNode` just
computed off `stack->top().second`, instead of a redundant local copy that
additionally went stale once the loop exited. `belowHelper` and
`peekNextItem` finally get the fallback `top()`'s own docstring promises: both
read `stack.top()` right after an assert-only emptiness check, with no
fallback for release builds, so both now return early on an empty stack
instead of dereferencing a null `SHAMapTreeNodePtr`.

`pushChild`'s hard guard also only checked the parent's depth against
`kLeafDepth`, one level too permissive for an inner child: a parent at 63
passed the check, then pushed an inner child at 64 with only a debug-only
assert catching it, the exact gap this commit exists to close. Tightened to
require depth + 1 below `kLeafDepth` for an inner child, with
`walkTowardsKey`'s no-stack path given the identical tightening so a
malformed map fails at the same node in both modes.
This commit is contained in:
Bart
2026-08-02 06:38:22 -04:00
parent 0f0b8fc650
commit ee6ddfbfdb
2 changed files with 111 additions and 31 deletions
Download Patch File Download Diff File Expand all files Collapse all files

56
src/libxrpl/shamap/SHAMap.cpp
View File

@@ -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<SHAMapInnerNode&>(*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<SHAMapLeafNode*>(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<SHAMapLeafNode*>(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<SHAMapMissingNode>(type_, id);
auto leaf = belowHelper(stack, BelowDirection::First);
if (leaf == nullptr)
Throw<SHAMapMissingNode>(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<SHAMapMissingNode>(type_, id);
auto const leaf = belowHelper(stack, direction);
if (leaf == nullptr)
Throw<SHAMapMissingNode>(type_, id);
@@ -768,7 +795,8 @@ SHAMap::addGiveItem(SHAMapNodeType type, boost::intrusive_ptr<SHAMapItem const>
while ((b1 = selectBranch(nodeID, tag)) == (b2 = selectBranch(nodeID, otherItem->key())))
{
stack.pushNode(node, tag);
if (!stack.pushNode(node, tag))
Throw<SHAMapMissingNode>(type_, tag);
// we need a new inner node, since both go on same branch at this
// level