Stop the shim silently going deaf mid-command - #3
Conversation
|
Verified each change independently:
Nice that the One thing before merging: Two smaller things:
|
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.
exec.ExitError.ExitCode() is -1 when the child was killed by a signal, and os.Exit(-1) becomes a bare 255, so `undo run -- <cmd>` was not the transparent wrapper it looks like. Report what a shell would.
b6c0a20 to
e15ac7f
Compare
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.
edaywalid
left a comment
edaywalid
left a comment
There was a problem hiding this comment.
Verified each change by reverting it alone and rerunning. Without the rmdir fix, case 20 fails with "nothing to undo". Without the dedup keying, case 21 fails with "second session recorded nothing". Case 22 covers the signal exit code.
The rework to hash slots is better than what I was going to suggest. It drops the use-after-free window when the session changes mid-process, and it takes strdup out of an interposed open(), so the shim now has no malloc, strdup or free anywhere. glibc floor is unchanged at 2.34.
I pushed one commit on top: a comment noting that hash slots mean the table can now lose a backup rather than only add one. The odds are around 1e-10 for a command touching 100k distinct paths and the trade is clearly right, it just wasn't written down.
Thanks for splitting the exit code change out and rebasing.
2 participants
Two bugs where the shim stops journaling with no sign to the user, so
undosays "nothing to undo" for changes it should have captured.rmdir()leaks the reentrancy guard. It setsin_shim = 1on entry but only clears it on the success path, so any failed rmdir leaves it set for the rest of the process — every later unlink/open/rename seesarmed() == falseand goes unrecorded.rmdir a bwith a non-emptyais enough to loseb. Every other interposed function clears it on all paths.The mod dedup table never resets between sessions. It's process-global with no session key. Fine for a fresh child per command, wrong under
UNDO_CAPTURE_SHELL=1where the shim lives in the shell across every command: a file backed up once is skipped forever after, so later edits are unrecoverable. Now keyed on the session dir.Minor:
undo runreturned 255 for signal deaths (ExitCode()is -1); now 128+signal.e2e cases 19 and 20 both fail on the old shim. Case 20 uses python3 to drive the one-process-two-sessions path, since bash's
exportdoesn't reliably update the Cenvironthe shim reads; it skips where python3 is absent so the musl job doesn't break.