Repository navigation
The keyholder does not outlive the app it was unlocked for - #100
Merged
Merged
Conversation
When the app that spawned this daemon dies abnormally, the daemon is orphaned and keeps running. It is still holding the advisory lock on the key file, so the reader's next launch types their passphrase and is told another Notary already has this key open. Nothing cleared it but the idle exit, a quarter of an hour later. The lock itself is right, and so is the comment above it: the kernel drops it when the process ends, however it ends. A daemon that crashes releases the key. A daemon whose PARENT crashed does not, because it has not crashed, and nothing told it to go. What was wrong is the claim next to the park loop that this "process lives as long as its parent does". Nothing enforced that. It parked in a sleep loop and the only thing that could ever end it was a timer. So the parent is watched. Its pid is read at startup, while this is still single-threaded and still owned by its spawner, and the reaper that already runs every second compares it against getppid. Different means re-parented, which means the app this key was unlocked for is gone, and the daemon exits so the lock goes with it. A pid rather than the stdin pipe, because the pipe is not available: the spawner writes the secret and closes it immediately, which readSecretFromStdin depends on to know the secret ended. There is no channel left to notice an end on. Scoped by where it is read. Reaching that line means --approval-http was passed and a secret arrived on stdin, which is the started-by-an-app path. A daemon somebody runs from a terminal takes the headless route, never builds this server, records no parent, and is not watched: it must not exit because a shell did. Verified live rather than only in tests. With the parent up the daemon stays up and its ppid is the parent. SIGKILL the parent, which is what a segfault leaves behind, and the daemon is gone in about a second with "orphaned" in the audit log. Both unit tests were probed by reverting what they cover.
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Reported as: Plaza crashes, and then relaunching and entering the passphrase says "another Notary already has this key open."
What happens
When the app that spawned this daemon dies abnormally, the daemon is orphaned and keeps running. It is still holding the advisory lock on the key file, so the next launch is correctly told the key is open elsewhere. It is open elsewhere: in a process nobody can see and nobody asked for.
The lock is not the bug, and neither is the comment above it. The kernel drops an advisory lock when the process ends, however it ends, so a daemon that crashes releases the key. A daemon whose parent crashed does not, because it has not crashed and nothing told it to go. The only thing that ever cleared it was the idle exit, fifteen minutes later.
The wrong claim is next to the park loop:
Nothing enforced the first half. It parks in
while (true) io.sleep(3600)and the only thing that could end it was a timer.The fix
The parent is watched. Its pid is read at startup, while the process is still single-threaded and still owned by its spawner, and the reaper that already runs once a second compares it against
getppid(). Different means re-parented, which means the app this key was unlocked for is gone, so the daemon exits and the lock goes with it.A pid rather than the stdin pipe. The pipe would be the nicer mechanism and it is not available: the spawner writes the secret and closes it immediately, which
readSecretFromStdindepends on to know the secret ended. There is no channel left to notice an end on.Compared against the recorded parent, not against pid 1. An orphan goes to launchd on macOS and to init or a subreaper on Linux, and which one is not this daemon's business. What it knows is which process started it.
Scoped by where the pid is read. Reaching that line means
--approval-httpwas passed and a secret arrived on stdin, which is the started-by-an-app path. A daemon somebody runs from a terminal takes the headless route, never builds this server, records no parent, and is not watched. It must not exit because a shell did.Verified live, not only in tests
The good path matters as much as the bad one here: a daemon that exited the moment it started would be far worse than the bug, so the first half of that check is the point.
Both unit tests were probed by reverting what they cover.
parentGoneforced to false kills the orphan test; moving thegetppidread to after the first thread spawn kills the ordering test. 70/70 green.Not in this
The
notarywindow is spawned by the same app and is orphaned the same way. It is visible and closable, so it strands nothing, and it does not hold the key. Worth doing, separately.Pairs with zig-nostr/plaza#363, which fixes the crash that made this reachable.