From a7e4a59d6df117f8afc74544074fd7d7c5ac8b07 Mon Sep 17 00:00:00 2001 From: David Gumberg Date: Wed, 28 Jan 2026 14:57:20 -0800 Subject: [PATCH 1/3] 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", From 8bfb422de83654c18cd341de8eb8e5351959d998 Mon Sep 17 00:00:00 2001 From: David Gumberg Date: Wed, 28 Jan 2026 18:54:31 -0800 Subject: [PATCH 2/3] test: functional: drop unused --keepcache argument MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit At the time this was added in #10197, building the test cache took 21 seconds, as described in that PR, but this is no longer true, as demonstrated by running the functional test framework with and without the --keepcache arguments on master prior to this commit: ``` hyperfine --warmup 1 --export-markdown results.md --runs 3 \ -n 'without --keepcache' './build/test/functional/test_runner.py -j $(nproc)' \ -n 'with --keepcache' './build/test/functional/test_runner.py -j $(nproc) --keepcache' ``` | Command | Mean [s] | Min [s] | Max [s] | Relative | |:----------------------|---------------:|--------:|---:|---:| | `without --keepcache` | 76.373 ± 3.058 | 74.083 | 79.846 | 1.00 | | `with --keepcache` | 77.384 ± 1.836 | 75.952 | 79.454 | 1.01 ± 0.05 | As a consequence, this argument can be removed from the test runner and this also has the benefit of being able to use an RAII-like `tempfile.TemporaryDirectory` instead of having to clean up the cache manually at the end of test runs. bitcoin/bitcoin#10197: https://github.com/bitcoin/bitcoin/pull/10197 --- test/functional/test_runner.py | 14 +++----------- 1 file changed, 3 insertions(+), 11 deletions(-) diff --git a/test/functional/test_runner.py b/test/functional/test_runner.py index cef49a3845e..6cf4aa92116 100755 --- a/test/functional/test_runner.py +++ b/test/functional/test_runner.py @@ -415,7 +415,6 @@ def main(): parser.add_argument('--extended', action='store_true', help='run the extended test suite in addition to the basic tests') parser.add_argument('--help', '-h', '-?', action='store_true', help='print help text and exit') parser.add_argument('--jobs', '-j', type=int, default=4, help='how many test scripts to run in parallel. Default=4.') - parser.add_argument('--keepcache', '-k', action='store_true', help='the default behavior is to flush the cache directory on startup. --keepcache retains the cache from the previous testrun.') parser.add_argument('--quiet', '-q', action='store_true', help='only print dots, results summary and failure logs') parser.add_argument('--tmpdirprefix', '-t', default=tempfile.gettempdir(), help="Root directory for datadirs") parser.add_argument('--failfast', '-F', action='store_true', help='stop execution after the first test failure') @@ -565,9 +564,6 @@ def main(): check_script_list(src_dir=config["environment"]["SRCDIR"], fail_on_warn=fail_on_warn) check_script_prefixes() - if not args.keepcache: - shutil.rmtree("%s/test/cache" % config["environment"]["BUILDDIR"], ignore_errors=True) - run_tests( test_list=test_list, build_dir=config["environment"]["BUILDDIR"], @@ -598,11 +594,6 @@ def run_tests(*, test_list, build_dir, tmpdir, jobs=1, enable_coverage=False, ar # pgrep not supported pass - # Warn if there is a cache directory - cache_dir = "%s/test/cache" % build_dir - if os.path.isdir(cache_dir): - print("%sWARNING!%s There is a cache directory here: %s. If tests fail unexpectedly, try deleting the cache directory." % (BOLD[1], BOLD[0], cache_dir)) - # Warn if there is not enough space on the testing dir min_space = MIN_FREE_SPACE + (jobs - 1) * ADDITIONAL_SPACE_PER_JOB if shutil.disk_usage(tmpdir).free < min_space: @@ -614,7 +605,8 @@ def run_tests(*, test_list, build_dir, tmpdir, jobs=1, enable_coverage=False, ar # a hard link or a copy on any platform. See https://github.com/bitcoin/bitcoin/pull/27561. sys.path.append(tests_dir) - flags = ['--cachedir={}'.format(cache_dir)] + args + cache_tmp_dir = tempfile.TemporaryDirectory(prefix="functional_test_cache") + flags = [f"--cachedir={cache_tmp_dir.name}"] + args if enable_coverage: coverage = RPCCoverage() @@ -631,7 +623,7 @@ def run_tests(*, test_list, build_dir, tmpdir, jobs=1, enable_coverage=False, ar sys.stdout.buffer.write(e.output) raise - #Run Tests + # Run Tests job_queue = TestHandler( num_tests_parallel=jobs, tests_dir=tests_dir, From 0d1301b47a35f1ff04c30d104f774b5f8f0c6995 Mon Sep 17 00:00:00 2001 From: David Gumberg Date: Wed, 28 Jan 2026 15:53:10 -0800 Subject: [PATCH 3/3] test: functional: drop rmtree usage and add lint check `shutil.rmtree` is dangerous because it recursively deletes. There are not likely to be any issues with it's current uses, but it is possible that some of the assumptions being made now won't always be true, e.g. about what some of the variables being passed to `rmtree` represent. For some remaining uses of rmtree that can't be avoided for now, use `cleanup_dir` which asserts that the recursively deleted folder is a child of the the `tmpdir` of the test run. Otherwise, `tempfile.TemporaryDirectory` should be used which does it's own deleting on being garbage collected, or old fashioned unlinking and rmdir in the case of directories with known contents. --- test/functional/feature_assumeutxo.py | 6 ++--- test/functional/feature_blocksdir.py | 3 +-- .../feature_coinstatsindex_compatibility.py | 2 +- test/functional/feature_init.py | 2 +- test/functional/feature_reindex_init.py | 3 +-- .../test_framework/test_framework.py | 9 ++++++- test/functional/test_framework/test_node.py | 9 ++----- test/functional/test_runner.py | 9 ++----- .../wallet_backwards_compatibility.py | 3 ++- test/functional/wallet_hd.py | 8 +++--- test/functional/wallet_migration.py | 9 +++++-- test/functional/wallet_multiwallet.py | 6 ++--- test/functional/wallet_reorgsrestore.py | 2 +- test/functional/wallet_startup.py | 2 +- test/lint/test_runner/src/lint_py.rs | 25 +++++++++++++++++++ test/lint/test_runner/src/main.rs | 7 +++++- 16 files changed, 68 insertions(+), 37 deletions(-) diff --git a/test/functional/feature_assumeutxo.py b/test/functional/feature_assumeutxo.py index 90f597de176..fdcfd75bf14 100755 --- a/test/functional/feature_assumeutxo.py +++ b/test/functional/feature_assumeutxo.py @@ -10,7 +10,6 @@ The assumeutxo value generated and used here is committed to in `CRegTestParams::m_assumeutxo_data` in `src/kernel/chainparams.cpp`. """ import contextlib -from shutil import rmtree from dataclasses import dataclass from test_framework.blocktools import ( @@ -192,7 +191,8 @@ class AssumeutxoTest(BitcoinTestFramework): expected_error(log_msg=error_details, error_msg=expected_error_msg) # resurrect node again - rmtree(chainstate_snapshot_path) + (chainstate_snapshot_path / "base_blockhash").unlink() + chainstate_snapshot_path.rmdir() self.start_node(0) def test_invalid_mempool_state(self, dump_output_path): @@ -304,7 +304,7 @@ class AssumeutxoTest(BitcoinTestFramework): # Start test fresh by cleaning up node directories for node in (snapshot_node, ibd_node): self.stop_node(node.index) - rmtree(node.chain_path) + self.cleanup_folder(node.chain_path) self.start_node(node.index, extra_args=self.extra_args[node.index]) # Sync-up headers chain on snapshot_node to load snapshot diff --git a/test/functional/feature_blocksdir.py b/test/functional/feature_blocksdir.py index 379c2bec166..629ba8f9efb 100755 --- a/test/functional/feature_blocksdir.py +++ b/test/functional/feature_blocksdir.py @@ -5,7 +5,6 @@ """Test the blocksdir option. """ -import shutil from pathlib import Path from test_framework.test_framework import BitcoinTestFramework, initialize_datadir @@ -20,7 +19,7 @@ class BlocksdirTest(BitcoinTestFramework): self.stop_node(0) assert self.nodes[0].blocks_path.is_dir() assert not (self.nodes[0].datadir_path / "blocks").is_dir() - shutil.rmtree(self.nodes[0].datadir_path) + self.cleanup_folder(self.nodes[0].datadir_path) initialize_datadir(self.options.tmpdir, 0, self.chain) self.log.info("Starting with nonexistent blocksdir ...") blocksdir_path = Path(self.options.tmpdir) / 'blocksdir' diff --git a/test/functional/feature_coinstatsindex_compatibility.py b/test/functional/feature_coinstatsindex_compatibility.py index 5031494fd8a..855dc74d82e 100755 --- a/test/functional/feature_coinstatsindex_compatibility.py +++ b/test/functional/feature_coinstatsindex_compatibility.py @@ -49,7 +49,7 @@ class CoinStatsIndexTest(BitcoinTestFramework): self.log.info("Test that gettxoutsetinfo() output is consistent for the new index running on a datadir with the old version") self.stop_nodes() - shutil.rmtree(node.chain_path / "indexes" / "coinstatsindex") + self.cleanup_folder(node.chain_path / "indexes" / "coinstatsindex") shutil.copytree(legacy_node.chain_path / "indexes" / "coinstats", node.chain_path / "indexes" / "coinstats") old_version_path = node.chain_path / "indexes" / "coinstats" msg = f'[warning] Old version of coinstatsindex found at {old_version_path}. This folder can be safely deleted unless you plan to downgrade your node to version 29 or lower.' diff --git a/test/functional/feature_init.py b/test/functional/feature_init.py index 05a939c2b39..259e07b1869 100755 --- a/test/functional/feature_init.py +++ b/test/functional/feature_init.py @@ -241,7 +241,7 @@ class InitTest(BitcoinTestFramework): start_expecting_error(err_fragment, startup_args) for dir in dirs: - shutil.rmtree(node.chain_path / dir) + self.cleanup_folder(node.chain_path / dir) shutil.move(node.chain_path / f"{dir}_bak", node.chain_path / dir) def init_pid_test(self): diff --git a/test/functional/feature_reindex_init.py b/test/functional/feature_reindex_init.py index d2ef6c5e220..ef21e8a7ed4 100755 --- a/test/functional/feature_reindex_init.py +++ b/test/functional/feature_reindex_init.py @@ -7,7 +7,6 @@ from test_framework.test_framework import BitcoinTestFramework from test_framework.util import assert_equal import os -import shutil class ReindexInitTest(BitcoinTestFramework): @@ -19,7 +18,7 @@ class ReindexInitTest(BitcoinTestFramework): self.stop_nodes() self.log.info("Removing the block index leads to init error") - shutil.rmtree(node.blocks_path / "index") + self.cleanup_folder(node.blocks_path / "index") node.assert_start_raises_init_error( expected_msg=f"Error initializing block database.{os.linesep}" "Please restart with -reindex or -reindex-chainstate to recover.", diff --git a/test/functional/test_framework/test_framework.py b/test/functional/test_framework/test_framework.py index 95bfc964d60..b809e68f4df 100755 --- a/test/functional/test_framework/test_framework.py +++ b/test/functional/test_framework/test_framework.py @@ -10,6 +10,7 @@ import argparse from datetime import datetime, timezone import logging import os +from pathlib import Path import platform import pdb import random @@ -331,7 +332,7 @@ class BitcoinTestFramework(metaclass=BitcoinTestMetaClass): h.flush() rpc_logger.removeHandler(h) if cleanup_tree_on_exit: - shutil.rmtree(self.options.tmpdir) + self.cleanup_folder(self.options.tmpdir) self.nodes.clear() return exit_code @@ -1031,3 +1032,9 @@ class BitcoinTestFramework(metaclass=BitcoinTestMetaClass): return result except ImportError: self.log.warning("sqlite3 module not available, skipping tests that inspect the database") + + def cleanup_folder(self, _path): + path = Path(_path) + if not path.is_relative_to(self.options.tmpdir): + raise AssertionError(f"Trying to delete #{path} outside of #{self.options.tmpdir}") + shutil.rmtree(path) diff --git a/test/functional/test_framework/test_node.py b/test/functional/test_framework/test_node.py index f2032bc458a..541c758b5ec 100755 --- a/test/functional/test_framework/test_node.py +++ b/test/functional/test_framework/test_node.py @@ -11,7 +11,6 @@ from enum import Enum import json import logging import os -import pathlib import platform import re import subprocess @@ -20,7 +19,6 @@ import time import urllib.parse import collections import shlex -import shutil import sys from collections.abc import Iterable from pathlib import Path @@ -163,8 +161,8 @@ class TestNode(): self.args.append("-ipcbind=unix") else: # Work around default CI path exceeding maximum socket path length. - self.ipc_tmp_dir = pathlib.Path(tempfile.mkdtemp(prefix="test-ipc-")) - self.ipc_socket_path = self.ipc_tmp_dir / "node.sock" + self.ipc_tmp_dir = tempfile.TemporaryDirectory(prefix="test-ipc-") + self.ipc_socket_path = Path(self.ipc_tmp_dir.name) / "node.sock" self.args.append(f"-ipcbind=unix:{self.ipc_socket_path}") if self.version_is_at_least(190000): @@ -248,9 +246,6 @@ class TestNode(): # this destructor is called. print(self._node_msg("Cleaning up leftover process"), file=sys.stderr) self.process.kill() - if self.ipc_tmp_dir: - print(self._node_msg(f"Cleaning up ipc directory {str(self.ipc_tmp_dir)!r}")) - shutil.rmtree(self.ipc_tmp_dir) def __getattr__(self, name): """Dispatches any unrecognised messages to the RPC connection or a CLI instance.""" diff --git a/test/functional/test_runner.py b/test/functional/test_runner.py index 6cf4aa92116..005df69b057 100755 --- a/test/functional/test_runner.py +++ b/test/functional/test_runner.py @@ -681,9 +681,6 @@ def run_tests(*, test_list, build_dir, tmpdir, jobs=1, enable_coverage=False, ar if coverage: coverage_passed = coverage.report_rpc_coverage() - - logging.debug("Cleaning up coverage data") - coverage.cleanup() else: coverage_passed = True @@ -898,7 +895,8 @@ class RPCCoverage(): """ def __init__(self): - self.dir = tempfile.mkdtemp(prefix="coverage") + self.temp_dir = tempfile.TemporaryDirectory(prefix="coverage") + self.dir = self.temp_dir.name self.flag = '--coveragedir=%s' % self.dir def report_rpc_coverage(self): @@ -916,9 +914,6 @@ class RPCCoverage(): print("All RPC commands covered.") return True - def cleanup(self): - return shutil.rmtree(self.dir) - def _get_uncovered_rpc_commands(self): """ Return a set of currently untested RPC commands. diff --git a/test/functional/wallet_backwards_compatibility.py b/test/functional/wallet_backwards_compatibility.py index a67b4676a86..13c3ef7488a 100755 --- a/test/functional/wallet_backwards_compatibility.py +++ b/test/functional/wallet_backwards_compatibility.py @@ -303,7 +303,8 @@ class BackwardsCompatibilityTest(BitcoinTestFramework): # Copy back to master wallet.unloadwallet() if n == node: - shutil.rmtree(node_master.wallets_path / wallet_name) + (node_master.wallets_path / wallet_name / "wallet.dat").unlink() + (node_master.wallets_path / wallet_name).rmdir() shutil.copytree( n.wallets_path / wallet_name, node_master.wallets_path / wallet_name, diff --git a/test/functional/wallet_hd.py b/test/functional/wallet_hd.py index 62ad88d2d7d..c130c731ce5 100755 --- a/test/functional/wallet_hd.py +++ b/test/functional/wallet_hd.py @@ -70,8 +70,8 @@ class WalletHDTest(BitcoinTestFramework): self.stop_node(1) # we need to delete the complete chain directory # otherwise node1 would auto-recover all funds in flag the keypool keys as used - shutil.rmtree(self.nodes[1].blocks_path) - shutil.rmtree(self.nodes[1].chain_path / "chainstate") + self.cleanup_folder(self.nodes[1].blocks_path) + self.cleanup_folder(self.nodes[1].chain_path / "chainstate") shutil.copyfile( self.nodes[1].datadir_path / "hd.bak", self.nodes[1].wallets_path / self.default_wallet_name / self.wallet_data_filename @@ -95,8 +95,8 @@ class WalletHDTest(BitcoinTestFramework): # Try a RPC based rescan self.stop_node(1) - shutil.rmtree(self.nodes[1].blocks_path) - shutil.rmtree(self.nodes[1].chain_path / "chainstate") + self.cleanup_folder(self.nodes[1].blocks_path) + self.cleanup_folder(self.nodes[1].chain_path / "chainstate") shutil.copyfile( self.nodes[1].datadir_path / "hd.bak", self.nodes[1].wallets_path / self.default_wallet_name / self.wallet_data_filename diff --git a/test/functional/wallet_migration.py b/test/functional/wallet_migration.py index 45ebd435cde..40bc6b3a130 100755 --- a/test/functional/wallet_migration.py +++ b/test/functional/wallet_migration.py @@ -3,6 +3,7 @@ # Distributed under the MIT software license, see the accompanying # file COPYING or http://www.opensource.org/licenses/mit-license.php. """Test Migrating a wallet from legacy to descriptor.""" +from contextlib import suppress from pathlib import Path import os.path import random @@ -664,8 +665,12 @@ class WalletMigrationTest(BitcoinTestFramework): # Test cleanup: Clear unnamed default wallet for subsequent tests (self.old_node.wallets_path / "wallet.dat").unlink() (self.master_node.wallets_path / "wallet.dat").unlink(missing_ok=True) - shutil.rmtree(self.master_node.wallets_path / "default_wallet_watchonly", ignore_errors=True) - shutil.rmtree(self.master_node.wallets_path / "default_wallet_solvables", ignore_errors=True) + with suppress(FileNotFoundError): + (self.master_node.wallets_path / "default_wallet_watchonly" / "wallet.dat").unlink() + (self.master_node.wallets_path / "default_wallet_watchonly").rmdir() + + (self.master_node.wallets_path / "default_wallet_solvables" / "wallet.dat").unlink() + (self.master_node.wallets_path / "default_wallet_solvables").rmdir() backup_file.unlink() def test_default_wallet(self): diff --git a/test/functional/wallet_multiwallet.py b/test/functional/wallet_multiwallet.py index e8d6c6caa71..5b9a23b593f 100755 --- a/test/functional/wallet_multiwallet.py +++ b/test/functional/wallet_multiwallet.py @@ -140,12 +140,12 @@ class MultiWalletTest(BitcoinTestFramework): self.stop_nodes() empty_wallet = os.path.join(self.options.tmpdir, 'empty.dat') os.rename(self.wallet_file(node, "empty"), empty_wallet) - shutil.rmtree(wallet_dir(node, "empty")) + os.rmdir(wallet_dir(node, "empty")) empty_created_wallet = os.path.join(self.options.tmpdir, 'empty.created.dat') os.rename(wallet_dir(node, "created", self.wallet_data_filename), empty_created_wallet) - shutil.rmtree(wallet_dir(node, "created")) + os.rmdir(wallet_dir(node, "created")) os.rename(self.wallet_file(node, "plain"), wallet_dir(node, "w8")) - shutil.rmtree(wallet_dir(node, "plain")) + os.rmdir(wallet_dir(node, "plain")) # restart node with a mix of wallet names: # w1, w2, w3 - to verify new wallets created when non-existing paths specified diff --git a/test/functional/wallet_reorgsrestore.py b/test/functional/wallet_reorgsrestore.py index 00693db85d1..353a9e9d4ba 100755 --- a/test/functional/wallet_reorgsrestore.py +++ b/test/functional/wallet_reorgsrestore.py @@ -72,7 +72,7 @@ class ReorgsRestoreTest(BitcoinTestFramework): # Stop both nodes and replace node0 chain entirely for the node1 chain self.stop_nodes() for path in ["chainstate", "blocks"]: - shutil.rmtree(self.nodes[0].chain_path / path) + self.cleanup_folder(self.nodes[0].chain_path / path) shutil.copytree(self.nodes[1].chain_path / path, self.nodes[0].chain_path / path) # Start node0 and verify that now it has node1 chain and no info about its previous best block diff --git a/test/functional/wallet_startup.py b/test/functional/wallet_startup.py index d2355360ac2..fe35ee12f9e 100755 --- a/test/functional/wallet_startup.py +++ b/test/functional/wallet_startup.py @@ -36,7 +36,7 @@ class WalletStartupTest(BitcoinTestFramework): self.nodes[0].createwallet(wallet_name=wallet_name, **kwargs) self.nodes[0].unloadwallet(wallet_name) shutil.move(self.nodes[0].wallets_path / wallet_name / "wallet.dat", self.nodes[0].wallets_path / "wallet.dat") - shutil.rmtree(self.nodes[0].wallets_path / wallet_name) + (self.nodes[0].wallets_path / wallet_name).rmdir() def run_test(self): self.log.info('Should start without any wallets') diff --git a/test/lint/test_runner/src/lint_py.rs b/test/lint/test_runner/src/lint_py.rs index d9177c86907..a335ba27bcd 100644 --- a/test/lint/test_runner/src/lint_py.rs +++ b/test/lint/test_runner/src/lint_py.rs @@ -73,3 +73,28 @@ pub fn lint_py_lint() -> LintResult { Err(e) => Err(format!("Error running `{bin_name}`: {e}")), } } + +pub fn lint_rmtree() -> LintResult { + let found = git() + .args([ + "grep", + "--line-number", + "rmtree", + "--", + "test/functional/", + ":(exclude)test/functional/test_framework/test_framework.py", + ]) + .status() + .expect("command error") + .success(); + if found { + Err(r#" +Use of shutil.rmtree() is dangerous and should be avoided. + "# + .trim() + .to_string()) + } else { + Ok(()) + } +} + diff --git a/test/lint/test_runner/src/main.rs b/test/lint/test_runner/src/main.rs index 9e80e0f32a8..8d34fc6e24c 100644 --- a/test/lint/test_runner/src/main.rs +++ b/test/lint/test_runner/src/main.rs @@ -17,7 +17,7 @@ use lint_cpp::{ 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; +use lint_py::{lint_py_lint, lint_rmtree}; use lint_repo_hygiene::{lint_scripted_diff, lint_subtree}; use lint_text_format::{ lint_commit_msg, lint_tabs_whitespace, lint_trailing_newline, lint_trailing_whitespace, @@ -52,6 +52,11 @@ fn get_linter_list() -> Vec<&'static Linter> { name: "py_lint", lint_fn: lint_py_lint, }, + &Linter { + description: "Check that shutil.rmtree is not used", + name: "rmtree", + lint_fn: lint_rmtree, + }, &Linter { description: "Check that std::filesystem is not used directly", name: "std_filesystem",