From 45f957af23ba3371b8f0a8f9c2e2595089ff3e5c Mon Sep 17 00:00:00 2001 From: flan Date: Mon, 13 Jul 2026 16:06:12 +0000 Subject: [PATCH] v0.3.5: log the recursive-delete failure instead of swallowing it delete_snapshot_tree tries one recursive delete first, then falls back to sweeping the tree by name. The exception from the fast path was discarded. That failure is usually benign -- stock's finally already removed the parent once our mounts were released, which is exactly what the sweep exists to handle. But if the cause were anything else, this was the only place it was ever visible, and it went straight to /dev/null. The sweep would then report some different, downstream symptom. It is now logged before falling through. Also annotated the two remaining static-analysis findings as considered rather than leaving them to be re-litigated every audit: subprocess is always invoked in list form (no shell, so ZFS dataset names cannot inject), and a partial `systemctl` path is moot in a script that only ever runs as root. Extended ruleset (E,F,W,B,S,SIM,UP,C4,RET,ARG,A,ISC) and shellcheck -S style both report zero. 132 tests. --- CHANGELOG.md | 15 +++++++++++++++ install.sh | 2 +- patch/apply.sh | 2 +- patch/create_task.py | 7 +++++-- patch/truecloud_nested.py | 18 +++++++++++++++--- recover.sh | 2 +- uninstall.sh | 2 +- 7 files changed, 39 insertions(+), 9 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2f57bba..474ce9f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,20 @@ # Changelog +## v0.3.5 — 2026-07-13 + +### Changed + +- `delete_snapshot_tree` swallowed the error from its recursive-delete fast path. + That failure is *usually* just "parent already gone" — stock's `finally` winning + the race once our mounts are released, which the by-name sweep then handles. But + if the cause were anything else, this was the only place it was visible, and it + went straight to `/dev/null`. It is now logged before falling through. + +- Annotated the two remaining static-analysis findings as considered-and-accepted + rather than leaving them to be re-litigated: `subprocess` is always called in + list form (no shell, so ZFS dataset names cannot inject), and the partial + `systemctl` path is moot in a script that only runs as root. + ## v0.3.4 — 2026-07-13 ### Changed diff --git a/install.sh b/install.sh index 1a5ead3..5dcd848 100755 --- a/install.sh +++ b/install.sh @@ -18,7 +18,7 @@ set -euo pipefail -VERSION="0.3.4" +VERSION="0.3.5" # The directory containing install.sh is the permanent install location. PATCH_DIR="$(cd "$(dirname "$0")" && pwd)" diff --git a/patch/apply.sh b/patch/apply.sh index 608cc1a..7aae4c3 100755 --- a/patch/apply.sh +++ b/patch/apply.sh @@ -32,7 +32,7 @@ # Derive PATCH_DIR from this script's location (parent of the patch/ directory). PATCH_DIR="$(cd "$(dirname "$0")/.." && pwd)" LOG="$PATCH_DIR/apply.log" -VERSION="0.3.4" +VERSION="0.3.5" # Rotate log at 512 KB to avoid unbounded growth on a system volume. # Keep two prior generations (.1 and .2) so the last three boots are always available. diff --git a/patch/create_task.py b/patch/create_task.py index c64285d..8f99bd1 100755 --- a/patch/create_task.py +++ b/patch/create_task.py @@ -52,7 +52,7 @@ import subprocess import sys import time -__version__ = "0.3.4" +__version__ = "0.3.5" _PATCH_DIR = os.path.dirname(os.path.dirname(os.path.abspath(__file__))) _STATUS_FILE = os.path.join(_PATCH_DIR, "hook_status.json") @@ -95,8 +95,11 @@ def midclt_call(method, *args): def _middlewared_start_epoch(): """Epoch timestamp of the running middlewared main process, or None.""" try: + # Partial path (S607) is fine here: this runs as root on TrueNAS, so an + # attacker who can poison PATH already has root. Hard-coding a path would + # be less portable (/bin vs /usr/bin) for no security gain. pid = int(subprocess.run( - ["systemctl", "show", "--property=MainPID", "--value", "middlewared"], + ["systemctl", "show", "--property=MainPID", "--value", "middlewared"], # noqa: S607 capture_output=True, text=True, timeout=10, check=True, ).stdout.strip()) if pid <= 0: diff --git a/patch/truecloud_nested.py b/patch/truecloud_nested.py index fbc5b03..18bea3a 100644 --- a/patch/truecloud_nested.py +++ b/patch/truecloud_nested.py @@ -279,7 +279,11 @@ def current_mounts_under(root, mounts_file="/proc/self/mounts"): def _run(cmd): - return subprocess.run(cmd, capture_output=True, text=True, check=False) + # List form, never shell=True: `cmd` is built from our own mount plan, so ZFS + # dataset names cannot inject. Runs as root by definition (it mounts). + return subprocess.run( # noqa: S603 + cmd, capture_output=True, text=True, check=False + ) def apply_plan(mounts, runner=_run, isdir=os.path.isdir): @@ -379,8 +383,16 @@ async def delete_snapshot_tree(middleware, snapshot, logger=None): try: await middleware.call("zfs.snapshot.delete", snapshot, {"recursive": True}) return - except Exception: # noqa: BLE001 - fall through to the explicit sweep - pass + except Exception as e: # noqa: BLE001 - fall through to the explicit sweep + # Usually just "parent already gone" (stock's finally won the race once our + # mounts were released), which the sweep below handles. Log it rather than + # swallow it: if the real cause is something else, this is the only place + # it is visible -- the sweep would report a different, downstream failure. + if logger: + logger.debug( + "truecloud-patch: recursive delete of %s failed (%r); sweeping " + "the tree by name instead", snapshot, e, + ) # The parent may already be gone -- stock's `finally` can win the race once # our mounts are released -- which fails the recursive delete while the diff --git a/recover.sh b/recover.sh index 9b4fd5f..7a25a90 100755 --- a/recover.sh +++ b/recover.sh @@ -17,7 +17,7 @@ # bash /mnt/tank/truenas-truecloud-patch/patch/apply.sh # systemctl restart middlewared -VERSION="0.3.4" +VERSION="0.3.5" PATCH_DIR="$(cd "$(dirname "$0")" && pwd)" diff --git a/uninstall.sh b/uninstall.sh index bb5a7a7..173a256 100755 --- a/uninstall.sh +++ b/uninstall.sh @@ -3,7 +3,7 @@ set -euo pipefail -VERSION="0.3.4" +VERSION="0.3.5" PATCH_DIR="$(cd "$(dirname "$0")" && pwd)" _HOOK_COMMENT='TrueCloud provider patch (S3/B2)'