From 23c0e6780d112d957455a428ef226268564ba75c Mon Sep 17 00:00:00 2001 From: lai0xn Date: Sun, 26 Jul 2026 18:23:24 +0100 Subject: [PATCH 1/3] Stop the shim silently going deaf mid-command Two paths made the shim stop journaling without any sign to the user, which is the worst failure mode this tool has: `undo` reports "nothing to undo" for changes it should have captured. rmdir() set the reentrancy guard on entry but only cleared it on the success path, so any failed rmdir (a non-empty directory, ENOENT) left in_shim set for the rest of the process. Everything that process did afterwards saw armed() == false and went unrecorded. `rmdir a b` with a non-empty `a` was enough to lose `b`. The mod dedup table was keyed on nothing, so it persisted across sessions in a process that outlives one command -- exactly the UNDO_CAPTURE_SHELL=1 setup, where the shim lives in the shell itself. A file written in one command suppressed its own backup in every later one. Reset the table when the session directory changes. The table now holds path hashes rather than strdup'd paths, so resetting it is a memset with nothing to free: a preload shim that mallocs inside an interposed open() is a hazard on its own, and freeing entries a second thread may be reading would be worse than the leak that avoiding the free implies. --- shim/undo_shim.c | 48 +++++++++++++++++++++++++++++++----------------- test/e2e.sh | 35 ++++++++++++++++++++++++++++++++++- 2 files changed, 65 insertions(+), 18 deletions(-) diff --git a/shim/undo_shim.c b/shim/undo_shim.c index 48cea6a..e4cd376 100644 --- a/shim/undo_shim.c +++ b/shim/undo_shim.c @@ -340,37 +340,53 @@ static int ignored(const char *abs) * otherwise save one backup per write. Only the first (pre-command) * backup is needed to restore, so we record which paths have been saved * and skip the rest. Best-effort: once the table fills we stop deduping, - * which only costs extra backups, never a missed one. */ + * which only costs extra backups, never a missed one. + * + * Slots hold path hashes, not the paths, so the table owns no memory: + * malloc inside an interposed open() is a hazard of its own, and with + * nothing to free there is nothing a second thread can pull out from + * under us when the session changes. */ #define DEDUP_CAP 16384 -static char *dedup_tab[DEDUP_CAP]; +static uint64_t dedup_tab[DEDUP_CAP]; static int dedup_count; +static char dedup_dir[PATH_MAX]; -static unsigned long path_hash(const char *s) +static uint64_t path_hash(const char *s) { - unsigned long h = 1469598103934665603UL; + uint64_t h = 1469598103934665603ULL; for (; *s; s++) { h ^= (unsigned char)*s; - h *= 1099511628211UL; + h *= 1099511628211ULL; } - return h; + return h ? h : 1; /* 0 marks an empty slot */ } /* returns 1 if `abs` was already saved this command (skip it); otherwise - * records it and returns 0. */ + * records it and returns 0. + * + * The table covers one command. A shell preloaded with the shim + * (UNDO_CAPTURE_SHELL=1) outlives every session it runs, so a path saved + * for an earlier command must not suppress its backup in the next one. */ static int mod_seen(const char *abs) { + const char *dir = session_dir(); + if (!dir) + return 0; + if (strcmp(dedup_dir, dir) != 0) { + memset(dedup_tab, 0, sizeof dedup_tab); + dedup_count = 0; + snprintf(dedup_dir, sizeof dedup_dir, "%s", dir); + } if (dedup_count * 4 >= DEDUP_CAP * 3) return 0; /* table nearly full: stop deduping, keep saving */ - unsigned long i = path_hash(abs) & (DEDUP_CAP - 1); + uint64_t h = path_hash(abs); + unsigned long i = h & (DEDUP_CAP - 1); while (dedup_tab[i]) { - if (strcmp(dedup_tab[i], abs) == 0) + if (dedup_tab[i] == h) return 1; i = (i + 1) & (DEDUP_CAP - 1); } - char *dup = strdup(abs); - if (!dup) - return 0; /* out of memory: save anyway */ - dedup_tab[i] = dup; + dedup_tab[i] = h; dedup_count++; return 0; } @@ -547,11 +563,9 @@ int rmdir(const char *path) int ok; handle_rmdir_pre(AT_FDCWD, path, abs, mode, &ok); int rc = real_rmdir(path); - if (rc == 0 && ok) { - in_shim = 1; + if (rc == 0 && ok) jwrite("rmdir", abs, mode, NULL); - in_shim = 0; - } + in_shim = 0; return rc; } diff --git a/test/e2e.sh b/test/e2e.sh index da37118..85c0141 100755 --- a/test/e2e.sh +++ b/test/e2e.sh @@ -204,7 +204,40 @@ else echo " (no cc, skipped)" fi -echo "== case 20: undo doctor passes its live self-test" +echo "== case 20: a failed rmdir does not stop the rest of the command being recorded" +mkdir -p "$PLAY/full/x" "$PLAY/gone" +run_armed "rmdir $PLAY/full $PLAY/gone || true" +[[ -d $PLAY/full && ! -d $PLAY/gone ]] || fail "rmdir did not run as expected" +"$UNDO" -y >/dev/null +[[ -d $PLAY/gone ]] || fail "second rmdir was not journaled after the first failed" + +# One long-lived process writing the same file under two sessions in a +# row: what UNDO_CAPTURE_SHELL=1 does, where the shim is loaded into the +# shell itself rather than into a fresh child per command. +if command -v python3 >/dev/null 2>&1; then + echo "== case 21: one process spanning two sessions backs up in both" + f=$PLAY/shared.txt + echo v0 >"$f" + s1=$UNDO_DATA_DIR/sessions/$(date +%s%N | cut -c1-16); mkdir -p "$s1/data" + echo "write v1" >"$s1/cmd"; sleep 0.01 + s2=$UNDO_DATA_DIR/sessions/$(date +%s%N | cut -c1-16); mkdir -p "$s2/data" + echo "write v2" >"$s2/cmd" + LD_PRELOAD="$LIB" python3 -c " +import os, sys +for sess, body in ((sys.argv[1], 'v1'), (sys.argv[2], 'v2')): + os.environ['UNDO_SESSION'] = sess + with open(sys.argv[3], 'w') as fh: + fh.write(body + '\n') +" "$s1" "$s2" "$f" + [[ $(cat "$f") == v2 ]] || fail "writes did not run" + grep -q "shared.txt" "$s2/journal" 2>/dev/null || fail "second session recorded nothing" + "$UNDO" -y >/dev/null + [[ $(cat "$f") == v1 ]] || fail "second session did not restore v1, got $(cat "$f")" +else + echo "== case 21: skipped (no python3)" +fi + +echo "== case 22: undo doctor passes its live self-test" out=$("$UNDO" doctor 2>&1) || fail "doctor exited non-zero: $out" grep -q "\[ok \] capture" <<<"$out" || fail "doctor capture check did not pass" grep -q "\[ok \] restore" <<<"$out" || fail "doctor restore check did not pass" From e15ac7f9541dffadca4d04fc2e077e7b624c8d93 Mon Sep 17 00:00:00 2001 From: lai0xn Date: Wed, 29 Jul 2026 11:47:23 +0100 Subject: [PATCH 2/3] Report a signal death from `undo run` as 128+signal exec.ExitError.ExitCode() is -1 when the child was killed by a signal, and os.Exit(-1) becomes a bare 255, so `undo run -- ` was not the transparent wrapper it looks like. Report what a shell would. --- cmd/undo/run.go | 6 ++++++ test/e2e.sh | 9 ++++++++- 2 files changed, 14 insertions(+), 1 deletion(-) diff --git a/cmd/undo/run.go b/cmd/undo/run.go index 1a03177..7f14c91 100644 --- a/cmd/undo/run.go +++ b/cmd/undo/run.go @@ -6,6 +6,7 @@ import ( "os/exec" "path/filepath" "strings" + "syscall" "github.com/edaywalid/undo/internal/session" ) @@ -105,6 +106,11 @@ func cmdRun(argv []string) { if runErr != nil { if ee, ok := runErr.(*exec.ExitError); ok { code = ee.ExitCode() + // killed by a signal: ExitCode is -1, which os.Exit turns + // into 255. Report it the way a shell does. + if ws, ok := ee.Sys().(syscall.WaitStatus); ok && ws.Signaled() { + code = 128 + int(ws.Signal()) + } } else { s.Remove() fatal(runErr) diff --git a/test/e2e.sh b/test/e2e.sh index 85c0141..3ffbed9 100755 --- a/test/e2e.sh +++ b/test/e2e.sh @@ -237,7 +237,14 @@ else echo "== case 21: skipped (no python3)" fi -echo "== case 22: undo doctor passes its live self-test" +echo "== case 22: undo run reports a signal death the way a shell does" +set +e +"$UNDO" run -- sh -c 'kill -TERM $$' >/dev/null 2>&1 +rc=$? +set -e +[[ $rc == 143 ]] || fail "expected 143 from a SIGTERM death, got $rc" + +echo "== case 23: undo doctor passes its live self-test" out=$("$UNDO" doctor 2>&1) || fail "doctor exited non-zero: $out" grep -q "\[ok \] capture" <<<"$out" || fail "doctor capture check did not pass" grep -q "\[ok \] restore" <<<"$out" || fail "doctor restore check did not pass" From eb44911244a4a1f4d6c3bb0c476fc889d7dc19df Mon Sep 17 00:00:00 2001 From: edaywalid Date: Wed, 29 Jul 2026 15:34:06 +0100 Subject: [PATCH 3/3] shim: note what the hash-keyed dedup table trades away The table can now lose a backup rather than only add one, which the comment did not say. The odds are negligible and the trade is right, but it should be written down next to the code that makes it. --- shim/undo_shim.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/shim/undo_shim.c b/shim/undo_shim.c index e4cd376..129a0e5 100644 --- a/shim/undo_shim.c +++ b/shim/undo_shim.c @@ -345,7 +345,12 @@ static int ignored(const char *abs) * Slots hold path hashes, not the paths, so the table owns no memory: * malloc inside an interposed open() is a hazard of its own, and with * nothing to free there is nothing a second thread can pull out from - * under us when the session changes. */ + * under us when the session changes. + * + * What that costs: two paths sharing a 64-bit hash dedup as one and the + * second goes unsaved. Around 1e-10 for a command touching 100k distinct + * paths, against allocating inside open() on every write. Worth it, but + * it is the one way this table can lose a backup rather than add one. */ #define DEDUP_CAP 16384 static uint64_t dedup_tab[DEDUP_CAP]; static int dedup_count;