wallet: RestoreWallet failure, erase only what was created
What the developer wrote
Authored by furszy
wallet: RestoreWallet failure, erase only what was created
Track what RestoreWallet creates so only those files and directories
are removed during a failure and nothing else. Preexisting paths
must be left untouched.
Note:
Using fs::remove_all() instead of fs::remove() in RestoreWallet does
not cause any problems currently, but the change is necessary for the
next commit which extends RestoreWallet to work with existing directories,
which may contain files that must not be deleted.
What changed, and why it matters
This change fixes a cleanup bug in Bitcoin Core's wallet restore feature. Previously, if restoring a wallet failed part-way through, the code would delete the entire wallet directory using a broad 'remove everything' command. The patch makes the function remember exactly which files and folders it created, and only delete those specific items on failure. Right now this doesn't cause data loss, but it is needed to safely support a follow-up change where restore may reuse an existing directory that could contain other important files.
Treat as a hardening/defensive fix with no immediate exploitable vulnerability. Review the follow-up commit that allows restoring into existing directories to ensure the new flags fully cover all created paths. Continue normal testing of wallet restore failure paths.
Security signals we found
Data-loss prevention: replaces broad fs::remove_all with targeted removal of only restore-created paths
State tracking: adds flags to record which filesystem mutations the function performed
Defensive cleanup: uses Assume(fs::is_empty(wallet_path)) before directory removal
Preparation for future feature that will allow restoring into existing directories
Evidence from the diff
RestoreWallet in src/wallet/wallet.cpp previously called fs::remove_all(wallet_path) when post-restore loading failed. The patch introduces two booleans, wallet_file_copied and created_parent_dir, to track state changes made during the restore attempt. On failure it now removes only the copied wallet.dat (if it was copied) and only removes the parent directory if RestoreWallet itself created it and it is empty. This prevents a future scenario—described in the commit message as the next commit—where an existing wallet directory containing unrelated files could be wiped by remove_all(). The current code path does not create that risk because it refuses to restore into an existing directory, so the change is defensive/prepatory.
Changed components
src/wallet/wallet.cppRestoreWallet functionwallet database file and directory creation/cleanupInspect captured patch +19 / −2
diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp
index 4becb340..9c51fb51 100644
--- a/src/wallet/wallet.cpp
+++ b/src/wallet/wallet.cpp
@@ -470,6 +470,8 @@ std::shared_ptr<CWallet> RestoreWallet(WalletContext& context, const fs::path& b
const fs::path wallet_path = fsbridge::AbsPathJoin(GetWalletDir(), fs::u8path(wallet_name));
auto wallet_file = wallet_path / "wallet.dat";
std::shared_ptr<CWallet> wallet;
+ bool wallet_file_copied = false;
+ bool created_parent_dir = false;
try {
if (!fs::exists(backup_file)) {
@@ -478,13 +480,22 @@ std::shared_ptr<CWallet> RestoreWallet(WalletContext& context, const fs::path& b
return nullptr;
}
- if (fs::exists(wallet_path) || !TryCreateDirectories(wallet_path)) {
+ if (fs::exists(wallet_path)) {
error = Untranslated(strprintf("Failed to create database path '%s'. Database already exists.", fs::PathToString(wallet_path)));
status = DatabaseStatus::FAILED_ALREADY_EXISTS;
return nullptr;
+ } else {
+ // The directory doesn't exist, create it
+ if (!TryCreateDirectories(wallet_path)) {
+ error = Untranslated(strprintf("Failed to restore database path '%s'.", fs::PathToString(wallet_path)));
+ status = DatabaseStatus::FAILED_ALREADY_EXISTS;
+ return nullptr;
+ }
+ created_parent_dir = true;
}
fs::copy_file(backup_file, wallet_file, fs::copy_options::none);
+ wallet_file_copied = true;
if (load_after_restore) {
wallet = LoadWallet(context, wallet_name, load_on_start, options, status, error, warnings);
@@ -497,7 +508,13 @@ std::shared_ptr<CWallet> RestoreWallet(WalletContext& context, const fs::path& b
// Remove created wallet path only when loading fails
if (load_after_restore && !wallet) {
- fs::remove_all(wallet_path);
+ if (wallet_file_copied) fs::remove(wallet_file);
+ // Clean up the parent directory if we created it during restoration.
+ // As we have created it, it must be empty after deleting the wallet file.
+ if (created_parent_dir) {
+ Assume(fs::is_empty(wallet_path));
+ fs::remove(wallet_path);
+ }
}
return wallet;
Why this scored 34/100
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.