ltc/node: fix kr1z1s use-after-free / SIGSEGV race (hold tracker lock in phase2 + guard notify contains) [#759 RANK-1]

src/impl/ltc/node.cpp

@@ -395,21 +395,38 @@ void NodeImpl::processing_shares_phase2(HandleSharesData& data, NetService addr)
     // Non-blocking mutex: if think() holds the exclusive lock on the compute
     // thread, queue this batch for processing after think() releases. The IO
     // thread never blocks — keepalive timers and network I/O continue.
-    {
-        std::unique_lock lock(m_tracker_mutex, std::try_to_lock);
-        if (!lock.owns_lock()) {
-            LOG_INFO << "[ASYNC-DEFER] processing_shares_phase2: mutex busy, queuing "
-                     << data.m_items.size() << " shares from " << addr.to_string()
-                     << " (pending=" << m_pending_adds.size() + 1 << ")";
-            m_pending_adds.push_back(PendingShareBatch{
-                std::make_unique<HandleSharesData>(std::move(data)), addr});
+    //
+    // HOLD this lock across the entire mutation body below (mirrors LTC f445db8e).
+    // try_to_lock keeps the IO thread non-blocking — busy => queue + return. Once
+    // acquired we must NOT release until all m_tracker.chain mutations are done:
+    // the prior code released here and ran the body lock-free, letting the
+    // compute-thread clean_tracker() exclusive prune (drop_tails) free chain nodes
+    // mid-mutation -> SIGSEGV (kr1z1s LTC/DGB). Released just before run_think().
+    std::unique_lock lock(m_tracker_mutex, std::try_to_lock);
+    if (!lock.owns_lock()) {
+        // ── Backpressure (V36 livelock defense-in-depth) ──────────────
+        // If think() is wedged/slow the deferred queue could grow without
+        // bound and blow memory. Cap it: over MAX_PENDING_ADDS we DROP the
+        // new batch (peers re-advertise their best share, so dropped shares
+        // are re-requested later) and warn instead of growing unbounded.
+        if (m_pending_adds.size() >= MAX_PENDING_ADDS) {
+            LOG_WARNING << "[ASYNC-DEFER] BACKPRESSURE: pending_adds at cap ("
+                        << m_pending_adds.size() << "/" << MAX_PENDING_ADDS
+                        << "), dropping batch of " << data.m_items.size()
+                        << " shares from " << addr.to_string()
+                        << " — think() may be wedged";
             return;
         }
+        LOG_INFO << "[ASYNC-DEFER] processing_shares_phase2: mutex busy, queuing "
+                 << data.m_items.size() << " shares from " << addr.to_string()
+                 << " (pending=" << m_pending_adds.size() + 1 << ")";
+        m_pending_adds.push_back(PendingShareBatch{
+            std::make_unique<HandleSharesData>(std::move(data)), addr});
+        return;
     }
-    // Lock released — proceed with normal processing.
-    // No lock needed for the rest: we only enter here when think() is NOT
-    // running (m_tracker_mutex was available), and ASIO single-thread
-    // guarantees no other IO handler overlaps.
+    // Lock acquired and HELD across the mutation body below; released just before
+    // the async run_think() trigger so the compute thread can take the exclusive
+    // lock. ASIO single-thread still guarantees no overlapping IO handler.
 
     // Step 1: collect verified shares (skip any that failed verification, hash still null)
     std::vector<ShareType> valid_shares;
@@ -570,6 +587,11 @@ void NodeImpl::processing_shares_phase2(HandleSharesData& data, NetService addr)
                  << " chain=" << m_tracker.chain.size() << ")";
     }
 
+    // Release the tracker lock before triggering think(): run_think() posts the
+    // think+prune job to the compute thread, which needs the exclusive lock. All
+    // chain mutations above are complete at this point.
+    lock.unlock();
+
     // Trigger think() after every share batch (p2pool: set_best_share after handle_shares).
     // p2pool calls set_best_share() after EVERY batch with new_count > 0 — no size gate.
     // think() scores heads and updates best_share + desired set for download_shares.
@@ -794,14 +816,18 @@ void NodeImpl::notify_local_share(const uint256& share_hash)
 {
     // p2pool: set_best_share() → think() synchronously on the reactor thread.
     // Use think() for ALL best_share decisions, as p2pool does.
-    if (share_hash.IsNull() || !m_tracker.chain.contains(share_hash))
+    if (share_hash.IsNull())
         return;
 
-    // Try inline verify — if think() holds the mutex, defer to next think() cycle.
-    // The share is already in the chain; think() will verify + score it.
+    // Both the chain.contains() read AND attempt_verify() run UNDER the tracker
+    // lock (mirrors LTC f445db8e). A bare m_tracker.chain.contains() here (the
+    // prior code) raced the compute-thread clean_tracker() exclusive prune freeing
+    // chain nodes -> SIGSEGV (kr1z1s LTC/DGB). try_to_lock keeps the IO thread
+    // non-blocking; if think()/clean holds the lock we skip the inline verify —
+    // the share is already in-chain and run_think() below will score it next cycle.
     {
         std::unique_lock lock(m_tracker_mutex, std::try_to_lock);
-        if (lock.owns_lock())
+        if (lock.owns_lock() && m_tracker.chain.contains(share_hash))
             m_tracker.attempt_verify(share_hash);
     }
 

src/impl/ltc/node.hpp

@@ -90,6 +90,11 @@ class NodeImpl : public pool::SharechainNode<ltc::Config, ltc::ShareChain, ltc::
     // compute thread (unsafe); the stuck cycle, if it ever returns, finds the
     // flag already false and its idempotent IO-phase drain runs harmlessly.
     static constexpr int THINK_WATCHDOG_SECONDS = 30;
+    // MAX_PENDING_ADDS caps the deferred queue so a stuck/slow think() cannot
+    // grow memory without bound — over the cap new batches are dropped with a
+    // LOG_WARNING backpressure message (peers re-advertise, so dropped shares
+    // are re-requested later).
+    static constexpr size_t MAX_PENDING_ADDS = 256;
     std::atomic<int64_t>  m_think_deadline_ns{0};
     std::atomic<uint64_t> m_think_generation{0};
     std::unique_ptr<boost::asio::steady_timer> m_watchdog_timer;

test/test_threading.cpp

@@ -61,8 +61,10 @@
 #include <chrono>
 #include <cstring>
 #include <future>
+#include <map>
 #include <memory>
 #include <set>
+#include <shared_mutex>
 #include <thread>
 
 namespace io = boost::asio;
@@ -495,4 +497,96 @@ TEST(VerifyShareThreading, Phase2SkipsNullHashShares)
         EXPECT_FALSE(share.hash.IsNull())
             << "Phase 2 emitted a share with null hash (index=" << share.index << ")";
     }
+}
+
+// ─── 8. Phase2HoldsLockAgainstComputePrune ───────────────────────────────────
+// Regression guard for the kr1z1s use-after-free / SIGSEGV race (issue #759).
+//
+// Models the exact hazard in NodeImpl: think()/clean_tracker() run on a SEPARATE
+// compute thread and take m_tracker_mutex EXCLUSIVELY to prune (drop_tails) —
+// FREEING chain nodes — while the IO thread runs processing_shares_phase2() /
+// notify_local_share() which mutate/read the same chain nodes.
+//
+// The FIXED discipline (btc/bch, back-ported to ltc): the IO-thread body must
+// hold the shared_mutex (try_to_lock; on !owns_lock defer) across every access
+// to the shared node container, and guard chain.contains() UNDER the lock.
+// The old ltc pattern released the lock early / read contains() bare, letting
+// the prune free a node mid-access -> UAF. Here the nodes are heap-allocated so
+// a discipline break is a genuine use-after-free (fails under ASan/TSan; may
+// also SIGSEGV in a plain build). With the correct discipline the run is clean.
+TEST(VerifyShareThreading, Phase2HoldsLockAgainstComputePrune)
+{
+    // A stand-in "chain": heap nodes keyed by id, guarded by a shared_mutex —
+    // the same primitive (std::shared_mutex m_tracker_mutex) the node uses.
+    struct Node {
+        int id;
+        std::atomic<uint64_t> touched{0};
+        explicit Node(int i) : id(i) {}   // atomic member => non-copyable/movable
+    };
+    std::map<int, std::unique_ptr<Node>> chain;
+    std::shared_mutex chain_mutex;
+
+    constexpr int MAX_ID = 512;
+    for (int i = 0; i < MAX_ID; ++i)
+        chain.emplace(i, std::make_unique<Node>(i));
+
+    std::atomic<bool> stop{false};
+    std::atomic<uint64_t> io_ops{0};
+    std::atomic<uint64_t> defers{0};
+
+    // COMPUTE thread: clean_tracker() analog. Exclusive lock -> free ~half the
+    // nodes (drop_tails), then repopulate. This is the writer that invalidates
+    // pointers the IO thread must never hold across the lock boundary.
+    std::thread compute([&] {
+        while (!stop.load(std::memory_order_relaxed)) {
+            {
+                std::unique_lock<std::shared_mutex> lock(chain_mutex);
+                for (int i = 0; i < MAX_ID; i += 2)
+                    chain.erase(i);          // free nodes (drop_tails)
+                for (int i = 0; i < MAX_ID; i += 2)
+                    chain.emplace(i, std::make_unique<Node>(i));
+            }
+            std::this_thread::yield();
+        }
+    });
+
+    // IO thread: processing_shares_phase2() + notify_local_share() analog.
+    // try_to_lock; ONLY touch the freed-able nodes while owns_lock() holds, and
+    // HOLD the lock across the whole mutation body (never cache a Node* past the
+    // unlock). notify's contains()+access are guarded under the same lock.
+    auto io_body = [&] {
+        for (int iter = 0; iter < 20000 && !stop.load(std::memory_order_relaxed); ++iter) {
+            // processing_shares_phase2: hold lock across the mutation body.
+            {
+                std::unique_lock<std::shared_mutex> lock(chain_mutex, std::try_to_lock);
+                if (!lock.owns_lock()) { defers.fetch_add(1); continue; }
+                for (auto& [id, node] : chain) {
+                    node->touched.fetch_add(1, std::memory_order_relaxed); // deref UNDER lock
+                    io_ops.fetch_add(1, std::memory_order_relaxed);
+                }
+            }
+            // notify_local_share: guarded contains() + access UNDER the lock.
+            {
+                int probe = iter % MAX_ID;
+                std::unique_lock<std::shared_mutex> lock(chain_mutex, std::try_to_lock);
+                if (lock.owns_lock()) {
+                    auto it = chain.find(probe);               // contains()
+                    if (it != chain.end())
+                        it->second->touched.fetch_add(1);      // deref UNDER lock
+                }
+            }
+        }
+    };
+
+    std::thread io1(io_body), io2(io_body);
+    io1.join(); io2.join();
+    stop.store(true);
+    compute.join();
+
+    // If we got here without a crash/ASan/TSan fault, the discipline held under
+    // concurrent prune. io_ops > 0 proves the IO body actually ran (didn't just
+    // defer forever), so the invariant was genuinely exercised.
+    EXPECT_GT(io_ops.load(), 0u)
+        << "IO body never acquired the lock — invariant not exercised";
+    SUCCEED() << "io_ops=" << io_ops.load() << " defers=" << defers.load();
 }