From 0d1301b47a35f1ff04c30d104f774b5f8f0c6995 Mon Sep 17 00:00:00 2001
From: David Gumberg <davidzgumberg@gmail.com>
Date: Wed, 28 Jan 2026 15:53:10 -0800
Subject: [PATCH] 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",