fix: multiple asan's finding - #7513
Conversation
test/lcg.h:28 state = state * 6364136223846793005 + 1442695040888963407
an MMIX linear congruential generator; and it's meant to be
unsigned overflowed
|
✅ Final review complete — no blockers (commit 11108da) |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (17)
🚧 Files skipped from review as they are similar to previous changes (15)
WalkthroughThe change migrates ChainLock, InstantSend, and quorum components from direct Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/index/addressindex.cpp`:
- Line 327: Update the input history removal near CustomAppend to pass the same
transaction position variable i used by the output removal, replacing i + 1.
Preserve the existing rewind behavior and other arguments.
In `@src/instantsend/signing.cpp`:
- Around line 391-393: Protect both quorum-selection calls with ::cs_main for
their entire SelectQuorumForSigning invocation. In src/instantsend/signing.cpp
lines 391-393, wrap the call using m_chainman.ActiveChain() in
WITH_LOCK(::cs_main, ...). In src/instantsend/net_instantsend.cpp lines 148-150,
wrap the rotating-quorum call similarly and replace
m_chainman.ActiveChainstate().m_chain with m_chainman.ActiveChain().
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 1533e15c-c7c1-4648-8b54-c274e29b911b
📒 Files selected for processing (18)
src/active/context.cppsrc/chainlock/signing.cppsrc/chainlock/signing.hsrc/index/addressindex.cppsrc/index/addressindex_types.hsrc/init.cppsrc/instantsend/net_instantsend.cppsrc/instantsend/net_instantsend.hsrc/instantsend/signing.cppsrc/instantsend/signing.hsrc/llmq/blockprocessor.cppsrc/llmq/blockprocessor.hsrc/llmq/context.cppsrc/llmq/snapshot.cppsrc/llmq/utils.cppsrc/rpc/evo.cppsrc/rpc/governance.cpptest/sanitizer_suppressions/ubsan
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4510f28215
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| // Note: This function can be called for new blocks | ||
| assert(nHeight <= m_chainstate.m_chain.Height() + 1); | ||
| const auto *const pindex = m_chainstate.m_chain.Height() < nHeight ? m_chainstate.m_chain.Tip() : m_chainstate.m_chain.Tip()->GetAncestor(nHeight); | ||
| const CChain& active_chain{m_chainman.ActiveChain()}; |
There was a problem hiding this comment.
When an assumeutxo snapshot is active, background IBD still connects blocks through the shared CSpecialTxProcessor, but this line now bases GetNumCommitmentsRequired() on chainman.ActiveChain() (the snapshot chain) instead of the chainstate whose block is being validated. Because IsMiningPhase() and GetQuorumBlockHash() depend on that chain at nHeight, background validation can require the wrong quorum commitments or reject valid ones once the active chain differs from the background chain; the block processor needs the relevant chainstate/chain for the block being connected rather than the global active chain.
AGENTS.md reference: AGENTS.md:L164-L165
Useful? React with 👍 / 👎.
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
Both reported blockers are confirmed against the exact PR head. The address-index loop rewrite leaves spending deltas behind during rewinds, while the InstantSend change calls a lock-annotated active-chain accessor without holding cs_main, causing a thread-safety warning that becomes a CI build error under --enable-werror.
Validated blockers were found in the Codex precheck. Sonnet is deferred until a fresh Codex revalidation clears the blocker gate.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— dash-core-commit-history (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 2 blocking
1 additional finding(s) omitted (not in diff).
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `src/index/addressindex.cpp`:
- [BLOCKING] src/index/addressindex.cpp:353-356: Reverse loop now deletes spending entries with the wrong transaction position
The rewritten loop makes `i` the actual block transaction position: it processes `block.vtx[i]`, and the receiving key correctly uses position `i`. The spending key still uses `i + 1`, so `RewindBatch()` attempts to erase a key that was never written and leaves the real negative spending delta in the address index. Invalidating a block that spends an indexed address therefore produces an incorrect `getaddressbalance` and `getaddressdeltas` result; the reorg assertions in `feature_addressindex.py` exercise this exact case.
In `src/instantsend/signing.cpp`:
- [BLOCKING] src/instantsend/signing.cpp:393: ActiveChain is called without its required cs_main lock
`ChainstateManager::ActiveChain()` is annotated `EXCLUSIVE_LOCKS_REQUIRED(GetMutex())`, but `TrySignInstantSendLock()` is called from paths that do not hold `cs_main`. Clang consequently emits `-Wthread-safety` for this new call, and source CI enables both that warning and `--enable-werror`. The selector's internal lock is acquired only after function arguments have been evaluated, so it cannot protect this accessor call. Holding `cs_main` across selection also prevents `ResetChainstates()` or snapshot activation from changing the active chainstate while its chain reference is being consumed, which is required for the lifetime fix implemented by this commit.
There was a problem hiding this comment.
💡 Codex Review
dash/src/index/addressindex.cpp
Line 355 in b82a4fc
Use original tx position when erasing spend entries
When -addressindex rewinds a block containing any non-coinbase transaction inputs, this reverse loop now makes i the actual block.vtx position, but the spend-history erase key still uses i + 1. CustomAppend() wrote those spend entries with the transaction position only once, so this erases a key that was never written and leaves the old negative spend delta behind after a reorg, corrupting address history and balance-style RPC results until the index is rebuilt.
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
m_block_height is int32_t but the on-disk field is a big-endian uint32, so Unserialize narrowed implicitly and -fsanitize=integer reported every read whose top bit was set: implicit conversion from type 'uint32_t' of value 3400325678 to type 'int32_t' changed the value to -894641618
The reverse-iteration idiom `for (size_t idx = n; idx-- > 0;)` decrements on the final check too, so idx wraps to SIZE_MAX every time the loop ends, not just when prev_cycles is empty. -fsanitize=integer reports it on each exit: llmq/utils.cpp:519:54: runtime error: unsigned integer overflow: 0 - 1 cannot be represented in type 'size_t' index/addressindex.cpp:309 undoing a block's transactions in reverse llmq/snapshot.cpp:194 building the mnlistdiff chain in reverse src/qt/proposalmodel.cpp:408 uses the same shape but counts with int, where 0 - 1 is representable, so it is left alone.
CQuorumBlockProcessor stored `Chainstate& m_chainstate`, bound once when
LLMQContext is built in node::DashChainstateSetup(). A reference cannot be
rebound, but the object it names does not live as long as LLMQContext:
ChainstateManager::ResetChainstates() destroys the chainstate and
InitializeChainstate() allocates a new one. Everything the block processor
does afterwards reads freed memory.
AddressSanitizer catches it in validation_chainstate_tests/chainstate_update_tip:
ERROR: AddressSanitizer: heap-use-after-free
READ of size 8 at offset 136 inside a 288-byte region
llmq::CQuorumBlockProcessor::ProcessBlock llmq/blockprocessor.cpp:171
CSpecialTxProcessor::ProcessSpecialTxsInBlock evo/specialtxman.cpp:707
Chainstate::ConnectBlock validation.cpp:2320
freed by
ChainstateManager::ResetChainstates validation.cpp:5807
Offset 136 is Chainstate::m_chainman, which is what line 171 dereferences.
Not reachable on a running node today: ResetChainstates() and
ActivateSnapshot() have no callers outside tests, and Dash has no
loadtxoutset RPC, so no snapshot is ever activated. The reference is still
wrong on its own terms though, because ActivateSnapshot() repoints
m_active_chainstate without freeing the old chainstate, which would leave
the block processor validating commitments against the background chain.
Better to fix it now than to wait for assumeutxo to be wired up.
Holding the manager and asking for the active chainstate per call removes
the lifetime question entirely: ChainstateManager is owned by NodeContext
and outlives LLMQContext. It also matches the immediate caller,
CSpecialTxProcessor, which already keeps `const ChainstateManager&`, and
shortens the nine sites that spelled out m_chainstate.m_chainman.
…te reference Same defect as CQuorumBlockProcessor, in the three remaining long-lived objects that captured a Chainstate reference at construction: chainlock::ChainLockSigner active/context.cpp:45 instantsend::InstantSendSigner active/context.cpp:47 NetInstantSend init.cpp:2179 All three were handed chainman.ActiveChainstate() once and stored the result as a reference. ChainstateManager::ResetChainstates() destroys that object, so any use afterwards touches freed memory, and ActivateSnapshot() repoints the active chainstate without freeing the old one, which would leave these three working against the background chain. None is reachable from a test that resets chainstates, so unlike CQuorumBlockProcessor there is no sanitizer failure to point at. They are the same bug regardless, and leaving three of four instances in place would just invite the next one. NetInstantSend needs a mutable chainstate for InvalidateBlock, ResetBlockFailureFlags and ActivateBestChain. That still works through a const manager, because ChainstateManager::ActiveChainstate() is a const method returning a non-const reference. The LookupBlockIndex at net_instantsend.cpp:604 goes through ActiveChainstate().m_blockman rather than the manager's own m_blockman, because the const manager would select the const overload and the result has to stay writable, as the comment above it already noted. SelectQuorumForSigning keeps using ActiveChainstate().m_chain instead of ActiveChain(); the latter is annotated EXCLUSIVE_LOCKS_REQUIRED(cs_main) and that call site does not hold it, so the direct member access preserves the existing locking exactly.
Four RPC handlers were built by a helper that takes a bool, and captured
that bool with [&]. The lambda is stored in the returned RPCHelpMan and
outlives the helper, so by the time a request arrives the parameter's
stack slot is long gone:
ERROR: AddressSanitizer: stack-use-after-return
READ of size 1 ... thread T20 (d-httpworker.2)
protx_register_fund_wrapper(bool)::$_0::operator() rpc/evo.cpp:528
RPCHelpMan::HandleRequest rpc/util.cpp:530
CRPCTable::execute rpc/server.cpp:516
HTTPReq_JSONRPC httprpc.cpp:242
Affected:
rpc/evo.cpp:526 protx register_fund / register_fund_legacy
rpc/evo.cpp:577 protx register / register_legacy
rpc/evo.cpp:629 protx register_prepare / register_prepare_legacy
rpc/governance.cpp:689 gobject list / gobject diff
The read decides whether the deprecated legacy BLS scheme is being
requested, and in gobject's case whether to return a diff, so a stale
value silently picks the wrong behaviour rather than failing loudly.
Capturing by value is enough; none of the four bodies uses anything else
from the enclosing scope.
protx_update_registrar_wrapper takes the same kind of parameter but
derives the flag from self.m_name inside the lambda, so it was never
affected. masternodelist_helper and rpc/node.cpp's echo() use their
parameter only while building the RPCHelpMan, before the lambda exists.
This is what made 33 functional tests fail under ASan: every masternode,
LLMQ and governance test registers a masternode, so all of them hit
protx register_fund and died in the same place.
Holding ChainstateManager rather than a Chainstate reference meant reaching
the chain through ChainstateManager::ActiveChain(), which is annotated
EXCLUSIVE_LOCKS_REQUIRED(GetMutex()) with GetMutex() returning ::cs_main.
Neither call site holds it, and the fuzz job builds with -Wthread-safety
-Werror, so this is a hard error there:
instantsend/signing.cpp:393:90: error: calling function 'ActiveChain'
requires holding mutex 'cs_main' exclusively
[-Werror,-Wthread-safety-analysis]
Use the idiom the other two SelectQuorumForSigning callers already use, in
llmq/ehf_signals.cpp and llmq/signing_shares.cpp: take cs_main just long
enough to obtain the reference, then call with it held no longer. In
BuildVerificationBatch the reference is hoisted out of the loop over pending
locks so the lock is taken once rather than per entry.
net_instantsend.cpp reached the chain as ActiveChainstate().m_chain, which
compiles because the member carries no annotation. That is the same
unlocked access, only hidden from the analyser, so it is converted too.
This deliberately does not change what is serialised. The traversal inside
SelectQuorumForSigning still runs without cs_main, exactly as it did before
the refactor, when both sites read m_chainstate.m_chain directly. Holding
cs_main across the whole call would be a real behaviour change: it takes
cs_main on the InstantSend signing path, which is where lock order
inversions live, and belongs in its own change with a tsan run behind it
rather than in a refactor.
b82a4fc to
11108da
Compare
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Codex + Sonnet
Both carried-forward blockers are fixed at the current head: the address-index rewind keys now use the correct transaction position, and ActiveChain() is evaluated while holding cs_main. No current-head correctness blockers remain. The latest stack rewrite leaves two commit-history suggestions: fold the active-chain locking correction into the refactor that requires it, and move the address-index position correction into the loop-rewrite commit so every intermediate commit remains coherent and correct.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (completed),gpt-5.6-sol— dash-core-commit-history (completed) - Verifier:
gpt-5.6-sol— final-verifier (fallback) - Sonnet reviewers:
claude-sonnet-5— general (completed),claude-sonnet-5— dash-core-commit-history (failed),claude-sonnet-5— dash-core-commit-history (failed),claude-sonnet-5— dash-core-commit-history (completed)
🟡 2 suggestion(s)
2 additional finding(s) omitted (not in diff).
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `<commit:fb7fa62f618>`:
- [SUGGESTION] <commit:fb7fa62f618>:1: Fold the active-chain lock fix into the ChainstateManager refactor
Commit `fb7fa62f618` changes `instantsend/signing.cpp` from direct `m_chainstate.m_chain` access to the lock-annotated `m_chainman.ActiveChain()` without holding `cs_main`, even though its commit message says this call site intentionally avoids `ActiveChain()` for precisely that reason. At this intermediate commit, builds using `-Wthread-safety -Werror` fail; `11108da3501` corrects the access only after an unrelated RPC commit. Fold `11108da3501` into `fb7fa62f618` and update the earlier commit body so the refactor is independently buildable and its explanation matches its diff.
In `<commit:a3a259e6a6f>`:
- [SUGGESTION] <commit:a3a259e6a6f>:1: Move the address-index position correction into the loop-rewrite commit
Commit `a3a259e6a6f` is scoped to explicit block-height narrowing, but it also changes only the spending key's transaction position from `i + 1` to `i`. In that commit's resulting tree, the old loop still processes `block.vtx[i + 1]` and the receiving key still uses `i + 1`, so this unrelated hunk temporarily makes spending-key removal incorrect. Commit `533db584466` later rewrites the loop to process `block.vtx[i]` and changes the receiving key to `i`, making the earlier hunk correct again. Move the spending-key hunk into `533db584466`, where it is part of the same indexing rewrite and every intermediate commit remains correct.
3 participants
Issue being fixed or feature implemented
Several issues has been found by #7503
This PR fixes multiple issues which help to enable these sanitizers beside
-undefined:detect_leaks=1:detect_stack_use_after_return=1:check_initialization_order=1:strict_init_order=1What was done?
How Has This Been Tested?
Tested locally by running changes from this PR + 7503
Breaking Changes
N/A
Checklist: