Skip to content

Commit 9f65006

Browse files
committed
Merge bitcoin/bitcoin#26005: Wallet: Fix error handling (copy_file failure in RestoreWallet, and in general via interfaces)
c3e5365 Bugfix: Wallet: Return util::Error rather than non-error nullptr when CreateWallet/LoadWallet/RestoreWallet fail (Luke Dashjr) 335ff98 Bugfix: Wallet: Wrap RestoreWallet content in a try block to ensure exceptions become returned errors and incomplete wallet directory is removed (Luke Dashjr) Pull request description: Bug 1: `copy_file` can throw exceptions, but `RestoreWallet` is expected to return a nullptr with a populated `errors` parameter. This is fixed by wrapping `copy_file` and `LoadWallet` (for good measure) in a `try` block, and converting any exceptions to the intended return style. Bug 2: `util::Result` turns what would have been a `false` unique_ptr into a `true` nullptr result, which leads to nullptr dereferences in at least the 3 cases of wallet creation/loading/restoring. This is fixed by keeping the pointer as a plain `std::unique_ptr` until actually returning it (ie, after the nullptr check). Fixes #661 ACKs for top commit: achow101: ACK c3e5365 Tree-SHA512: 4291b3dbbb147acea2e63a704324c9371bc16ecb4237f8753729b0b0a6e55c9758ad61bfe8bd432fd7b0bae95d8b63a9831e61ac8b8d5c0197b550a2e0f4a105
2 parents 55e1deb + c3e5365 commit 9f65006

File tree

2 files changed

+38
-20
lines changed

2 files changed

+38
-20
lines changed

src/wallet/interfaces.cpp

Lines changed: 18 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -560,8 +560,12 @@ class WalletLoaderImpl : public WalletLoader
560560
options.create_flags = wallet_creation_flags;
561561
options.create_passphrase = passphrase;
562562
bilingual_str error;
563-
util::Result<std::unique_ptr<Wallet>> wallet{MakeWallet(m_context, CreateWallet(m_context, name, /*load_on_start=*/true, options, status, error, warnings))};
564-
return wallet ? std::move(wallet) : util::Error{error};
563+
std::unique_ptr<Wallet> wallet{MakeWallet(m_context, CreateWallet(m_context, name, /*load_on_start=*/true, options, status, error, warnings))};
564+
if (wallet) {
565+
return {std::move(wallet)};
566+
} else {
567+
return util::Error{error};
568+
}
565569
}
566570
util::Result<std::unique_ptr<Wallet>> loadWallet(const std::string& name, std::vector<bilingual_str>& warnings) override
567571
{
@@ -570,15 +574,23 @@ class WalletLoaderImpl : public WalletLoader
570574
ReadDatabaseArgs(*m_context.args, options);
571575
options.require_existing = true;
572576
bilingual_str error;
573-
util::Result<std::unique_ptr<Wallet>> wallet{MakeWallet(m_context, LoadWallet(m_context, name, /*load_on_start=*/true, options, status, error, warnings))};
574-
return wallet ? std::move(wallet) : util::Error{error};
577+
std::unique_ptr<Wallet> wallet{MakeWallet(m_context, LoadWallet(m_context, name, /*load_on_start=*/true, options, status, error, warnings))};
578+
if (wallet) {
579+
return {std::move(wallet)};
580+
} else {
581+
return util::Error{error};
582+
}
575583
}
576584
util::Result<std::unique_ptr<Wallet>> restoreWallet(const fs::path& backup_file, const std::string& wallet_name, std::vector<bilingual_str>& warnings) override
577585
{
578586
DatabaseStatus status;
579587
bilingual_str error;
580-
util::Result<std::unique_ptr<Wallet>> wallet{MakeWallet(m_context, RestoreWallet(m_context, backup_file, wallet_name, /*load_on_start=*/true, status, error, warnings))};
581-
return wallet ? std::move(wallet) : util::Error{error};
588+
std::unique_ptr<Wallet> wallet{MakeWallet(m_context, RestoreWallet(m_context, backup_file, wallet_name, /*load_on_start=*/true, status, error, warnings))};
589+
if (wallet) {
590+
return {std::move(wallet)};
591+
} else {
592+
return util::Error{error};
593+
}
582594
}
583595
std::string getWalletDir() override
584596
{

src/wallet/wallet.cpp

Lines changed: 20 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -386,25 +386,31 @@ std::shared_ptr<CWallet> RestoreWallet(WalletContext& context, const fs::path& b
386386
ReadDatabaseArgs(*context.args, options);
387387
options.require_existing = true;
388388

389-
if (!fs::exists(backup_file)) {
390-
error = Untranslated("Backup file does not exist");
391-
status = DatabaseStatus::FAILED_INVALID_BACKUP_FILE;
392-
return nullptr;
393-
}
394-
395389
const fs::path wallet_path = fsbridge::AbsPathJoin(GetWalletDir(), fs::u8path(wallet_name));
390+
auto wallet_file = wallet_path / "wallet.dat";
391+
std::shared_ptr<CWallet> wallet;
396392

397-
if (fs::exists(wallet_path) || !TryCreateDirectories(wallet_path)) {
398-
error = Untranslated(strprintf("Failed to create database path '%s'. Database already exists.", fs::PathToString(wallet_path)));
399-
status = DatabaseStatus::FAILED_ALREADY_EXISTS;
400-
return nullptr;
401-
}
393+
try {
394+
if (!fs::exists(backup_file)) {
395+
error = Untranslated("Backup file does not exist");
396+
status = DatabaseStatus::FAILED_INVALID_BACKUP_FILE;
397+
return nullptr;
398+
}
402399

403-
auto wallet_file = wallet_path / "wallet.dat";
404-
fs::copy_file(backup_file, wallet_file, fs::copy_options::none);
400+
if (fs::exists(wallet_path) || !TryCreateDirectories(wallet_path)) {
401+
error = Untranslated(strprintf("Failed to create database path '%s'. Database already exists.", fs::PathToString(wallet_path)));
402+
status = DatabaseStatus::FAILED_ALREADY_EXISTS;
403+
return nullptr;
404+
}
405405

406-
auto wallet = LoadWallet(context, wallet_name, load_on_start, options, status, error, warnings);
406+
fs::copy_file(backup_file, wallet_file, fs::copy_options::none);
407407

408+
wallet = LoadWallet(context, wallet_name, load_on_start, options, status, error, warnings);
409+
} catch (const std::exception& e) {
410+
assert(!wallet);
411+
if (!error.empty()) error += Untranslated("\n");
412+
error += strprintf(Untranslated("Unexpected exception: %s"), e.what());
413+
}
408414
if (!wallet) {
409415
fs::remove(wallet_file);
410416
fs::remove(wallet_path);

0 commit comments

Comments
 (0)