mirror of
https://github.com/XRPLF/rippled.git
synced 2026-08-22 06:40:53 +00:00
2ab143cb9c
`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`.