diff --git a/crates/sysknife-brain/src/planning_tools/propose_plan.rs b/crates/sysknife-brain/src/planning_tools/propose_plan.rs index 48d34165..493968f5 100644 --- a/crates/sysknife-brain/src/planning_tools/propose_plan.rs +++ b/crates/sysknife-brain/src/planning_tools/propose_plan.rs @@ -194,7 +194,7 @@ this is runtime status, NOT the saved configuration; on Ubuntu the saved config ("AddSwap", "create a swap file, enable it, and persist to /etc/fstab — params: file* (absolute path), size_mb* (integer MB); High risk"), ("RemoveSwap", - "disable a swap file, remove it, and drop its /etc/fstab entry — param: file* (must already be a swap file per /proc/swaps or /etc/fstab, and not a symlink); High risk"), + "disable and remove a swap file after swapoff succeeds, then drop its /etc/fstab entry; if swapoff fails while it remains active or active state cannot be read, refuse and preserve both — param: file* (must already be a swap file per /proc/swaps or /etc/fstab, and not a symlink); High risk"), // apt pinning (preferences.d) — Ubuntu/Debian only ("GetAptPins", "show apt pin priorities (apt-cache policy) — param: package (optional); read-only"), diff --git a/docs/action-reference.md b/docs/action-reference.md index 1a8593ab..96a884f9 100644 --- a/docs/action-reference.md +++ b/docs/action-reference.md @@ -127,7 +127,7 @@ Every row is derived from the live code: the command from each action's `ActionS | `AddMount` | `sudo /usr/lib/sysknife/mount-edit --op mount --device /dev/sdb1 --mountpoint /mnt/data --fstype ext4 --options defaults` | High | All | – | – | mount a device and persist it to /etc/fstab with nofail — params: device\* (/dev/.., UUID=.., LABEL=.., //host/share, or host:/export), mountpoint\* (absolute; not a system dir), fstype\* (ext4/xfs/btrfs/vfat/nfs/cifs/…), options (csv, optional); High risk | | `RemoveMount` | `sudo /usr/lib/sysknife/mount-edit --op unmount --mountpoint /mnt/data` | High | All | – | – | unmount and drop the /etc/fstab entry for a mountpoint — param: mountpoint\*; High risk | | `AddSwap` | `sudo /usr/lib/sysknife/mount-edit --op addswap --file /swapfile --size-mb 2048` | High | All | – | – | create a swap file, enable it, and persist to /etc/fstab — params: file\* (absolute path), size_mb\* (integer MB); High risk | -| `RemoveSwap` | `sudo /usr/lib/sysknife/mount-edit --op rmswap --file /swapfile` | High | All | – | – | disable a swap file, remove it, and drop its /etc/fstab entry — param: file\* (must already be a swap file per /proc/swaps or /etc/fstab, and not a symlink); High risk | +| `RemoveSwap` | `sudo /usr/lib/sysknife/mount-edit --op rmswap --file /swapfile` | High | All | – | – | disable and remove a swap file after swapoff succeeds, then drop its /etc/fstab entry; if swapoff fails while it remains active or active state cannot be read, refuse and preserve both — param: file\* (must already be a swap file per /proc/swaps or /etc/fstab, and not a symlink); High risk | ## Log management diff --git a/packaging/sysknife-mount-edit b/packaging/sysknife-mount-edit index 1651c66d..ed1be0a7 100755 --- a/packaging/sysknife-mount-edit +++ b/packaging/sysknife-mount-edit @@ -400,6 +400,23 @@ def op_addswap(args) -> None: write_fstab(append_fstab_entry(lines, entry)) +def active_swap_files(): + """Absolute paths currently active as swap FILES, excluding partitions.""" + active = set() + try: + with open(PROC_SWAPS, "r") as fh: + next(fh, None) # header: Filename Type Size Used Priority + for line in fh: + fields = line.split() + if len(fields) >= 2 and fields[1] == "file": + active.add(fields[0]) + except OSError: + # A missing or unreadable proc file cannot prove that a swap path is + # inactive. known_swap_files still uses fstab to identify candidates. + return None + return active + + def known_swap_files() -> set: """Absolute paths this host actually knows as swap FILES. @@ -408,17 +425,8 @@ def known_swap_files() -> set: is configured to have). Swap *partitions* are excluded from both — they are block devices, never something to unlink — as are commented-out fstab lines. """ - known = set() - try: - with open(PROC_SWAPS, "r") as fh: - next(fh, None) # header: Filename Type Size Used Priority - for line in fh: - fields = line.split() - if len(fields) >= 2 and fields[1] == "file": - known.add(fields[0]) - except OSError: - # No /proc/swaps (a container, a test harness): fstab alone decides. - pass + active = active_swap_files() + known = active if active is not None else set() for line in read_fstab(): if line.lstrip().startswith("#"): continue @@ -446,7 +454,18 @@ def op_rmswap(args) -> None: # have accepted the link as swap in the first place. if os.path.islink(path): die(f"{path!r} is a symlink, not a swap file; refusing to remove it") - subprocess.run([SWAPOFF, path], check=False, stderr=subprocess.DEVNULL) + swapoff = subprocess.run([SWAPOFF, path], check=False, stderr=subprocess.PIPE) + if swapoff.returncode != 0: + active = active_swap_files() + if active is None or path in active: + detail = swapoff.stderr.decode(errors="replace").strip() + message = ( + f"swapoff failed for active or unverifiable swap file {path!r}; " + "leaving the file and fstab entry intact" + ) + if detail: + message += f": {detail}" + die(message) lines = read_fstab() kept = remove_fstab_lines(lines, fstab_field1, path) if len(kept) != len(lines): diff --git a/tests/release/rmswap.test.sh b/tests/release/rmswap.test.sh index 1d0ab18d..3ac1a54d 100755 --- a/tests/release/rmswap.test.sh +++ b/tests/release/rmswap.test.sh @@ -15,7 +15,9 @@ SCRIPT="$ROOT/packaging/sysknife-mount-edit" [ -f "$SCRIPT" ] || { echo "FAIL: $SCRIPT not found"; exit 1; } python3 - "$SCRIPT" <<'PY' +import contextlib import importlib.util +import io import os import sys import tempfile @@ -43,20 +45,31 @@ class Args: class FakeCompleted: - returncode = 0 - stderr = b"" + def __init__(self, returncode=0, stderr=b""): + self.returncode = returncode + self.stderr = stderr -def run_rmswap(path): - """Run op_rmswap with swapoff stubbed out. Returns the SystemExit code, or - None if it ran to completion.""" +def run_rmswap(path, swapoff_returncode=0, swapoff_stderr=b""): + """Run op_rmswap with swapoff stubbed out; return its exit code and stderr.""" real_run = mod.subprocess.run - mod.subprocess.run = lambda *a, **k: FakeCompleted() + result = FakeCompleted(swapoff_returncode, swapoff_stderr) + + def fake_run(*args, **kwargs): + if kwargs.get("stderr") == mod.subprocess.PIPE: + return result + return FakeCompleted(swapoff_returncode, None) + + mod.subprocess.run = fake_run + captured_stderr = io.StringIO() try: - mod.op_rmswap(Args(path)) - return None - except SystemExit as exc: - return exc.code + with contextlib.redirect_stderr(captured_stderr): + try: + mod.op_rmswap(Args(path)) + code = None + except SystemExit as exc: + code = exc.code + return code, captured_stderr.getvalue() finally: mod.subprocess.run = real_run @@ -67,21 +80,24 @@ with tempfile.TemporaryDirectory() as d: victim = os.path.join(d, "shadow") declared = os.path.join(d, "declared-swap") active = os.path.join(d, "active-swap") + active_only = os.path.join(d, "active-only-swap") with open(victim, "w") as fh: fh.write("root:!:19000:0:99999:7:::\n") - for p in (declared, active): + for p in (declared, active, active_only): with open(p, "w") as fh: fh.write("swapdata") with open(fstab, "w") as fh: fh.write("UUID=1234\t/\text4\tdefaults\t0\t1\n") fh.write(f"{declared}\tnone\tswap\tsw,nofail\t0\t0\n") + fh.write(f"{active}\tnone\tswap\tsw,nofail\t0\t0\n") # A commented-out entry must not make a path "known". fh.write(f"#{victim}\tnone\tswap\tsw,nofail\t0\t0\n") with open(swaps, "w") as fh: fh.write("Filename\t\t\t\tType\t\tSize\t\tUsed\t\tPriority\n") fh.write(f"{active}\t\t\t\tfile\t\t2097148\t\t0\t\t-2\n") + fh.write(f"{active_only}\t\t\t\tfile\t\t2097148\t\t0\t\t-1\n") fh.write("/dev/sda2\t\t\t\tpartition\t2097148\t\t0\t\t-3\n") mod.FSTAB = fstab @@ -94,13 +110,17 @@ with tempfile.TemporaryDirectory() as d: failures.append("known_swap_files missed the fstab-declared swap file") if active not in known: failures.append("known_swap_files missed the active /proc/swaps file") + if active_only not in known: + failures.append("known_swap_files missed the /proc/swaps-only file") + if any(mod.fstab_field1(line) == active_only for line in mod.read_fstab()): + failures.append("/proc/swaps-only test file unexpectedly appeared in fstab") if victim in known: failures.append("known_swap_files treated a commented-out fstab line as a swap file") if "/dev/sda2" in known: failures.append("known_swap_files treated a swap partition as a removable swap file") # 2. THE BUG: an unrelated root-owned file must not be deleted. - code = run_rmswap(victim) + code, _ = run_rmswap(victim) if code in (None, 0): failures.append(f"op_rmswap did NOT refuse an unknown path (exit {code!r})") if not os.path.exists(victim): @@ -113,24 +133,73 @@ with tempfile.TemporaryDirectory() as d: os.symlink(victim, link) with open(fstab, "a") as fh: fh.write(f"{link}\tnone\tswap\tsw,nofail\t0\t0\n") - code = run_rmswap(link) + code, _ = run_rmswap(link) if code in (None, 0): failures.append(f"op_rmswap did NOT refuse a symlinked swap path (exit {code!r})") - # 4. The action must still work: a genuine, declared swap file is removed. - code = run_rmswap(declared) + # 4. A stale fstab entry is not active swap. A failed swapoff is expected + # here, so the file and its declaration can still be removed. + code, _ = run_rmswap( + declared, + swapoff_returncode=1, + swapoff_stderr=b"not an active swap area", + ) if code not in (None, 0): failures.append(f"op_rmswap refused a legitimate declared swap file (exit {code!r})") if os.path.exists(declared): failures.append("op_rmswap did not remove a legitimate declared swap file") + if any(mod.fstab_field1(line) == declared for line in mod.read_fstab()): + failures.append("op_rmswap kept the fstab entry for an inactive swap file") + + # 5. A failed swapoff for an active file must preserve both the file and its + # fstab entry and surface the utility's stderr. + code, message = run_rmswap( + active, + swapoff_returncode=1, + swapoff_stderr=b"device is busy", + ) + if code in (None, 0): + failures.append(f"op_rmswap did NOT refuse a failed swapoff for an active file (exit {code!r})") + if not os.path.exists(active): + failures.append("op_rmswap DELETED an active swap file after swapoff failed") + if not any(mod.fstab_field1(line) == active for line in mod.read_fstab()): + failures.append("op_rmswap removed the fstab entry after swapoff failed") + if "device is busy" not in message: + failures.append("op_rmswap did not include swapoff stderr in its refusal") + + # 6. If /proc/swaps cannot be read, a failed swapoff is not evidence that + # the active path is safe to remove. Preserve the file and fstab entry. + mod.PROC_SWAPS = os.path.join(d, "missing-swaps") + code, message = run_rmswap( + active, + swapoff_returncode=1, + swapoff_stderr=b"cannot inspect active swaps", + ) + if code in (None, 0): + failures.append(f"op_rmswap treated unreadable /proc/swaps as inactive (exit {code!r})") + if not os.path.exists(active): + failures.append("op_rmswap deleted a swap file when it could not inspect /proc/swaps") + if not any(mod.fstab_field1(line) == active for line in mod.read_fstab()): + failures.append("op_rmswap removed the fstab entry when /proc/swaps was unreadable") + if "cannot inspect active swaps" not in message: + failures.append("op_rmswap did not include swapoff stderr when /proc/swaps was unreadable") - # 5. ...and so is one that is only known because it is currently active. - code = run_rmswap(active) + mod.PROC_SWAPS = swaps + + # 7. A successful swapoff still removes a genuinely active file. + code, _ = run_rmswap(active) if code not in (None, 0): failures.append(f"op_rmswap refused a legitimate active swap file (exit {code!r})") if os.path.exists(active): failures.append("op_rmswap did not remove a legitimate active swap file") + # 8. A file known only from /proc/swaps (no fstab line) is still removable. + code, _ = run_rmswap(active_only) + if code not in (None, 0): + failures.append(f"op_rmswap refused a swap file known only from /proc/swaps (exit {code!r})") + if os.path.exists(active_only): + failures.append("op_rmswap did not remove a swap file known only from /proc/swaps") + if failures: for f in failures: print("FAIL:", f)