wallet: Avoid a segfault in migratewallet failure cleanup #26594

pull achow101 wants to merge 3 commits into bitcoin:master from achow101:fix-migratewallet-cleanup-segfault changing 3 files +15 −3
  1. achow101 commented at 11:37 pm on November 28, 2022: member

    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.

  2. DrahtBot commented at 11:37 pm on November 28, 2022: contributor

    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 stickies-v, furszy, S3RK
    Concept ACK shaavan
    Stale ACK luke-jr

    If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #26596 (wallet: Migrate legacy wallets to descriptor wallets without requiring BDB by achow101)
    • #26595 (wallet: be able to specify a wallet name and passphrase to migratewallet by achow101)
    • #25722 (refactor: Use util::Result class for wallet loading by ryanofsky)

    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.

  3. DrahtBot added the label Wallet on Nov 28, 2022
  4. achow101 force-pushed on Nov 28, 2022
  5. achow101 force-pushed on Nov 28, 2022
  6. achow101 force-pushed on Nov 29, 2022
  7. S3RK commented at 8:03 am on November 29, 2022: contributor

    tACK 91c935a495f2bb25ad24173e73852e749db21b0e

    I think this should be backported to 24.x with a better error message saying that encrypted wallets will be supported in the future.

  8. maflcko added the label Needs backport (24.x) on Nov 29, 2022
  9. fanquake requested review from furszy on Nov 29, 2022
  10. fanquake requested review from stickies-v on Nov 29, 2022
  11. fanquake added this to the milestone 24.0.1 on Nov 29, 2022
  12. in src/wallet/wallet.cpp:4097 in 91c935a495 outdated 
    4181@@ -4182,8 +4182,8 @@ util::Result<MigrationResult> MigrateLegacyToDescriptor(std::shared_ptr<CWallet>
4182 
4183         // Make list of wallets to cleanup
4184         std::vector<std::shared_ptr<CWallet>> created_wallets;
4185-        created_wallets.push_back(std::move(res.watchonly_wallet));
4186-        created_wallets.push_back(std::move(res.solvables_wallet));
4187+        if (res.watchonly_wallet) created_wallets.push_back(std::move(res.watchonly_wallet));
4188+        if (res.solvables_wallet) created_wallets.push_back(std::move(res.solvables_wallet));
    stickies-v commented at 5:19 pm on November 29, 2022:

    res is returned by the function, couldn’t a move operation introduce UB here?

    0        if (res.watchonly_wallet) created_wallets.push_back(res.watchonly_wallet);
1        if (res.solvables_wallet) created_wallets.push_back(res.solvables_wallet);
    achow101 commented at 7:14 pm on November 29, 2022:
    res isn’t returned by the failure path. These also need to be moved in order to keep the refcount at 1 so that these wallets are properly removed and cleaned up in this failure path.
    stickies-v commented at 7:38 pm on November 29, 2022:
    Good point, thanks. Resolved.
  13. stickies-v commented at 5:32 pm on November 29, 2022: contributor
    Approach ACK 91c935a49 - not very familiar with the wallet migration process but this looks like an appropriate bug fix approach to me.
  14. in test/functional/wallet_migration.py:407 in 91c935a495 outdated 
    402+        default = self.nodes[0].get_wallet_rpc(self.default_wallet_name)
403+
404+        wallet.encryptwallet("pass")
405+        addr = wallet.getnewaddress()
406+        default.sendtoaddress(addr, 1)
407+        self.generate(self.nodes[0], 1)
    shaavan commented at 7:08 pm on November 29, 2022:

    These lines seem to be unrelated to the behavior we are testing for, and the test compiles and runs perfectly for the branch and fails for the master (as expected). So, could these lines be removed?

    achow101 commented at 7:30 pm on November 29, 2022:
    Done
  15. shaavan commented at 7:09 pm on November 29, 2022: contributor

    Concept ACK

    • The bug fix seems conceptually correct.
    • The test correctly works and captures the correct working of this part of the code. I ran the test on the master, and it failed, as expected.
  16. achow101 force-pushed on Nov 29, 2022
  17. achow101 force-pushed on Nov 29, 2022
  18. stickies-v approved
  19. stickies-v commented at 7:48 pm on November 29, 2022: contributor
    ACK 7d7d02fad
  20. achow101 commented at 9:17 pm on November 29, 2022: member

    I think this should be backported to 24.x with a better error message saying that encrypted wallets will be supported in the future.

    I’ve added a commit for an explicit error message saying so.

  21. wallet: Avoid null pointer deref when cleaning up migratewallet 
    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.
    86ef7b3c7b
  22. tests: Test for migrating encrypted wallets 
    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.
    88afc73ae0
  23. achow101 force-pushed on Nov 30, 2022
  24. luke-jr approved
  25. luke-jr commented at 0:45 am on November 30, 2022: member
    utACK 0c340e93e160d788fc70ba8d12c578ceeedf8008
  26. in src/wallet/rpc/wallet.cpp:734 in 0c340e93e1 outdated 
    729@@ -730,7 +730,9 @@ static RPCHelpMan migratewallet()
730             std::shared_ptr<CWallet> wallet = GetWalletForJSONRPCRequest(request);
731             if (!wallet) return NullUniValue;
732 
733-            EnsureWalletIsUnlocked(*wallet);
734+            if (wallet->IsCrypted()) {
735+                throw JSONRPCError(RPC_WALLET_UNLOCK_NEEDED, "Error: migratewallet on encrypted wallets is currently unsupported.");
    shaavan commented at 6:20 am on November 30, 2022:

    As stated earlier in this PR. The encrypted legacy wallet cannot be migrated even if it is unlocked. So, in this case, I don’t think RPC_WALLET_UNLOCK_NEEDED would be an appropriate error code to use.

    Suggestion:

    We can add a new error code for this particular behavior in the src/rpc/protocol.h

    0RPC_WALLET_LEGACY_ENCRYPTED     = -37, //!< The legacy wallet is encrypted and is not yet supported for migration.
    stickies-v commented at 11:03 am on November 30, 2022:

    I agree that RPC_WALLET_UNLOCK_NEEDED can be confusing feedback, but I’d prefer not to add an error code for a very specific situation that IIUC is only meant to be temporary. I think we have a couple of existing codes that work fine here, with RPC_WALLET_WRONG_ENC_STATE looking like the most suitable:

    0RPC_WALLET_WRONG_ENC_STATE      = -15, //!< Command given in wrong wallet encryption state (encrypting an encrypted wallet etc.)
    achow101 commented at 3:31 pm on November 30, 2022:
    Changed to RPC_WALLET_WRONG_ENC_STATE.
  27. furszy approved
  28. furszy commented at 12:53 pm on November 30, 2022: member
    Code review ACK 0c340e9. Will re-ACK it if you decide to change the RPC error code too.
  29. wallet: Explicitly say migratewallet on encrypted wallets is unsupported 5e65a216d1
  30. achow101 force-pushed on Nov 30, 2022
  31. fanquake requested review from furszy on Nov 30, 2022
  32. fanquake requested review from stickies-v on Nov 30, 2022
  33. stickies-v approved
  34. stickies-v commented at 5:08 pm on November 30, 2022: contributor
    ACK 5e65a21
  35. furszy approved
  36. furszy commented at 5:15 pm on November 30, 2022: member
    diff ACK 5e65a21
  37. S3RK commented at 8:42 pm on November 30, 2022: contributor
    reACK 5e65a216d1fd00c447757736d4f2899d235e731a
  38. fanquake merged this on Dec 1, 2022
  39. fanquake closed this on Dec 1, 2022
  40. fanquake referenced this in commit 7a97a56ffb on Dec 1, 2022
  41. fanquake referenced this in commit d464b2af30 on Dec 1, 2022
  42. fanquake referenced this in commit 95fded1069 on Dec 1, 2022
  43. fanquake removed the label Needs backport (24.x) on Dec 1, 2022
  44. fanquake commented at 10:28 am on December 1, 2022: member
    Backported in #26616.
  45. sidhujag referenced this in commit c28caa0410 on Dec 1, 2022
  46. maflcko referenced this in commit 3afbc7d67d on Dec 6, 2022
  47. bitcoin locked this on Dec 27, 2023


achow101 DrahtBot S3RK stickies-v shaavan luke-jr furszy fanquake

Labels
Wallet

Milestone
24.0.1

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-12-19 06:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me