From a7e4a59d6df117f8afc74544074fd7d7c5ac8b07 Mon Sep 17 00:00:00 2001 From: David Gumberg Date: Wed, 28 Jan 2026 14:57:20 -0800 Subject: [PATCH] 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. --- src/bench/wallet_create.cpp | 9 ++++++--- src/test/fuzz/i2p.cpp | 2 +- src/test/kernel/test_kernel.cpp | 4 ++-- src/test/util_tests.cpp | 3 ++- test/lint/test_runner/src/lint_cpp.rs | 26 ++++++++++++++++++++++++++ test/lint/test_runner/src/main.rs | 7 ++++++- 6 files changed, 43 insertions(+), 8 deletions(-) diff --git a/src/bench/wallet_create.cpp b/src/bench/wallet_create.cpp index 1c92beddec2..d29ef339d12 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 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 613b856dffd..d262e1d0f31 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 d4a8647b662..c1c63ab1ab4 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 8896807f6ea..8a1be051b7d 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 4ad3c06d30b..e587c4c03e3 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 fa69d2b5f9a..9e80e0f32a8 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",