mirror of
https://github.com/bitcoin/bitcoin.git
synced 2026-05-22 11:55:22 +02:00
a7bea426b4
14547eb489kernel: guard btck::Handle move-assignment against self-move (Thomas) Pull request description: 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. ACKs for top commit: sedited: Thanks, ACK14547eb489alexanderwiederin: ACK14547eb489yuvicc: lgtm ACK14547eb489Tree-SHA512: 334f5ad3045d5f9b06cc0dd096bc911a992773c59cc469765c2082975a9f7a90f2349b9ad94b4b5127290de1fab2f5424a621384c1b4bb9152de99f5da8ed6aa