From 67d235262393d9c6fa4dcb54a66579139eda8c70 Mon Sep 17 00:00:00 2001 From: adamXbot <111877622+adamXbot@users.noreply.github.com> Date: Thu, 6 Aug 2026 22:50:46 +1000 Subject: [PATCH] fix: a pid-less lock must not silently kill the run Release verification of v0.4.0 caught two defects in the lock hardening that shipped in c045cc0, both mine. acquire_lock read the recorded owner with an unguarded al_pid=$(cat "$LOCK_DIR/pid" 2>/dev/null) An assignment from a failing command substitution takes that failure as its own status, and acquire_lock is invoked as the last command of an AND-OR list, where errexit is not suppressed - so a lock directory with no readable pid killed the script instantly. Reproduced: `stop` exited 1 having printed nothing at all on stdout or stderr, and `doctor` stopped mid-report after the disk-space check. That state is reachable by a run killed between mkdir and the pid write, or by a disk full at that moment, and it made the folder look inert until someone found the lock by hand. The read is now guarded and a lock with no owner is reported rather than either aborting or being stolen - with no pid there is nothing to judge. Liveness also used `kill -0`, which returns "not permitted" rather than "no such process" for a process owned by another user. On a shared group-writable folder that made someone else's live run look dead: the second run announced it was clearing a stale lock and proceeded concurrently, where v0.3.0 correctly refused. It now asks `ps` first, which can see other users' processes, and falls back to `kill -0` where ps has no -p. The unguarded `rm -rf "$LOCK_DIR"` that aborted before its own error message when the lock belonged to root is guarded too. Also corrects text that would have been published as the release notes: 0.4.0 is the first release to carry a checksum, not 0.4.1. Both defects have regression tests; reverting manage.sh alone fails 4 of them. Co-Authored-By: Claude Opus 5 --- CHANGELOG.md | 14 +++++++++--- docker_volumes/manage.sh | 49 +++++++++++++++++++++++++++++----------- tests/run-tests.sh | 23 ++++++++++++++++++- 3 files changed, 69 insertions(+), 17 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2211ca8..d18033f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -27,7 +27,9 @@ dead on the first broken app and said almost nothing about the rest. (`.dockerdance.lock`), so another local user can no longer pre-create the predictable name and permanently lock the tool. It records the owning pid, and a lock whose process is gone clears itself instead of demanding manual - removal after a crash or reboot. + removal after a crash or reboot. Liveness is checked with `ps` rather than + `kill -0`, which cannot see another user's process and would otherwise + declare their running job dead and take the lock from under it. - **Webhook payloads escape their content.** An app name or error message containing a quote, backslash, tab or newline no longer breaks (or rewrites) the JSON sent to `NOTIFY_WEBHOOK`. @@ -56,8 +58,9 @@ dead on the first broken app and said almost nothing about the rest. many older archives exist. - **Releases carry a checksum, and `update-self` verifies it.** The release workflow publishes `manage.sh.sha256` beside the notes; `update-self` - checks the downloaded script against it before installing (releases - without one - everything before 0.4.1 - still install as before). + checks the downloaded script against it before installing (0.4.0 is the + first release to carry one; older releases without it still install as + before). `update-self` also now tells a GitHub rate-limit apart from being offline. - **A test suite.** `tests/run-tests.sh` drives `manage.sh` against a stub docker (no daemon needed) and asserts on exit codes, output and the exact @@ -106,6 +109,11 @@ dead on the first broken app and said almost nothing about the rest. - A `PARALLEL_PULLS` of `0`, a negative or a typo left the pull orchestrator with no slot it could ever fill, hanging the run; it now falls back to the default of 3. +- A lock directory with no readable pid inside - left by a run killed between + creating the lock and recording its owner, or by a full disk - made every + state-changing command exit 1 with no output whatsoever, and cut `doctor`'s + report off mid-way. Such a lock is now reported (and left alone, since + there is no owner to judge) instead of aborting the script. ## [0.3.0] - 2026-07-13 diff --git a/docker_volumes/manage.sh b/docker_volumes/manage.sh index c502ed8..030b82f 100755 --- a/docker_volumes/manage.sh +++ b/docker_volumes/manage.sh @@ -226,24 +226,45 @@ run_step() { return 0 } +#Is process $1 still running? ps is asked first because it can see other +#users' processes: kill -0 only reports "not permitted" for those, which +#would make someone else's live run look dead and let us steal their lock. +#Where ps has no -p (busybox) this falls back to kill -0. +pid_alive() { + ps -p "$1" >/dev/null 2>&1 && return 0 + kill -0 "$1" 2>/dev/null +} + +#Read the pid recorded in the lock, or "" when there isn't a readable one. +#The `|| true` matters: an unguarded assignment from a failing command +#substitution aborts the whole script under set -e, and acquire_lock is +#called as the last command of an AND-OR list, where errexit still applies. +lock_pid() { + lp=$(cat "$LOCK_DIR/pid" 2>/dev/null) || lp="" + printf '%s' "$lp" +} + acquire_lock() { [ -n "$HAVE_LOCK" ] && return 0 if ! mkdir "$LOCK_DIR" 2>/dev/null; then #A lock whose recorded process is gone is left over from a crash or a - #reboot - clear it and try once more. (kill -0 can't tell a foreign - #user's live process from a dead one, but the lock sits inside this - #user's own docker_volumes folder, so that ambiguity doesn't arise.) - al_pid=$(cat "$LOCK_DIR/pid" 2>/dev/null) - if [ -n "$al_pid" ] && ! kill -0 "$al_pid" 2>/dev/null; then + #reboot - clear it and try once more. A lock with no readable pid can't + #be judged, so it's left alone and reported rather than stolen. + al_pid=$(lock_pid) + if [ -n "$al_pid" ] && ! pid_alive "$al_pid"; then warn "Clearing a stale lock left by exited run (pid $al_pid)" - rm -rf "$LOCK_DIR" + rm -rf "$LOCK_DIR" 2>/dev/null || true fi if ! mkdir "$LOCK_DIR" 2>/dev/null; then - error "Another manage.sh run is active here (lock: $LOCK_DIR, pid ${al_pid:-unknown})." + if [ -n "$al_pid" ]; then + error "Another manage.sh run is active here (lock: $LOCK_DIR, pid $al_pid)." + else + error "A lock with no owner recorded is in the way: $LOCK_DIR - remove it if no run is active." + fi exit 1 fi fi - echo "$$" >"$LOCK_DIR/pid" + echo "$$" >"$LOCK_DIR/pid" 2>/dev/null || true HAVE_LOCK=1 } @@ -1058,7 +1079,7 @@ backup_app() { #Newest-first list of $1's own backup archives. The name after the app part #must be exactly a date (with an optional _HHMMSS), in either the current -#'app_YYYY-MM-DD' form or the pre-0.4.1 'appYYYY-MM-DD' form - so an app +#'app_YYYY-MM-DD' form or the pre-0.4.0 'appYYYY-MM-DD' form - so an app #whose name extends another's (vault / vault2) never matches its neighbour's #archives. A bare glob did, which let BACKUP_KEEP prune the wrong app's #backups and restore pick the wrong archive. @@ -1401,11 +1422,13 @@ run_doctor() { fi fi if [ -d "$LOCK_DIR" ]; then - dr_lockpid=$(cat "$LOCK_DIR/pid" 2>/dev/null) - if [ -n "$dr_lockpid" ] && kill -0 "$dr_lockpid" 2>/dev/null; then + dr_lockpid=$(lock_pid) + if [ -n "$dr_lockpid" ] && pid_alive "$dr_lockpid"; then dwarn "A run lock is held by an active run (pid $dr_lockpid)" - else + elif [ -n "$dr_lockpid" ]; then dwarn "A stale run lock exists ($LOCK_DIR) - the next state-changing command will clear it" + else + dwarn "A lock with no owner recorded exists ($LOCK_DIR) - remove it if no run is active" fi else dok "No stale run lock" @@ -1491,7 +1514,7 @@ update_self() { error "The downloaded script failed a syntax check - not installing it." exit 1 fi - #Releases from v0.4.1 publish a checksum next to the script; verify when + #Releases from v0.4.0 publish a checksum next to the script; verify when #it's there (and this host can hash). Older releases simply don't have one. expected_sha=$(fetch_url "https://github.com/$SELF_REPO/releases/download/$latest_tag/manage.sh.sha256" 2>/dev/null | awk 'NR==1{print $1}') || expected_sha="" if [ -n "$expected_sha" ]; then diff --git a/tests/run-tests.sh b/tests/run-tests.sh index feeff2c..9e159a1 100755 --- a/tests/run-tests.sh +++ b/tests/run-tests.sh @@ -170,7 +170,7 @@ assert_no_action "UP:alpha" begin "archives: prefix apps (vault/vault2) never cross - prune and restore" apps vault vault2 mkdir -p "$SANDBOX/backup" -#vault's own legacy no-separator archive (pre-0.4.1), and vault2's newer one +#vault's own legacy no-separator archive (pre-0.4.0), and vault2's newer one echo v1 | tar -cjf "$SANDBOX/backup/vault2026-08-01.tar.bz2" -T /dev/null 2>/dev/null || : >"$SANDBOX/backup/vault2026-08-01.tar.bz2" sleep 1 : >"$SANDBOX/backup/vault22026-08-03.tar.bz2" @@ -283,6 +283,27 @@ assert_out "unhealthy: beta" assert_out "Updated 1 of 2 apps" assert_action "UP:beta" #it did start - health is what failed +begin "lock: a pid-less lock is reported, never silently fatal" +#A run killed between mkdir and the pid write (or a full disk) leaves a lock +#with no owner recorded. That must not abort the script under set -e with no +#output at all, and must not be stolen either - we can't tell if it's live. +apps alpha +printf 'alpha\n' >"$DD_STATE/up" +mkdir "$SANDBOX/.dockerdance.lock" +run stop alpha +assert_status 1 +assert_out "no owner recorded" +assert_no_action "STOP:alpha" +#doctor must still print its whole report rather than stopping mid-way +run doctor +assert_status 0 +assert_out "no owner recorded" +assert_out "settings:" #the section printed after the lock check +rmdir "$SANDBOX/.dockerdance.lock" +run stop alpha #and the folder works again once cleared +assert_status 0 +assert_action "STOP:alpha" + begin "missing app folder: reported, run continues" apps alpha gamma