ARCHIVE/rippled
1
0
Fork 0
mirror of https://github.com/XRPLF/rippled.git synced 2026-08-19 05:10:55 +00:00
Code Issues Packages Projects Releases Wiki Activity

fix: Clamp the depth used to index selectBranch's key byte

Browse Source 
`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`.
This commit is contained in:
Bart
2026-08-01 21:35:54 -04:00
parent d854982fd7
commit 6f0f065505
2 changed files with 242 additions and 7 deletions
Download Patch File Download Diff File Expand all files Collapse all files

59
src/libxrpl/shamap/SHAMapNodeID.cpp
View File

@@ -6,6 +6,7 @@
#include <xrpl/protocol/Serializer.h>
#include <xrpl/shamap/SHAMap.h>
#include <algorithm>
#include <cstddef>
#include <optional>
#include <stdexcept>
@@ -40,9 +41,39 @@ depthMask(unsigned int depth)
return kMasks.entry[depth];
}
// The prefix of `key` at `depth`: the leading nibbles naming the subtree a node at that depth
// identifies, with the remainder of the key masked off.
static uint256
maskedToDepth(uint256 const& key, unsigned int depth)
{
return key & depthMask(depth);
}
// Whether `id` at `depth` is what `key` looks like once masked down to that depth, i.e.
// whether an ID with this depth and id names a subtree that `key` falls under.
static bool
isPrefixOfAtDepth(uint256 const& id, unsigned int depth, uint256 const& key)
{
return maskedToDepth(key, depth) == id;
}
// canonicalize the hash to a node ID for this depth
SHAMapNodeID::SHAMapNodeID(unsigned int depth, uint256 const& hash) : id_(hash), depth_(depth)
{
// Every SHAMapNodeID's depth is stored here, so this is the one place that can stop an
// out-of-range one from being kept: a depth past kLeafDepth would go on to index depthMask
// out of bounds, and getRawString would narrow it to a byte, silently renaming the node.
// Clamp rather than throw, since node IDs are built from peer-supplied depths on the ledger
// data path, where no caller catches an exception before it reaches a thread boundary.
if (depth_ > SHAMap::kLeafDepth)
{
// LCOV_EXCL_START
UNREACHABLE("xrpl::SHAMapNodeID::SHAMapNodeID : depth within tree");
depth_ = SHAMap::kLeafDepth;
id_ = maskedToDepth(id_, depth_);
// LCOV_EXCL_STOP
}
XRPL_ASSERT(
depth <= SHAMap::kLeafDepth, "xrpl::SHAMapNodeID::SHAMapNodeID : maximum depth input");
XRPL_ASSERT(
@@ -89,7 +120,7 @@ SHAMapNodeID::getChildNodeID(unsigned int branch) const
bool
SHAMapNodeID::isPrefixOf(uint256 const& key) const
{
return (key & depthMask(depth_)) == id_;
return isPrefixOfAtDepth(id_, depth_, key);
}
[[nodiscard]] std::optional<SHAMapNodeID>
@@ -102,9 +133,9 @@ deserializeSHAMapNodeID(void const* data, std::size_t size)
unsigned int const depth = *(static_cast<unsigned char const*>(data) + 32);
if (depth <= SHAMap::kLeafDepth)
{
auto const id = uint256::fromVoid(data);
if (id == (id & depthMask(depth)))
// Reject a serialized ID carrying bits below its own depth. Checked before
// constructing, since the constructor asserts that same property.
if (auto const id = uint256::fromVoid(data); isPrefixOfAtDepth(id, depth, id))
ret.emplace(depth, id);
}
}
@@ -115,7 +146,11 @@ deserializeSHAMapNodeID(void const* data, std::size_t size)
[[nodiscard]] unsigned int
selectBranch(SHAMapNodeID const& id, uint256 const& hash)
{
auto const depth = id.getDepth();
XRPL_ASSERT(id.getDepth() < SHAMap::kLeafDepth, "xrpl::selectBranch : depth below leaf depth");
// A depth-64 ID has no nibble left to select. Callers must not ask, but clamp anyway to keep
// the read below the end of the 32-byte key.
auto const depth = std::min(id.getDepth(), SHAMap::kLeafDepth - 1u);
auto branch = static_cast<unsigned int>(*(hash.begin() + (depth / 2)));
if ((depth & 1) != 0u)
@@ -134,8 +169,18 @@ selectBranch(SHAMapNodeID const& id, uint256 const& hash)
SHAMapNodeID
SHAMapNodeID::createID(unsigned int depth, uint256 const& key)
{
XRPL_ASSERT(depth <= SHAMap::kLeafDepth, "xrpl::SHAMapNodeID::createID : valid depth");
return SHAMapNodeID(depth, key & depthMask(depth));
// The mask is chosen here, before the constructor runs, so the clamp there cannot cover this
// call: an out-of-range depth would index depthMask's table while still evaluating this
// argument. A public factory has to hold its own bound.
if (depth > SHAMap::kLeafDepth)
{
// LCOV_EXCL_START
UNREACHABLE("xrpl::SHAMapNodeID::createID : depth within tree");
depth = SHAMap::kLeafDepth;
// LCOV_EXCL_STOP
}
return SHAMapNodeID(depth, maskedToDepth(key, depth));
}
} // namespace xrpl

190
src/tests/libxrpl/shamap/SHAMapNodeID.cpp Normal file
View File

@@ -0,0 +1,190 @@
#include <xrpl/shamap/SHAMapNodeID.h>
#include <xrpl/basics/base_uint.h>
#include <xrpl/protocol/Serializer.h>
#include <xrpl/shamap/SHAMap.h>
#include <gtest/gtest.h>
#include <stdexcept>
namespace xrpl::tests {
// An arbitrary 32-byte key reused across tests below that don't care about its specific value,
// only that it is a well-formed key.
constexpr uint256 kTestKey("b92891fe4ef6cee585fdc6fda1e09eb4d386363158ec3321b8123e5a772c6ca8");
TEST(SHAMapNodeIDTest, root_is_prefix_of_every_key)
{
SHAMapNodeID const root;
EXPECT_EQ(root.getDepth(), 0u);
EXPECT_TRUE(root.isPrefixOf(uint256{}));
EXPECT_TRUE(root.isPrefixOf(kTestKey));
}
TEST(SHAMapNodeIDTest, child_id_is_prefix_of_keys_in_that_branch)
{
// Walking the branches spelled by the key's own nibbles must keep every
// intermediate ID a prefix of that key.
SHAMapNodeID id;
for (auto depth = 0u; depth < SHAMap::kLeafDepth; ++depth)
{
id = id.getChildNodeID(selectBranch(id, kTestKey));
EXPECT_EQ(id.getDepth(), depth + 1);
EXPECT_TRUE(id.isPrefixOf(kTestKey)) << "depth " << id.getDepth();
}
}
TEST(SHAMapNodeIDTest, wrong_branch_is_not_prefix_of_key)
{
SHAMapNodeID const root;
auto const correct = selectBranch(root, kTestKey);
ASSERT_EQ(correct, 0xbu);
// An ID built from the wrong branch still has a valid depth and a self-consistent mask, so
// isPrefixOf(kTestKey) below is what actually distinguishes the correct branch from the rest.
for (auto branch = 0u; branch < SHAMap::kBranchFactor; ++branch)
{
auto const child = root.getChildNodeID(branch);
EXPECT_EQ(child.getDepth(), 1u);
EXPECT_EQ(child.isPrefixOf(kTestKey), branch == correct) << "branch " << branch;
}
}
TEST(SHAMapNodeIDTest, prefix_check_is_depth_sensitive)
{
// kTestKey and kOther agree on the first two nibbles ("b9") and then diverge.
constexpr uint256 kOther("b99891fe4ef6cee585fdc6fda1e09eb4d386363158ec3321b8123e5a772c6ca8");
auto id = SHAMapNodeID{}.getChildNodeID(selectBranch(SHAMapNodeID{}, kTestKey));
EXPECT_TRUE(id.isPrefixOf(kTestKey));
EXPECT_TRUE(id.isPrefixOf(kOther)) << "shared first nibble";
id = id.getChildNodeID(selectBranch(id, kTestKey));
EXPECT_TRUE(id.isPrefixOf(kTestKey));
EXPECT_TRUE(id.isPrefixOf(kOther)) << "shared second nibble";
// Third nibble differs, so the deeper ID no longer covers kOther.
id = id.getChildNodeID(selectBranch(id, kTestKey));
EXPECT_TRUE(id.isPrefixOf(kTestKey));
EXPECT_FALSE(id.isPrefixOf(kOther));
}
TEST(SHAMapNodeIDTest, leaf_id_from_key_is_prefix_of_that_key)
{
SHAMapNodeID const leaf{SHAMap::kLeafDepth, kTestKey};
EXPECT_TRUE(leaf.isPrefixOf(kTestKey));
// At full depth the prefix is the whole key, so nothing else matches.
constexpr uint256 kOther("b92891fe4ef6cee585fdc6fda1e09eb4d386363158ec3321b8123e5a772c6ca9");
EXPECT_FALSE(leaf.isPrefixOf(kOther));
}
TEST(SHAMapNodeIDTest, create_id_masks_key_to_depth)
{
for (auto depth = 0u; depth <= SHAMap::kLeafDepth; ++depth)
{
auto const id = SHAMapNodeID::createID(depth, kTestKey);
EXPECT_EQ(id.getDepth(), depth);
EXPECT_TRUE(id.isPrefixOf(kTestKey)) << "depth " << depth;
}
}
// The guards below must hold with XRPL_ASSERT compiled out (NDEBUG), so each one
// has to be a real runtime check rather than an assert.
TEST(SHAMapNodeIDTest, child_of_leaf_depth_id_throws)
{
auto const leafDepthID = SHAMapNodeID::createID(SHAMap::kLeafDepth, kTestKey);
ASSERT_EQ(leafDepthID.getDepth(), SHAMap::kLeafDepth);
EXPECT_THROW((void)leafDepthID.getChildNodeID(0), std::logic_error);
}
TEST(SHAMapNodeIDTest, out_of_range_depth_is_clamped)
{
// A depth past kLeafDepth has no mask in depthMask's 65-entry table, so both the constructor
// and createID clamp it. createID needs its own clamp: it picks the mask while evaluating the
// constructor's argument, so the constructor's clamp cannot cover that read.
//
// Both clamps are marked UNREACHABLE, which is an assert and therefore fatal wherever asserts
// are live. Only a build with them compiled out (or routed to Antithesis's non-fatal handler)
// reaches the clamp itself, so that is the only configuration that can assert on the result.
#if defined(NDEBUG) || defined(ENABLE_VOIDSTAR)
for (auto const depth : {SHAMap::kLeafDepth + 1u, 100u, 255u, 256u, 320u})
{
auto const id = SHAMapNodeID::createID(depth, kTestKey);
// Clamped to a real depth, not the depth asked for, and not a byte-narrowed version of it:
// 256 would otherwise become 0 and name the root, 320 would become 64.
EXPECT_EQ(id.getDepth(), SHAMap::kLeafDepth) << "depth " << depth;
// id_ and depth_ still agree, so the object is usable rather than merely non-crashing.
EXPECT_TRUE(id.isPrefixOf(kTestKey)) << "depth " << depth;
EXPECT_EQ(id, SHAMapNodeID::createID(SHAMap::kLeafDepth, kTestKey)) << "depth " << depth;
// The clamp holds through the wire format too, which encodes the depth in one byte.
auto const roundTripped = deserializeSHAMapNodeID(id.getRawString());
ASSERT_TRUE(roundTripped.has_value()) << "depth " << depth;
EXPECT_EQ(roundTripped->getDepth(), SHAMap::kLeafDepth) << "depth " << depth;
}
// The constructor clamps on its own, for the paths that do not go through createID.
SHAMapNodeID const direct{SHAMap::kLeafDepth + 1u, uint256{}};
EXPECT_EQ(direct.getDepth(), SHAMap::kLeafDepth);
#else
EXPECT_DEATH(
(void)SHAMapNodeID::createID(SHAMap::kLeafDepth + 1u, kTestKey), "depth within tree");
#endif
}
TEST(SHAMapNodeIDTest, select_branch_clamps_leaf_depth)
{
// selectBranch's own precondition is depth < kLeafDepth: a depth-64 ID has no nibble left
// to select. That makes it unlike the guards above, which have a throw/return reachable
// even with XRPL_ASSERT compiled out; selectBranch has no such path, so the two build
// configurations have to be tested differently.
//
// Under ENABLE_VOIDSTAR, XRPL_ASSERT routes to Antithesis's assert_impl, which only records
// the hit and returns rather than aborting, even though NDEBUG is undefined there (voidstar
// requires a Debug build). So the assert is live in name but never fatal, the same as the
// NDEBUG case below.
auto const leafDepthID = SHAMapNodeID::createID(SHAMap::kLeafDepth, kTestKey);
#if defined(NDEBUG) || defined(ENABLE_VOIDSTAR)
// With the assert compiled out or routed to a non-fatal handler, the clamp is what stands
// between this call and reading past the end of the 32-byte key. Clamping means it reads the
// same byte, and returns the same branch, as the deepest ID that still has one: depth 63.
auto const deepestWithBranchID = SHAMapNodeID::createID(SHAMap::kLeafDepth - 1u, kTestKey);
auto const branch = selectBranch(leafDepthID, kTestKey);
EXPECT_LT(branch, SHAMap::kBranchFactor);
EXPECT_EQ(branch, selectBranch(deepestWithBranchID, kTestKey));
#else
// In a debug build the assert is live and must reject this call outright, in a forked
// process so a failure here cannot take down the rest of the suite.
EXPECT_DEATH((void)selectBranch(leafDepthID, kTestKey), "depth below leaf depth");
#endif
}
TEST(SHAMapNodeIDTest, deserialize_rejects_out_of_range_depth)
{
// getRawString() only serializes a depth already accepted by the constructor's own
// assertion, so an out-of-range depth here is built by hand instead.
auto serializeWithRawDepth = [](unsigned int depth) {
Serializer s;
s.addBitString(uint256{});
s.add8(static_cast<unsigned char>(depth));
return s.getString();
};
for (auto const depth : {65u, 100u, 255u})
EXPECT_FALSE(deserializeSHAMapNodeID(serializeWithRawDepth(depth)).has_value())
<< "depth " << depth;
// A depth-64 ID is legal, since leaves live there, but it has no children.
auto const id =
deserializeSHAMapNodeID(SHAMapNodeID{SHAMap::kLeafDepth, uint256{}}.getRawString());
ASSERT_TRUE(id.has_value());
EXPECT_THROW((void)id->getChildNodeID(0), std::logic_error);
}
} // namespace xrpl::tests