Skip to content

kernel: guard btck::Handle move-assignment against self-move#35143

Merged
fanquake merged 1 commit intobitcoin:masterfrom
thomasbuilds:fix-btck-handle-self-move-assign
Apr 26, 2026
Merged

kernel: guard btck::Handle move-assignment against self-move#35143
fanquake merged 1 commit intobitcoin:masterfrom
thomasbuilds:fix-btck-handle-self-move-assign

Conversation

@thomasbuilds
Copy link
Copy Markdown
Contributor

The move-assignment operator for btck::Handle<> in src/kernel/bitcoinkernel_wrapper.h unconditionally called DestroyFunc(m_ptr) before reading the source pointer. On a self-move (h = std::move(h)), this destroys the held resource and then restores the now-dangling pointer via std::exchange(other.m_ptr, nullptr) (since &other == this), which leaves m_ptr pointing at freed memory. The destructor then calls DestroyFunc again on it, resulting in a double-free.

Trace of h = std::move(h) with the old code, where h.m_ptr == P:

  1. DestroyFunc(m_ptr) -> delete P. this->m_ptr still literally stores the now-dangling value P.
  2. std::exchange(other.m_ptr, nullptr) — because &other == this, this reads the dangling P back, writes nullptr to m_ptr, and returns P.
  3. m_ptr = P restores the dangling pointer.
  4. ~Handle() later runs DestroyFunc(P) -> double free, UB.

The copy-assignment operator already guards against self-assignment with if (this != &other); the move variant should be symmetric. The standard library requires moved-from objects to be in a valid (at minimum, safely destructible) state, which the previous implementation violated when source and destination alias.

Handle<> is the base class of 16 public types in the kernel C++ API wrapper (Transaction, Block, BlockHeader, ChainParams, Context, Coin, BlockValidationState, ScriptPubkey, TransactionOutput, Txid, OutPoint, TransactionInput, PrecomputedTransactionData, BlockHash, BlockSpentOutputs, TransactionSpentOutputs), so self-move can arise from generic algorithms operating on containers of these types (std::sort, std::remove, erase-remove idioms, etc.).

Fix: mirror the copy-assignment pattern by guarding the move-assignment body with if (this != &other), making a self-move a no-op.

Also extend CheckHandle in src/test/kernel/test_kernel.cpp to exercise self-move-assignment for every Handle-derived type, checking that the stored pointer and the serialized bytes (where applicable) are unchanged.

The move-assignment operator for btck::Handle<> unconditionally called
DestroyFunc(m_ptr) before reading the source pointer. On a self-move
(h = std::move(h)), this destroyed the held resource and then reassigned
the now-dangling pointer back to m_ptr via std::exchange, leading to a
double-free when the object is later destroyed.

Mirror the existing self-check in the copy-assignment operator by
guarding the move-assignment with 'if (this != &other)' so a self-move
becomes a no-op, leaving the object in a valid state as required by the
standard library.

Handle<> is the base of 16 public types in the kernel C++ API wrapper
(Transaction, Block, BlockHeader, ChainParams, Context, Coin,
BlockValidationState, ScriptPubkey, TransactionOutput, Txid, OutPoint,
TransactionInput, PrecomputedTransactionData, BlockHash,
BlockSpentOutputs, TransactionSpentOutputs), so self-move can arise
from generic algorithms operating on containers of these types.

Extend CheckHandle in test_kernel to cover self-move-assignment for
every Handle-derived type.
@DrahtBot
Copy link
Copy Markdown
Contributor

DrahtBot commented Apr 23, 2026

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Reviews

See the guideline for information on the review process.

Type Reviewers
ACK sedited, alexanderwiederin, yuvicc

If your review is incorrectly listed, please copy-paste <!--meta-tag:bot-skip--> into the comment that the bot should ignore.

Copy link
Copy Markdown
Contributor

@sedited sedited left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, ACK 14547eb

@github-project-automation github-project-automation Bot moved this to Architecture & Performance in The libbitcoinkernel Project Apr 23, 2026
@sedited sedited moved this from Architecture & Performance to API Development in The libbitcoinkernel Project Apr 23, 2026
Copy link
Copy Markdown
Contributor

@alexanderwiederin alexanderwiederin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 14547eb

Fixes double-free on self-move assignment with the if (this != &other) guard already present in the copy-assignment operator.

Copy link
Copy Markdown
Contributor

@yuvicc yuvicc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm ACK 14547eb

@fanquake fanquake merged commit a7bea42 into bitcoin:master Apr 26, 2026
27 checks passed
@github-project-automation github-project-automation Bot moved this from API Development to Done or Closed or Rethinking in The libbitcoinkernel Project Apr 26, 2026
@thomasbuilds thomasbuilds deleted the fix-btck-handle-self-move-assign branch April 27, 2026 06:25
sedited pushed a commit to sedited/rust-bitcoinkernel that referenced this pull request Apr 30, 2026
…e8612d6

fb0e8612d6 Merge bitcoin/bitcoin#35175: multi_index: fix compilation failure with boost >= 1.91
cef58341a0 Merge bitcoin/bitcoin#32876: refactor: use options struct for signing and PSBT operations
0bc9d354df multi_index: fix compilation failure with boost >= 1.91
eab72d14d7 refactor: use SignOptions for MutableTransactionSignatureCreator
5ed41752c5 refactor: use SignOptions for SignTransaction
dc4a5d1270 refactor: use PSBTFillOptions for filling and signing
8592152186 Merge bitcoin/bitcoin#34911: rpc, mempool: -deprecatedrpc fullrbf and bip125-replaceable from mempool RPCs
ad3f73862b Merge bitcoin/bitcoin#35149: doc: clarify clang-tidy in developer notes
f40da7afc0 Merge bitcoin/bitcoin#35153: doc: update llvm based coverage example
a7bea426b4 Merge bitcoin/bitcoin#35143: kernel: guard btck::Handle move-assignment against self-move
ef21e29298 doc: update llvm based coverage example
a1e534bbf0 doc: clarify clang-tidy in developer notes
8deed0df06 doc: add release notes for PR 34911
1a85ca1dff rpc, mempool: rpcdeprecate `bip125-replaceable` key in mempool RPCs reponses
f89d18c3b1 rpc, mempool: rpcdeprecate `fullrbf` key in getmempoolinfo RPC response
14547eb489 kernel: guard btck::Handle move-assignment against self-move

git-subtree-dir: libbitcoinkernel-sys/bitcoin
git-subtree-split: fb0e8612d6f74071af77b3f27d915da69e0a726b
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Status: Done or Closed or Rethinking

Development

Successfully merging this pull request may close these issues.

6 participants