diff --git a/src/impl/ltc/node.cpp b/src/impl/ltc/node.cpp index 5c59fac28..11e83d440 100644 --- a/src/impl/ltc/node.cpp +++ b/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(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(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 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); } diff --git a/src/impl/ltc/node.hpp b/src/impl/ltc/node.hpp index 64e2bfd21..1a7d1e392 100644 --- a/src/impl/ltc/node.hpp +++ b/src/impl/ltc/node.hpp @@ -90,6 +90,11 @@ class NodeImpl : public pool::SharechainNode m_think_deadline_ns{0}; std::atomic m_think_generation{0}; std::unique_ptr m_watchdog_timer; diff --git a/test/test_threading.cpp b/test/test_threading.cpp index 14606352c..6d93629e6 100644 --- a/test/test_threading.cpp +++ b/test/test_threading.cpp @@ -61,8 +61,10 @@ #include #include #include +#include #include #include +#include #include 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 touched{0}; + explicit Node(int i) : id(i) {} // atomic member => non-copyable/movable + }; + std::map> 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(i)); + + std::atomic stop{false}; + std::atomic io_ops{0}; + std::atomic 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 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(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 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 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(); } \ No newline at end of file