backport: bitcoin-core/gui#693: Fix segfault when shutdown during wallet open #6609
Conversation
…open 9a1d73f Fix segfault when shutdown during wallet open (John Moffett) Pull request description: Fixes dashpay#689 ## Summary If you open a wallet and send a shutdown signal during that process, you'll get a segfault when the wallet finishes opening. That's because the `WalletController` object gets deleted manually in bitcoin.cpp during shutdown, but copies of the pointer (and pointers to child objects) are dangling in various places and are accessed in queued events after the deletion. ## Details The issue in dashpay#689 is caused by the following sequence of events: 1. Wallet open modal dialog is shown and worker thread does the actual work. 2. Every 200ms, the main event loop checks to see if a shutdown has been requested, but only if a modal is not being shown. 3. Request a shutdown while the modal window is shown. 4. The wallet open process completes, the modal window is dismissed, and various `finish` signals are sent. 5. During handling of one of the `finish` signals, `qApp->processEvents()` is [called](https://github.com/bitcoin-core/gui/blob/e9262ea32a6e1d364fb7974844fadc36f931f8c6/src/qt/sendcoinsdialog.cpp#L603), which causes the main event loop to detect the shutdown (now that the modal window has been dismissed). The `WalletController` and all the `WalletModel`s are [deleted](https://github.com/bitcoin-core/gui/blob/65de8eeeca29e71378aa34602b287ab921b040e4/src/qt/bitcoin.cpp#L394-L401). 6. Control returns to the `finish` method, which eventually tries to send a [signal](https://github.com/bitcoin-core/gui/blob/e9262ea32a6e1d364fb7974844fadc36f931f8c6/src/qt/sendcoinsdialog.cpp#L167) from a wallet model, but it's been deleted already (and the signal is sent from a now-[dangling](https://github.com/bitcoin-core/gui/blob/d8bdee0fc889def7c5f5076da13db4fce0a3728a/src/qt/walletview.cpp#L65) pointer). The simplest fix for that is to change the `qApp->processEvents()` into a `QueuedConnection` call. (The `qApp->processEvents() was a [workaround](bitcoin#593 (comment)) to get the GUI to scroll to the last item in a list that just got added, and this is just a safer way of doing that). However, once that segfault is fixed, another segfault occurs due to some queued wallet events happening after the wallet controller object is deleted here: https://github.com/bitcoin-core/gui/blob/65de8eeeca29e71378aa34602b287ab921b040e4/src/qt/bitcoin.cpp#L394-L401 Since `m_wallet_controller` is a copy of that pointer in `bitcoingui.cpp`, it's now dangling and `if(null)` checks won't work correctly. For instance, this line: https://github.com/bitcoin-core/gui/blob/65de8eeeca29e71378aa34602b287ab921b040e4/src/qt/bitcoingui.cpp#L413 sets up a `QueuedConnection` to `setCurrentWallet`, but by the time control reaches that method (one event cycle after shutdown deleted `m_wallet_controller` in `bitcoin.cpp`), the underlying objects have been destroyed (but the pointers are still dangling). Ideally, we'd use a `QPointer` or `std::shared_ptr / std::weak_ptr`s for these, but the changes would be more involved. This is a minimal fix for the issues. Just set `m_wallet_controller` to `nullptr` in `bitcoingui.cpp`, check its value in a couple places, and avoid a use of `qApp->processEvents`. ACKs for top commit: hebasto: ACK 9a1d73f, I have reviewed the code and it looks OK. furszy: ACK bitcoin-core/gui@9a1d73f Tree-SHA512: a1b94676eb2fcb7606e68fab443b1565b4122aab93c35382b561842a049f4b43fecc459535370d67a64d6ebc4bcec0ebcda981fff633ebd41bdba6f7093ea540
WalkthroughThe pull request implements updates across wallet management and user interface components. In the wallet GUI logic, a new connection to the wallet controller’s destruction signal is added so that the internal pointer is set to nullptr upon controller destruction, and checks in wallet-related methods now verify that the wallet controller is valid before proceeding. In the send coins dialog, the vertical scrollbar update after adding a new entry is moved from an immediate event processing call to an asynchronous update using QMetaObject::invokeMethod, addressing timing and positioning issues. Additionally, the test suite for wallet transactions now includes an explicit event processing call after submitting transactions to ensure the transaction table model accurately reflects changes. ✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/qt/bitcoingui.cpp(3 hunks)src/qt/sendcoinsdialog.cpp(1 hunks)src/qt/test/wallettests.cpp(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: linux64-build / Build source
- GitHub Check: linux64_sqlite-build / Build source
- GitHub Check: linux64_ubsan-build / Build source
- GitHub Check: linux64_nowallet-build / Build source
- GitHub Check: x86_64-pc-linux-gnu_multiprocess / Build depends
🔇 Additional comments (4)
src/qt/test/wallettests.cpp (1)
166-167: Good fix for event processing timingAdding this call to process pending events ensures that the transaction table model is fully updated before assertions are checked. This addresses a potential race condition in the tests and is a good practice when working with Qt's event-driven architecture, especially when dealing with model updates that happen via QueuedConnection.
src/qt/sendcoinsdialog.cpp (1)
607-615: Excellent improvement for scrollbar updatesReplacing a direct event processing call with a queued method invocation is much safer. This approach:
- Prevents potential reentrancy issues associated with
processEvents()- Correctly handles the timing of scrollbar updates after widget layout calculations
- Isolates UI updates from the main control flow, reducing the chance of segfaults during interrupt scenarios like application shutdown
The detailed comment also provides clear rationale for the implementation choice, which is helpful for future maintenance.
src/qt/bitcoingui.cpp (2)
918-918: Important null check for wallet controllerThis additional check prevents potential crashes if wallet operations are attempted after the wallet controller has been destroyed. It complements the connection to the
destroyedsignal by adding defensive programming at the usage sites.
966-966: Important null check for wallet controllerSimilar to the previous change, this null check in
setCurrentWalletprevents accessing an invalid wallet controller. This is a good defensive programming practice that helps prevent crashes during application shutdown.
| connect(wallet_controller, &WalletController::destroyed, this, [this] { | ||
| // wallet_controller gets destroyed manually, but it leaves our member copy dangling | ||
| m_wallet_controller = nullptr; | ||
| }); |
There was a problem hiding this comment.
Critical fix for segfault during shutdown
Adding this connection to the destroyed signal ensures that the m_wallet_controller member is set to nullptr when the controller is destroyed. This prevents the dangerous scenario of accessing a dangling pointer, which was the likely cause of the segfault when shutting down during wallet opening.
This is an essential memory safety improvement that properly handles object lifecycle events.
4 participants
Issue being fixed or feature implemented
Pls see the original PR description + fixes #6569 (comment)
What was done?
backport bitcoin-core/gui#693
How Has This Been Tested?
Run
./src/qt/dash-qtBreaking Changes
n/a
Checklist: