`belowHelper` built each stack entry's `SHAMapNodeID` from `branch`, the branch
used to reach the subtree root, rather than `childBranch`, the branch it had
just descended. The resulting IDs carried a correct depth but named a
different subtree, and nothing rejected them: such an ID has a legal depth and
a legal mask, so only comparing it against an actual leaf key exposes the
mismatch. The affected stacks feed read-only traversals whose consumers use
only the depth, so no ledger state, hash, or peer message was affected, but
any future consumer of `getNodeID()` would have silently received the wrong
position.
Rather than fix the one call, make the mistake unrepresentable.
`NodePathStack` replaces the bare `std::stack` and refuses to accept an ID at
all: every push takes the branch being descended and derives the ID itself, so
a node and its ID cannot disagree. `isPrefixOf` assertions on each push catch a
wrong branch at the point it happens rather than wherever the ID is later
read. Leaf entries now keep the depth they were reached at instead of a
normalized `kLeafDepth`, which is what lets those assertions hold:
`addGiveItem` splits a leaf from the depth it actually sits at.
The new traversal tests fail on the previous code: reverting the branch
derivation trips the leaf-key assertion on the first iteration. Also adds a
`deepFanOutKeysAtLeafDepth` helper and mirrors them against it, since the
existing `deepFanOutKeys`'s fan-out at the 6th nibble keeps its tree only
about 6 levels deep and never exercised the depth-63/64 code these tests are
meant to protect, plus a case that collapses the entire depth-63 chain of
single-child inner nodes into a leaf on the final delete, which the
every-other-key deletion pattern the other new tests use never triggers.
`selectBranch` reads the key byte at `depth / 2`, which is out of bounds for a
32-byte key once the depth reaches 64. Every branch selection in the map
funnels through here, so this is the one place a stray depth can turn into a
bad read. Assert the precondition for callers, then clamp anyway: a wrong
answer for an input that should never occur is better than reading past the
buffer. Verified under ASan that the unclamped form reads one byte past a
32-byte allocation while the clamped form does not.
`depthMask`'s own 65-entry table had the same exposure one level up, reachable
through the public `createID` factory rather than only from inside the map. A
depth past `kLeafDepth` indexed that table out of bounds, confirmed under ASan
as a 4-byte `global-buffer-overflow` immediately after `kMasks`. Both places
that can set a depth now clamp it: the constructor, which is the single point
every `SHAMapNodeID`'s `depth_` passes through, and `createID`, which needs its
own bound because it picks the mask while evaluating the constructor's
argument, before the constructor body could correct anything.
Clamping rather than throwing, which is what `getChildNodeID` does for the
analogous case: `createID` is reached from `getSHAMapNodeID` with a
peer-supplied depth, and two of that function's three callers
(`InboundTransactions::gotData`, `PeerImp::onMessage`) sit on paths with no
handler between them and a thread boundary, so a throw there would end the
process rather than the message. Clamping also has to fix up `id_` alongside
`depth_`, since a node ID whose id and depth disagree fails the invariant every
read of `id_` relies on. Leaving the depth unclamped would additionally let
`getRawString` narrow it to a byte, turning depth 256 into a node claiming to
be the root.
`deserializeSHAMapNodeID` gets the same mask check `isPrefixOf` already
performs, pulled into a shared `isPrefixOfAtDepth` helper, and the masking both
it and `createID` perform is now a named `maskedToDepth` rather than a repeated
bitwise-and.
Tests cover the depth-sensitivity of `isPrefixOf`, the guards that must hold
with asserts stripped, that `deserializeSHAMapNodeID` rejects an out-of-range
depth, and the clamp itself under both build configurations (`EXPECT_DEATH`
in a forked process when the assert is live, and the clamped result compared
against depth 63 when it is not). The clamp test also gates on
`ENABLE_VOIDSTAR`, not just `NDEBUG`: under Antithesis instrumentation
`XRPL_ASSERT` routes to a handler that records the hit but never aborts, so a
Debug build with voidstar enabled has `NDEBUG` undefined yet still hits the
same non-fatal assert as a release build, and without this gate would send
that configuration into the `EXPECT_DEATH` arm, where the forked child never
dies and the test fails, breaking the CI job that runs this suite under
`-Dvoidstar=ON`.
A SHAMap has 65 levels, and nibbles run out at level 64: `selectBranch` indexes
the key byte at `depth / 2`, so at depth 64 it reads byte 32 of a 32-byte key.
Only the leaf terminating a path may sit at that depth, but proof path nodes
come off the wire, so a peer could send 65 hash-chained inner nodes and drive
that read past the end of the buffer.
The existing length bound cannot be tightened to catch this: a path for two
keys sharing all 63 leading nibbles legitimately holds 64 inner nodes plus a
leaf, so 65 elements is valid. The claimed node type at the final depth is the
thing to reject, using `>=` rather than `==` to match the convention every
other `kLeafDepth` comparison in this codebase already follows.
Reachable from `TMProofPathResponse` via `LedgerReplayMsgHandler`; confirmed
under ASan with asserts compiled out that the unguarded read lands one byte
past a 32-byte heap allocation. Also closes a second gap: nothing checked the
terminal leaf's own key against `key`, so the hash chain alone let a peer
substitute any leaf whose subtree hashes matched at every level above it.
Pinned by tests: the 65-element path that must verify for both keys sharing
the deep prefix, and the forged all-inner path that must not.
Also guards `visitDifferences` against an inner node claimed at leaf depth:
`hasLeafNode` only checked the comparison map, not the map being walked, so a
corrupt node in the map under `visitDifferences` itself could still throw
uncaught. Skip such a node's children instead.
