qa: Remove all instances of `remove_all` except test cleanup
What the developer wrote
Authored by David Gumberg
qa: Remove all instances of `remove_all` except test cleanup
Adds a lint check for `remove_all()`
`fs::remove_all()`/`std::filesystem::remove_all()` is extremely
dangerous, all user-facing instances of it have been removed, and it
also deserves to be removed from the places in our test code where it is
being used unnecessarily.
What changed, and why it matters
This commit is a defensive cleanup: it removes almost all uses of a filesystem function called remove_all() that can wipe out entire directory trees, and adds an automated lint check to prevent it from being reintroduced. The changes only affect benchmark and test code, not the live Bitcoin wallet or node software that ordinary users run. It reduces the risk of accidental data loss during testing, but does not fix an active security bug.
No urgent action needed. Reviewers should verify the remaining excluded remove_all() uses in src/test/kernel/test_kernel.cpp and src/test/util/setup_common.cpp are truly constrained to test cleanup and cannot be replaced with safer alternatives. Monitor future PRs for lint bypasses or new remove_all() introductions.
Security signals we found
Commit message labels fs::remove_all()/std::filesystem::remove_all() as 'extremely dangerous'
Replaces recursive directory deletion with single-file/directory deletion in tests/benchmarks
Adds a CI lint to block future remove_all() usage in src/
No changes to production wallet/node code paths
Two test cleanup locations remain excluded from the lint as 'likely not avoidable'
Evidence from the diff
The patch replaces fs::remove_all()/std::filesystem::remove_all() with targeted fs::remove() calls in four benchmark/test files, and adds a new lint_remove_all() check in the Rust lint runner that greps ./src/ for remove_all(.*) and fails CI if found (with two test-file exclusions). The commit message explicitly calls remove_all ‘extremely dangerous’ and states all user-facing instances were already removed; this change removes unnecessary uses in test code and prevents regressions.
Changed components
src/bench/wallet_create.cppsrc/test/fuzz/i2p.cppsrc/test/kernel/test_kernel.cppsrc/test/util_tests.cpptest/lint/test_runner/src/lint_cpp.rstest/lint/test_runner/src/main.rsInspect captured patch +43 / −8
diff --git a/src/bench/wallet_create.cpp b/src/bench/wallet_create.cpp
index 1c92bedd..d29ef339 100644
--- a/src/bench/wallet_create.cpp
+++ b/src/bench/wallet_create.cpp
@@ -44,16 +44,19 @@ static void WalletCreate(benchmark::Bench& bench, bool encrypted)
bilingual_str error_string;
std::vector<bilingual_str> warnings;
- auto wallet_path = fs::PathToString(test_setup->m_path_root / "test_wallet");
+ const auto wallet_path = test_setup->m_path_root / "test_wallet";
+ const auto wallet_name = fs::PathToString(wallet_path);
+
bench.run([&] {
- auto wallet = CreateWallet(context, wallet_path, /*load_on_start=*/std::nullopt, options, status, error_string, warnings);
+ auto wallet = CreateWallet(context, wallet_name, /*load_on_start=*/std::nullopt, options, status, error_string, warnings);
assert(status == DatabaseStatus::SUCCESS);
assert(wallet != nullptr);
// Release wallet
RemoveWallet(context, wallet, /*load_on_start=*/ std::nullopt);
WaitForDeleteWallet(std::move(wallet));
- fs::remove_all(wallet_path);
+ fs::remove(wallet_path / "wallet.dat");
+ fs::remove(wallet_path);
});
}
diff --git a/src/test/fuzz/i2p.cpp b/src/test/fuzz/i2p.cpp
index 613b856d..d262e1d0 100644
--- a/src/test/fuzz/i2p.cpp
+++ b/src/test/fuzz/i2p.cpp
@@ -61,7 +61,7 @@ FUZZ_TARGET(i2p, .init = initialize_i2p)
}
}
- fs::remove_all(private_key_path);
+ fs::remove(private_key_path);
CreateSock = CreateSockOrig;
}
diff --git a/src/test/kernel/test_kernel.cpp b/src/test/kernel/test_kernel.cpp
index d4a8647b..c1c63ab1 100644
--- a/src/test/kernel/test_kernel.cpp
+++ b/src/test/kernel/test_kernel.cpp
@@ -1175,8 +1175,8 @@ BOOST_AUTO_TEST_CASE(btck_chainman_regtest_tests)
BOOST_CHECK_EQUAL(count, chain.CountEntries());
- fs::remove_all(test_directory.m_directory / "blocks" / "blk00000.dat");
+ fs::remove(test_directory.m_directory / "blocks" / "blk00000.dat");
BOOST_CHECK(!chainman->ReadBlock(tip_2).has_value());
- fs::remove_all(test_directory.m_directory / "blocks" / "rev00000.dat");
+ fs::remove(test_directory.m_directory / "blocks" / "rev00000.dat");
BOOST_CHECK_THROW(chainman->ReadBlockSpentOutputs(tip), std::runtime_error);
}
diff --git a/src/test/util_tests.cpp b/src/test/util_tests.cpp
index 8896807f..8a1be051 100644
--- a/src/test/util_tests.cpp
+++ b/src/test/util_tests.cpp
@@ -1138,7 +1138,8 @@ BOOST_AUTO_TEST_CASE(test_LockDirectory)
#endif
// Clean up
ReleaseDirectoryLocks();
- fs::remove_all(dirname);
+ fs::remove(dirname / lockname);
+ fs::remove(dirname);
}
BOOST_AUTO_TEST_CASE(test_ToLower)
diff --git a/test/lint/test_runner/src/lint_cpp.rs b/test/lint/test_runner/src/lint_cpp.rs
index 4ad3c06d..e587c4c0 100644
--- a/test/lint/test_runner/src/lint_cpp.rs
+++ b/test/lint/test_runner/src/lint_cpp.rs
@@ -122,6 +122,32 @@ fs:: namespace, which has unsafe filesystem functions marked as deleted.
}
}
+pub fn lint_remove_all() -> LintResult {
+ let found = git()
+ .args([
+ "grep",
+ "--line-number",
+ "remove_all(.*)",
+ "--",
+ "./src/",
+ // These are likely not avoidable.
+ ":(exclude)src/test/kernel/test_kernel.cpp",
+ ":(exclude)src/test/util/setup_common.cpp",
+ ])
+ .status()
+ .expect("command error")
+ .success();
+ if found {
+ Err(r#"
+Use of fs::remove_all or std::filesystem::remove_all is dangerous and should be avoided.
+ "#
+ .trim()
+ .to_string())
+ } else {
+ Ok(())
+ }
+}
+
pub fn lint_rpc_assert() -> LintResult {
let found = git()
.args([
diff --git a/test/lint/test_runner/src/main.rs b/test/lint/test_runner/src/main.rs
index fa69d2b5..9e80e0f3 100644
--- a/test/lint/test_runner/src/main.rs
+++ b/test/lint/test_runner/src/main.rs
@@ -14,7 +14,7 @@ use std::fs;
use std::process::{Command, ExitCode};
use lint_cpp::{
- lint_boost_assert, lint_includes_build_config, lint_rpc_assert, lint_std_filesystem,
+ lint_boost_assert, lint_includes_build_config, lint_remove_all, lint_rpc_assert, lint_std_filesystem,
};
use lint_docs::{lint_doc_args, lint_doc_release_note_snippets, lint_markdown};
use lint_py::lint_py_lint;
@@ -57,6 +57,11 @@ fn get_linter_list() -> Vec<&'static Linter> {
name: "std_filesystem",
lint_fn: lint_std_filesystem
},
+ &Linter {
+ description: "Check that remove_all is not used",
+ name: "remove_all",
+ lint_fn: lint_remove_all
+ },
&Linter {
description: "Check that fatal assertions are not used in RPC code",
name: "rpc_assert",
Why this scored 27/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.