fix: Refuse to walk an invalid SHAMap in getMissingNodes

A walk that reaches a position only a leaf may occupy now marks the map
Invalid and abandons the descent instead of continuing:
SHAMapNodeID::getChildNodeID() throws past kLeafDepth, uncaught, all the
way to std::terminate(). It's reachable without going through
addKnownNode() at all - InboundLedgers::gotStaleData() stores any
parseable node from an unsolicited liAS_NODE reply into the fetch pack by
its own hash with no relatedness check - making this a conditioned remote
denial of service, not just a single bad packet.

As in addKnownNode(), the depth check runs before the full-below cache
lookup, for the same cache-doesn't-cover-depth reason. Callers must
re-check isValid() before reading an empty result as nothing left to
fetch, which getMissingNodes()'s docstring now says.

.cspell.config.yaml

@@ -349,6 +349,7 @@ words:
   - unflatten
   - unfund
   - unimpair
+  - unjudged
   - unroutable
   - unscalable
   - unserviced

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)

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<SHAMapState> 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<std::pair<SHAMapNodeID, uint256>>
     getMissingNodes(int maxNodes, SHAMapSyncFilter const* filter);

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<SHAMapInnerNode*>(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<std::pair<SHAMapNodeID, uint256>>
 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();
 

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.

src/tests/libxrpl/shamap/SHAMapSync.cpp

@@ -27,12 +27,14 @@
 #include <shamap/DeepChain.h>
 #include <shamap/common.h>
 
+#include <atomic>
 #include <chrono>
 #include <cstddef>
 #include <cstdint>
 #include <list>
 #include <map>
 #include <optional>
+#include <thread>
 #include <unordered_set>
 #include <utility>
 #include <vector>
@@ -217,6 +219,53 @@ protected:
         std::map<SHAMapHash, Blob> 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<Blob>
+        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<SHAMapHash, Blob> 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<bool> 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<void>(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