diff --git a/.cspell.config.yaml b/.cspell.config.yaml index a46a9f2bac..9a152ea4d8 100644 --- a/.cspell.config.yaml +++ b/.cspell.config.yaml @@ -349,6 +349,7 @@ words: - unflatten - unfund - unimpair + - unjudged - unroutable - unscalable - unserviced diff --git a/cmake/XrplSanitizers.cmake b/cmake/XrplSanitizers.cmake index 893c880374..76b590d1e3 100644 --- a/cmake/XrplSanitizers.cmake +++ b/cmake/XrplSanitizers.cmake @@ -77,19 +77,24 @@ if(is_clang) message(STATUS " Ignorelist: ${ignorelist_path}") endif() -# Define SANITIZERS macro for BuildInfo.cpp +# Define SANITIZERS macro for BuildInfo.cpp, plus one of XRPL_ASAN/XRPL_TSAN/XRPL_UBSAN per +# active sanitizer, so other code can test for a specific one with #ifdef instead of parsing +# the dot-joined SANITIZERS string. set(sanitizers_list) if(SANITIZERS MATCHES "address") set(enable_asan ON) list(APPEND sanitizers_list "ASAN") + target_compile_definitions(common INTERFACE XRPL_ASAN) endif() if(SANITIZERS MATCHES "thread") set(enable_tsan ON) list(APPEND sanitizers_list "TSAN") + target_compile_definitions(common INTERFACE XRPL_TSAN) endif() if(SANITIZERS MATCHES "undefinedbehavior") set(enable_ubsan ON) list(APPEND sanitizers_list "UBSAN") + target_compile_definitions(common INTERFACE XRPL_UBSAN) endif() if(sanitizers_list) diff --git a/include/xrpl/shamap/SHAMap.h b/include/xrpl/shamap/SHAMap.h index 8f712e0e21..3767f88e77 100644 --- a/include/xrpl/shamap/SHAMap.h +++ b/include/xrpl/shamap/SHAMap.h @@ -143,10 +143,10 @@ private: /** * The map's state. * - * A getMissingNodes() walk writes it, through clearSynching(), while whatever - * drives the acquisition reads it. Nothing here requires the caller to hold a - * lock across the walk, and the acquisition code does not, so this is atomic - * rather than guarded. + * A getMissingNodes() walk writes it, through setInvalid() and + * clearSynching(), while whatever drives the acquisition reads it. + * Nothing here requires the caller to hold a lock across the walk, and + * the acquisition code does not, so this is atomic rather than guarded. */ std::atomic state_; SHAMapType const type_; @@ -339,9 +339,14 @@ public: * concurrency, to discover nodes referenced in the * SHAMap but not available locally. * + * Marks the map Invalid and abandons the traversal on meeting an inner + * node at or beyond kLeafDepth, a shape no valid tree can have, so + * callers must re-check isValid() before reading an empty result as + * "nothing left to fetch". + * * @param maxNodes The maximum number of found nodes to return * @param filter The filter to use when retrieving nodes - * @param return The nodes known to be missing + * @return The nodes known to be missing, or empty if the map is Invalid */ std::vector> getMissingNodes(int maxNodes, SHAMapSyncFilter const* filter); diff --git a/src/libxrpl/shamap/SHAMapSync.cpp b/src/libxrpl/shamap/SHAMapSync.cpp index e79d5a09a7..c14cc1d44c 100644 --- a/src/libxrpl/shamap/SHAMapSync.cpp +++ b/src/libxrpl/shamap/SHAMapSync.cpp @@ -227,7 +227,13 @@ SHAMap::gmnProcessNodes(MissingNodes& mn, MissingNodes::StackEntry& se) // we already know this child node is missing fullBelow = false; } - else if (!backed_ || !f_.getFullBelowCache()->touchIfExists(childHash.asUInt256())) + // The depth test precedes the cache lookup for the same reason it does in addKnownNode(): + // the cache is keyed by node hash and shared across maps, and a hash covers a node's + // children but not its depth, so a hit would skip the depth guard below. Skipping the + // shortcut only forgoes an optimization. + else if ( + !backed_ || isLeafDepth(nodeID.getDepth() + 1) || + !f_.getFullBelowCache()->touchIfExists(childHash.asUInt256())) { bool pending = false; auto d = descendAsync( @@ -258,6 +264,18 @@ SHAMap::gmnProcessNodes(MissingNodes& mn, MissingNodes::StackEntry& se) if (--mn.max <= 0) return; } + else if (d->isInner() && isLeafDepth(nodeID.getDepth() + 1)) + { + // Only a leaf belongs that deep (see isLeafDepth and SHAMap::addKnownNode). A node + // resolved locally never passes through addKnownNode(), so the walk has to reach + // this verdict itself. Ordered ahead of the full-below test below, which + // canonicalization shares across maps, or a node already marked full below would go + // unjudged. + JLOG(journal_.warn()) << "Inner node at branch " << branch << " below " << nodeID + << " makes the map invalid"; + setInvalid(); + return; + } else if (d->isInner() && !safeDowncast(d)->isFullBelow(mn.generation)) { mn.stack.push(se); @@ -331,17 +349,23 @@ SHAMap::gmnProcessDeferredReads(MissingNodes& mn) mn.deferred = 0; } -/** - * Get a list of node IDs and hashes for nodes that are part of this SHAMap - * but not available locally. The filter can hold alternate sources of - * nodes that are not permanently stored locally - */ std::vector> SHAMap::getMissingNodes(int max, SHAMapSyncFilter const* filter) { XRPL_ASSERT(root_->getHash().isNonZero(), "xrpl::SHAMap::getMissingNodes : nonzero root hash"); XRPL_ASSERT(max > 0, "xrpl::SHAMap::getMissingNodes : valid max input"); + // An already-invalid map short-circuits here instead of re-deriving the verdict in the walk + // below, which reaches it on its own. + if (!isValid()) + { + // journal_ is the family journal, shared by every map, so name which one this is. The root + // node's own hash rather than SHAMap::getHash(), which unshares the tree on a zero hash. + JLOG(journal_.warn()) << "getMissingNodes called on an invalid map, root hash " + << root_->getHash() << " seq " << ledgerSeq(); + return {}; + } + MissingNodes mn( max, filter, @@ -374,6 +398,11 @@ SHAMap::getMissingNodes(int max, SHAMapSyncFilter const* filter) { gmnProcessNodes(mn, pos); + // The walk just invalidated the map. Stop descending, but fall through to the drain + // below rather than returning, since posted reads hold a reference to mn. + if (!isValid()) + break; + if (mn.max <= 0) break; @@ -403,6 +432,11 @@ SHAMap::getMissingNodes(int max, SHAMapSyncFilter const* filter) if (mn.deferred != 0) gmnProcessDeferredReads(mn); + // Reads are drained, so the map can now be abandoned. Whatever was collected belongs to + // a tree that cannot exist, so discard it. + if (!isValid()) + return {}; + if (mn.max <= 0) return std::move(mn.missingNodes); @@ -436,6 +470,12 @@ SHAMap::getMissingNodes(int max, SHAMapSyncFilter const* filter) } while (node != nullptr); + // Tested once more, since an addKnownNode() on another thread can write the verdict after the + // loop's own test above, and an empty result would then be read as "satisfied". clearSynching() + // refuses either way, so this is about not asking rather than about the state it would leave. + if (!isValid()) + return {}; + if (mn.missingNodes.empty()) clearSynching(); diff --git a/src/tests/libxrpl/shamap/DeepChain.h b/src/tests/libxrpl/shamap/DeepChain.h index 57675b9781..745bde42a4 100644 --- a/src/tests/libxrpl/shamap/DeepChain.h +++ b/src/tests/libxrpl/shamap/DeepChain.h @@ -76,6 +76,24 @@ struct DeepChain { } + /** + * The same chain, with a second and unresolvable child at every level. + * + * On a backed map descendAsync() then posts a real asynchronous read at + * every level, which is what leaves reads in flight when a walk reaches + * kLeafDepth. Offered only for this shape: the decoy sits on branch 1, + * which is free only because a fabricated chain's pathKey is zero and + * so every real child sits on branch 0. + * + * @param seed Varies the whole chain. See the constructor. + * @return The chain. + */ + [[nodiscard]] static DeepChain + withDecoys(unsigned int seed = 1) + { + return DeepChain{std::nullopt, seed, Decoy::Yes}; + } + /** * A chain ending in a real transaction leaf, which completes an * acquisition. diff --git a/src/tests/libxrpl/shamap/SHAMapSync.cpp b/src/tests/libxrpl/shamap/SHAMapSync.cpp index e78a9ccd7e..8add6efd4f 100644 --- a/src/tests/libxrpl/shamap/SHAMapSync.cpp +++ b/src/tests/libxrpl/shamap/SHAMapSync.cpp @@ -27,12 +27,14 @@ #include #include +#include #include #include #include #include #include #include +#include #include #include #include @@ -217,6 +219,53 @@ protected: std::map served_; }; + /** + * A sync filter that serves a range of a DeepChain's nodes, by hash. + * + * Stands in for a fetch pack, which is checked against each node's own + * hash and never structurally, so a walk can resolve nodes locally + * without any of them passing through addKnownNode(). Anything outside + * the range - including a decoy child - looks unavailable. + */ + class ChainFilter : public SHAMapSyncFilter + { + public: + /** + * @param chain The chain whose nodes to serve. + * @param maxDepth The deepest node to serve. + * @param minDepth The shallowest node to serve. + */ + explicit ChainFilter( + DeepChain const& chain, + unsigned int maxDepth = SHAMap::kLeafDepth, + unsigned int minDepth = 0) + { + for (auto depth = minDepth; depth <= maxDepth; ++depth) + nodes_.emplace(chain.nodeAt(depth)->getHash(), chain.prefixedNodeAt(depth)); + } + + void + gotNode( + bool, + SHAMapHash const&, + std::uint32_t, + Blob&&, // NOLINT(cppcoreguidelines-rvalue-reference-param-not-moved) + SHAMapNodeType) const override + { + } + + [[nodiscard]] std::optional + getNode(SHAMapHash const& hash) const override + { + if (auto const it = nodes_.find(hash); it != nodes_.end()) + return it->second; + return std::nullopt; + } + + private: + std::map nodes_; + }; + /** * A root inner node with all 16 branches occupied and not one of them * resolvable. @@ -497,6 +546,243 @@ TEST_F(SHAMapSyncTest, snapshotOfInvalidMapStaysInvalid) EXPECT_TRUE(valid.snapShot(true)->isValid()); } +// getMissingNodes() refuses an invalid map outright, before ever consulting a filter. The +// offending node was never hooked into the tree by addKnownNode(), but without this guard a fetch +// pack could still resolve it and let a walk reach the same verdict itself (see +// getMissingNodesRejectsInnerNodeAtLeafDepth). +TEST_F(SHAMapSyncTest, getMissingNodesRefusesInvalidMap) +{ + TestNodeFamily f{j_}; + DeepChain const chain; + + SHAMap map{SHAMapType::FREE, f}; + // Unbacked, like TransactionAcquire's map, so the walk resolves synchronously. + map.setUnbacked(); + map.setSynching(); + + ASSERT_TRUE(chain.fill(map)); + + auto const offendingResult = chain.addOffendingNode(map); + ASSERT_TRUE(tallyIs(offendingResult, 0, 1, 0)); + ASSERT_FALSE(map.isValid()); + + // Only the node the map rejected, offered back the way a fetch pack would. + ChainFilter const filter{chain, SHAMap::kLeafDepth, SHAMap::kLeafDepth}; + + EXPECT_TRUE(map.getMissingNodes(kMaxNodesPerRequest, &filter).empty()); + EXPECT_FALSE(map.isValid()); + EXPECT_FALSE(map.setImmutable()); +} + +// A map can reach kLeafDepth without addKnownNode() ever being involved, since a fetch pack is +// not checked structurally and the walk resolves every level locally. The map stays Modifying +// throughout, so the isValid() guard never fires and the walk must reach the verdict itself. +TEST_F(SHAMapSyncTest, getMissingNodesRejectsInnerNodeAtLeafDepth) +{ + TestNodeFamily f{j_}; + DeepChain const chain; + + SHAMap map{SHAMapType::FREE, f}; + // Unbacked so the walk resolves each level synchronously through the filter. + map.setUnbacked(); + map.setSynching(); + + // Only the root goes in through the sync path; everything below comes from the filter, so + // nothing invalidates the map before the walk. + ASSERT_TRUE(map.addRootNode(chain.rootHash, chain.nodeAt(0), nullptr).isGood()); + ASSERT_TRUE(map.isValid()); + + ChainFilter const filter{chain}; + + EXPECT_TRUE(map.getMissingNodes(kMaxNodesPerRequest, &filter).empty()); + + // The walk reaches the same verdict addKnownNode() does, so the map is now unusable and + // unpersistable. + EXPECT_FALSE(map.isValid()); + EXPECT_FALSE(map.setImmutable()); + + // An empty result means "satisfied" for a valid map and clears the synching flag. Returning as + // soon as the map is abandoned keeps that call out of reach, and the map stays invalid across a + // second walk. + EXPECT_TRUE(map.getMissingNodes(kMaxNodesPerRequest, &filter).empty()); + EXPECT_FALSE(map.isValid()); +} + +// The depth guard must not be bypassable through the full-below cache. A node's hash covers its +// child hashes but not its depth, so an earlier walk can mark the same subtree hash complete at one +// depth and this one reach it at kLeafDepth, with no collision involved. This is the backed-map +// case, which is what InboundLedger uses. +TEST_F(SHAMapSyncTest, getMissingNodesRejectsInnerNodeAtLeafDepthOnCacheHit) +{ + TestNodeFamily f{j_}; + DeepChain const chain; + + // Backed, unlike the cases above, so the full-below cache is consulted at all. + SHAMap map{SHAMapType::FREE, f}; + map.setSynching(); + + ASSERT_TRUE(map.addRootNode(chain.rootHash, chain.nodeAt(0), nullptr).isGood()); + ASSERT_TRUE(map.isValid()); + + // Mark the offending node itself as full below. The cache is keyed on the hash of the child + // being considered, so this is what the lookup at the kLeafDepth boundary asks about, and a hit + // is what would skip the whole branch - guard included. + f.getFullBelowCache()->insert(chain.nodeAt(SHAMap::kLeafDepth)->getHash().asUInt256()); + + ChainFilter const filter{chain}; + + EXPECT_TRUE(map.getMissingNodes(kMaxNodesPerRequest, &filter).empty()); + EXPECT_FALSE(map.isValid()); + EXPECT_FALSE(map.setImmutable()); +} + +// Abandoning the walk must not abandon the reads it already posted. The MissingNodes block lives on +// getMissingNodes()'s stack frame and every posted read holds a reference to it, so the verdict +// breaks out of the descent but still falls through to the drain. Each level here has an +// unresolvable second child, so reads are in flight when the verdict lands. The failure mode is a +// use-after-free rather than a wrong answer, so it takes ASan to see. +TEST_F(SHAMapSyncTest, getMissingNodesDrainsPostedReadsWhenInvalidated) +{ + TestNodeFamily f{j_}; + auto const chain = DeepChain::withDecoys(); + + // Backed, so descendAsync() posts real asynchronous reads rather than resolving inline. + SHAMap map{SHAMapType::FREE, f}; + map.setSynching(); + + ASSERT_TRUE(map.addRootNode(chain.rootHash, chain.nodeAt(0), nullptr).isGood()); + ASSERT_TRUE(map.isValid()); + + // Only the chain nodes are served, so the decoy at each level has to be read asynchronously. + ChainFilter const filter{chain}; + + // The walk descends the chain, posting a read per level for the decoy child, and marks the map + // invalid on reaching kLeafDepth. Returning empty is the visible part; draining first is the + // part only a sanitizer can see. + EXPECT_TRUE(map.getMissingNodes(kMaxNodesPerRequest, &filter).empty()); + EXPECT_FALSE(map.isValid()); + EXPECT_FALSE(map.setImmutable()); +} + +// A walk that only meets legitimate depths must be left alone. Stopping one level short of +// kLeafDepth leaves a deepest node whose child is genuinely missing, so the walk reports it and +// the map stays valid. +TEST_F(SHAMapSyncTest, getMissingNodesAcceptsInnerNodeAboveLeafDepth) +{ + TestNodeFamily f{j_}; + DeepChain const chain; + + SHAMap map{SHAMapType::FREE, f}; + map.setUnbacked(); + map.setSynching(); + + ASSERT_TRUE(map.addRootNode(chain.rootHash, chain.nodeAt(0), nullptr).isGood()); + + // Everything except the node at kLeafDepth, which is the only one that would + // put the walk at a position no valid tree can occupy. + SHAMapHash const withheld = chain.nodeAt(SHAMap::kLeafDepth)->getHash(); + ChainFilter const filter{chain, SHAMap::kLeafDepth - 1}; + + auto const missing = map.getMissingNodes(kMaxNodesPerRequest, &filter); + + ASSERT_EQ(missing.size(), 1u); + EXPECT_EQ(missing[0].first.getDepth(), SHAMap::kLeafDepth); + EXPECT_EQ(missing[0].second, withheld.asUInt256()); + EXPECT_TRUE(map.isValid()); +} + +// The clearSynching() call site in addRootNode() needs a leaf root, and so a zero root hash. An +// invalid map always has an inner root with a non-zero hash, so the root is treated as a duplicate +// and the flag stands. +TEST_F(SHAMapSyncTest, addRootNodeLeavesInvalidMapInvalid) +{ + TestNodeFamily f{j_}; + DeepChain const chain; + + SHAMap map{SHAMapType::FREE, f}; + map.setUnbacked(); + map.setSynching(); + + ASSERT_TRUE(chain.fill(map)); + + auto const offendingResult = chain.addOffendingNode(map); + ASSERT_TRUE(tallyIs(offendingResult, 0, 1, 0)); + ASSERT_FALSE(map.isValid()); + + // A duplicate: counted as good, but nothing new was taken from the peer. + auto const result = map.addRootNode(chain.rootHash, chain.nodeAt(0), nullptr); + EXPECT_TRUE(tallyIs(result, 0, 0, 1)); + EXPECT_TRUE(result.isGood()); + EXPECT_FALSE(result.isUseful()); + + EXPECT_FALSE(map.isValid()); + EXPECT_FALSE(map.setImmutable()); +} + +// The concurrent half of the same contract: a walk writing Invalid while another thread calls +// setImmutable(). The verdict must win, and the map must end up unable to become immutable. +// +// What is deliberately not asserted is that a setImmutable() returning true implies a valid map +// when it returns. Nothing offers that: the compare-exchange can succeed and the walk can then +// write Invalid, all before the caller's next statement. The guarantee is only that trySetState() +// never leaves Invalid, which is what the post-join expectations below check. +// +// Only meaningful under ThreadSanitizer, which observes the collision this creates but cannot force +// it. Skipped at run time rather than compiled out, so every build still parses the body. Under +// SANITIZERS=thread, making state_ a plain member is reported as a data race here and +// setImmutable() then succeeds on an invalid map. The compare-exchange in trySetState() is not +// covered: an atomic load-then-store leaves a window too narrow to hit. +TEST_F(SHAMapSyncTest, invalidStateSurvivesConcurrentSetImmutable) +{ +#ifndef XRPL_TSAN + GTEST_SKIP() << "Only meaningful under ThreadSanitizer"; +#endif + + static constexpr auto kRounds = 200uz; + + for (auto round = 0uz; round < kRounds; ++round) + { + TestNodeFamily f{j_}; + DeepChain const chain; + + SHAMap map{SHAMapType::FREE, f}; + map.setUnbacked(); + map.setSynching(); + + // Only the root goes in through the sync path, so nothing has judged the map yet; the walk + // below resolves the rest through the filter and reaches the verdict itself. + ASSERT_TRUE(map.addRootNode(chain.rootHash, chain.nodeAt(0), nullptr).isGood()); + ChainFilter const filter{chain}; + + // One thread walks and invalidates; the other keeps calling setImmutable(). Started as + // close together as a latch allows, so the two collide somewhere in the middle rather than + // serializing. + std::atomic go{false}; + + std::thread walker([&] { + while (!go.load(std::memory_order_acquire)) + std::this_thread::yield(); + map.getMissingNodes(kMaxNodesPerRequest, &filter); + }); + + std::thread setter([&] { + while (!go.load(std::memory_order_acquire)) + std::this_thread::yield(); + for (auto attempt = 0uz; attempt < 64uz; ++attempt) + static_cast(map.setImmutable()); + }); + + go.store(true, std::memory_order_release); + walker.join(); + setter.join(); + + // The walk always reaches the verdict, so the map must end up invalid and unable to become + // immutable however the two threads interleaved. + EXPECT_FALSE(map.isValid()) << "round " << round; + EXPECT_FALSE(map.setImmutable()) << "round " << round; + } +} + // A map marked complete in the database withdraws that claim the first time a read misses, and // reports the miss once so the ledger can be re-acquired. Sixteen unresolvable branches are posted // in one pass, so with four reader threads the misses overlap and finishFetch() runs concurrently