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",