transaction, eckey: fix use-after-free on registry idx collision in add_*_locked - #30
Merged
edtubbs merged 2 commits intoJul 21, 2026
Conversation
…saction_locked The stateless refactor mints working_transaction ids as HASH_COUNT()+1, so ids recycle after any removal. When a fresh start_transaction_ts() lands on an id still held by a find_transaction_ts() caller, add_transaction_locked() resolved the collision with HASH_REPLACE_INT and then unconditionally dogecoin_free()'d the displaced entry. That raw free bypassed the retain/deferred-delete model introduced for find-then-use: it ignored refcount/pending_delete, freeing the wrapper out from under a legitimate holder (use-after-free, and a double free on the holder's paired release_transaction_ts()), and it freed the wrapper without freeing tx->transaction (leak). Route the displaced entry through the same deferred-delete path as an explicit removal by extracting dispose_unlinked_transaction_locked(), used by both the collision path and remove_transaction_locked(). An entry that is still referenced is marked pending_delete and freed by the last release; an unreferenced one is freed immediately, transaction included. Add test_transaction_ts_replace_retained(), a deterministic single-threaded regression that reproduces the collision against a retained entry. It traps the use-after-free under ASan/TSan before this change and passes after.
The eckey registry has the same collision bug just fixed for transactions.
Key ids are minted as HASH_COUNT()+1, so they recycle after a removal; a fresh
start_key_ts() can then collide with an id still held by a find_eckey_ts()
caller. add_eckey_locked() resolved the collision with HASH_REPLACE_INT and then
unconditionally dogecoin_free()'d the displaced key.
That raw free was doubly wrong here:
- it ignored the eckey_lifetime side table (refcount/pending_delete) that
find_eckey_ts() populates, freeing the key out from under a legitimate
holder -> use-after-free, plus a stranded/dangling lifetime record; and
- it bypassed destroy_eckey_locked(), so the displaced key's private-key and
public-key material was never cleansed -- left in freed heap (CWE-226).
Extract dispose_unlinked_eckey_locked() -- the lifetime-aware defer-or-destroy
logic that remove_eckey_locked() already used -- and route the collision path
through it too. A still-referenced key is marked pending_delete and cleansed+freed
by the last release; an unreferenced one is cleansed+freed immediately.
Add test_eckey_ts_replace_retained(), a deterministic single-threaded regression
mirroring test_transaction_ts_replace_retained(): it collides a fresh mint against
a retained key and asserts the retained key stays valid and unmodified. Traps the
use-after-free under ASan/TSan before this change; passes after.
xanimo
changed the title
edtubbs
approved these changes
Jul 21, 2026
edtubbs
left a comment
edtubbs
left a comment
Owner
There was a problem hiding this comment.
ACK, good catch from the sanitizer. Regression confirms the use-after-free fix now goes through the deferred-delete path instead of raw-freeing a still-retained entry.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Fixes the same use-after-free in both registry
add_*_locked()paths on the#330stateless refactor branch — first the transaction registry, then its eckey twin found by auditing for the same shape.Registry ids are minted as
HASH_COUNT() + 1, so ids recycle after any removal. When a freshstart_transaction_ts()/start_key_ts()collides with an id still held by afind_*_ts()caller, the collision path didHASH_REPLACE_INTand then unconditionallydogecoin_free()'d the displaced entry — bypassing the retain / deferred-delete model that#330itself introduced (1b144b0bfor transactions,b6567a95for eckey).Consequences (both registries):
release_*_ts();The eckey case is additionally a CWE-226 key-material leak: the raw free bypassed
destroy_eckey_locked(), so the displaced key's private/public bytes were never cleansed — left in freed heap.Both are reachable single-threaded — no concurrency required.
Fix
Per registry, extract a lifetime-aware
dispose_unlinked_*_locked()helper (the defer-or-free logicremove_*_locked()already used) and route both the collision path and the explicit-removal path through it. A still-referenced entry is markedpending_deleteand freed by the last release (cleansed, for eckey); an unreferenced one is freed immediately.src/transaction.c—dispose_unlinked_transaction_locked()src/eckey.c—dispose_unlinked_eckey_locked()Tests
Two deterministic single-threaded regressions, each colliding a fresh mint against a retained entry:
test_transaction_ts_replace_retained()test_eckey_ts_replace_retained()(also asserts the retained key's WIF is unmodified)Verified for both:
add_*_lockedbefore → clean after.Scope & related PRs (cross-checked)
CONFLICTING, and built on a pre-b6567a95build, ci, docs, include, spv, src, test: stateless thread-safe refactor dogecoinfoundation/libdogecoin#330 base (no eckey lifetime side-table), so it lands after build, ci, docs, include, spv, src, test: stateless thread-safe refactor dogecoinfoundation/libdogecoin#330 and will need reconciliation. This PR fixes build, ci, docs, include, spv, src, test: stateless thread-safe refactor dogecoinfoundation/libdogecoin#330 at merge time using the lifetime-aware disposal that build, ci, docs, include, spv, src, test: stateless thread-safe refactor dogecoinfoundation/libdogecoin#330-head actually has. The two are complementary: eckey: mint never-reused key ids; stop evicting (and leaking) live keys dogecoinfoundation/libdogecoin#336's monotonic ids are defense-in-depth on top of a correct disposal path — on rebase, keep the lifetime-aware dispose rather than reverting to raw handling.Separate pre-existing issue (not addressed here)
A concurrent eckey harness aborts on
assert(secp256k1_ctx)insrc/ecc.c:85during parallelstart_key_ts()— a global-ECC-context race, reproduces identically on stock dogecoinfoundation#330 without this change. Flagging it as its own follow-up; out of scope for this PR.