mirror of
https://github.com/XRPLF/rippled.git
synced 2026-08-21 22:30:57 +00:00
029a330c91
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.