From 86ef7b3c7be84e4183098f448c77ecc9ea7367ab Mon Sep 17 00:00:00 2001 From: Andrew Chow Date: Mon, 28 Nov 2022 18:27:27 -0500 Subject: [PATCH 1/3] 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. --- src/wallet/wallet.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index e0f1655ab7..81c79037c6 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -4093,8 +4093,8 @@ util::Result MigrateLegacyToDescriptor(std::shared_ptr // Make list of wallets to cleanup std::vector> created_wallets; - created_wallets.push_back(std::move(res.watchonly_wallet)); - created_wallets.push_back(std::move(res.solvables_wallet)); + if (res.watchonly_wallet) created_wallets.push_back(std::move(res.watchonly_wallet)); + if (res.solvables_wallet) created_wallets.push_back(std::move(res.solvables_wallet)); // Get the directories to remove after unloading for (std::shared_ptr& w : created_wallets) { From 88afc73ae0c67a4482ecd3d77eb2a8fd2673f82d Mon Sep 17 00:00:00 2001 From: Andrew Chow Date: Mon, 28 Nov 2022 18:30:40 -0500 Subject: [PATCH 2/3] 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. --- test/functional/wallet_migration.py | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/test/functional/wallet_migration.py b/test/functional/wallet_migration.py index 3c1cb6ac32..355046c9af 100755 --- a/test/functional/wallet_migration.py +++ b/test/functional/wallet_migration.py @@ -393,6 +393,16 @@ class WalletMigrationTest(BitcoinTestFramework): assert_equal(bals, wallet.getbalances()) + def test_encrypted(self): + self.log.info("Test migration of an encrypted wallet") + wallet = self.create_legacy_wallet("encrypted") + + wallet.encryptwallet("pass") + + wallet.walletpassphrase("pass", 10) + assert_raises_rpc_error(-4, "Error: Unable to produce descriptors for this legacy wallet. Make sure the wallet is unlocked first", wallet.migratewallet) + # TODO: Fix migratewallet so that we can actually migrate encrypted wallets + def run_test(self): self.generate(self.nodes[0], 101) @@ -402,6 +412,7 @@ class WalletMigrationTest(BitcoinTestFramework): self.test_other_watchonly() self.test_no_privkeys() self.test_pk_coinbases() + self.test_encrypted() if __name__ == '__main__': WalletMigrationTest().main() From 5e65a216d1fd00c447757736d4f2899d235e731a Mon Sep 17 00:00:00 2001 From: Andrew Chow Date: Tue, 29 Nov 2022 16:05:51 -0500 Subject: [PATCH 3/3] wallet: Explicitly say migratewallet on encrypted wallets is unsupported --- src/wallet/rpc/wallet.cpp | 4 +++- test/functional/wallet_migration.py | 3 +-- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/src/wallet/rpc/wallet.cpp b/src/wallet/rpc/wallet.cpp index 675c4a759d..971814e9cd 100644 --- a/src/wallet/rpc/wallet.cpp +++ b/src/wallet/rpc/wallet.cpp @@ -730,7 +730,9 @@ static RPCHelpMan migratewallet() std::shared_ptr wallet = GetWalletForJSONRPCRequest(request); if (!wallet) return NullUniValue; - EnsureWalletIsUnlocked(*wallet); + if (wallet->IsCrypted()) { + throw JSONRPCError(RPC_WALLET_WRONG_ENC_STATE, "Error: migratewallet on encrypted wallets is currently unsupported."); + } WalletContext& context = EnsureWalletContext(request.context); diff --git a/test/functional/wallet_migration.py b/test/functional/wallet_migration.py index 355046c9af..4f060f9960 100755 --- a/test/functional/wallet_migration.py +++ b/test/functional/wallet_migration.py @@ -399,8 +399,7 @@ class WalletMigrationTest(BitcoinTestFramework): wallet.encryptwallet("pass") - wallet.walletpassphrase("pass", 10) - assert_raises_rpc_error(-4, "Error: Unable to produce descriptors for this legacy wallet. Make sure the wallet is unlocked first", wallet.migratewallet) + assert_raises_rpc_error(-15, "Error: migratewallet on encrypted wallets is currently unsupported.", wallet.migratewallet) # TODO: Fix migratewallet so that we can actually migrate encrypted wallets def run_test(self):