wallet: Avoid a segfault in migratewallet failure cleanup #26594
Conversation
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
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. ConflictsReviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first. |
120d8ca to
91c935a
Compare
|
tACK 91c935a I think this should be backported to 24.x with a better error message saying that encrypted wallets will be supported in the future. |
stickies-v
reviewed
Nov 29, 2022
There was a problem hiding this comment.
Approach ACK 91c935a - not very familiar with the wallet migration process but this looks like an appropriate bug fix approach to me.
shaavan
reviewed
Nov 29, 2022
5bb48c9 to
7d7d02f
Compare
I've added a commit for an explicit error message saying so. |
If migratewallet fails, we do a cleanup which removes the watchonly and solvables wallets if they were created. However, if they were not, their pointers are nullptr and we don't check for that, which causes a segfault during the cleanup. So check that they aren't nullptr before cleaning them up.
Due to an oversight, we cannot currently migrate encrypted wallets, regardless of whether they are unlocked. Migrating such wallets will trigger an error, and result in the cleanup being run. This conveniently allows us to check some parts of the cleanup code.
d09e946 to
0c340e9
Compare
luke-jr
approved these changes
Nov 30, 2022
shaavan
reviewed
Nov 30, 2022
0c340e9 to
5e65a21
Compare
furszy
approved these changes
Nov 30, 2022
|
reACK 5e65a21 |
fanquake
pushed a commit
to fanquake/bitcoin
that referenced
this pull request
Dec 1, 2022
If migratewallet fails, we do a cleanup which removes the watchonly and solvables wallets if they were created. However, if they were not, their pointers are nullptr and we don't check for that, which causes a segfault during the cleanup. So check that they aren't nullptr before cleaning them up. Github-Pull: bitcoin#26594 Rebased-From: 86ef7b3
fanquake
pushed a commit
to fanquake/bitcoin
that referenced
this pull request
Dec 1, 2022
Due to an oversight, we cannot currently migrate encrypted wallets, regardless of whether they are unlocked. Migrating such wallets will trigger an error, and result in the cleanup being run. This conveniently allows us to check some parts of the cleanup code. Github-Pull: bitcoin#26594 Rebased-From: 88afc73
fanquake
pushed a commit
to fanquake/bitcoin
that referenced
this pull request
Dec 1, 2022
Github-Pull: bitcoin#26594 Rebased-From: 5e65a21
|
Backported in #26616. |
sidhujag
pushed a commit
to syscoin/syscoin
that referenced
this pull request
Dec 1, 2022
…e cleanup 5e65a21 wallet: Explicitly say migratewallet on encrypted wallets is unsupported (Andrew Chow) 88afc73 tests: Test for migrating encrypted wallets (Andrew Chow) 86ef7b3 wallet: Avoid null pointer deref when cleaning up migratewallet (Andrew Chow) Pull request description: When `migratewallet` fails, we do an automatic cleanup in order to reset everything so that the user does not experience any interruptions. However, this apparently has a segfault in it, caused by the the pointers to the watchonly and solvables wallets being nullptr. If those wallets are not created (either not needed, or failed early on), we will accidentally attempt to dereference these nullptrs, which causes a segfault. This failure can be easily reached by trying to migrate an encrypted wallet. Currently, we can't migrate encrypted wallets because of how we unload wallets before migrating, and therefore forget the encryption key if the wallet was unlocked. So any encrypted wallets will fail, entering the cleanup, and because watchonly and solvables wallets don't exist yet, the segfault is reached. This PR fixes this by not putting those nullptrs in a place that we will end up dereferencing them later. It also adds a test that uses the encrypted wallet issue. ACKs for top commit: S3RK: reACK 5e65a21 stickies-v: ACK [5e65a21](bitcoin@5e65a21) furszy: diff ACK 5e65a21 Tree-SHA512: f75643797220d4232ad3ab8cb4b46d0f3667f00486e910ca748c9b6d174d446968f1ec4dd7f907da1be9566088849da7edcd8cd8f12de671c3241b513deb8e80
maflcko
pushed a commit
that referenced
this pull request
Dec 6, 2022
8b726bf test: Coin Selection, duplicated preset inputs selection (furszy) 9d73176 test: wallet, coverage for CoinsResult::Erase function (furszy) 195f0df wallet: bugfix, 'CoinsResult::Erase' is erasing only one output of the set (furszy) e5d097b [test] Add p2p_tx_privacy.py (dergoegge) c842670 [net processing] Assume that TxRelay::m_tx_inventory_to_send is empty pre-verack (dergoegge) e15b306 [net processing] Ensure transaction announcements are only queued for fully connected peers (dergoegge) 95fded1 wallet: Explicitly say migratewallet on encrypted wallets is unsupported (Andrew Chow) d464b2a tests: Test for migrating encrypted wallets (Andrew Chow) 7a97a56 wallet: Avoid null pointer deref when cleaning up migratewallet (Andrew Chow) Pull request description: Backports remaining changes on the 24.0.1 milestone. Currently backports: * #26594 * #26569 * #26560 ACKs for top commit: josibake: ACK 8b726bf Tree-SHA512: db77ec1a63a7b6a4412750a0f4c0645681fc346a5df0a7cd38d5d27384e1d0fa95f3953af90042afe131ddbd4b6a6e009527095f13e9f58c0190cd378738a9e5
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
When
migratewalletfails, we do an automatic cleanup in order to reset everything so that the user does not experience any interruptions. However, this apparently has a segfault in it, caused by the the pointers to the watchonly and solvables wallets being nullptr. If those wallets are not created (either not needed, or failed early on), we will accidentally attempt to dereference these nullptrs, which causes a segfault.This failure can be easily reached by trying to migrate an encrypted wallet. Currently, we can't migrate encrypted wallets because of how we unload wallets before migrating, and therefore forget the encryption key if the wallet was unlocked. So any encrypted wallets will fail, entering the cleanup, and because watchonly and solvables wallets don't exist yet, the segfault is reached.
This PR fixes this by not putting those nullptrs in a place that we will end up dereferencing them later. It also adds a test that uses the encrypted wallet issue.