Repository navigation
fix: keep a published Unix socket's identity through startup failure - #112
Merged
Merged
Conversation
This comment has been minimized.
This comment has been minimized.
added 3 commits
September 30, 2026 12:39
A review of #109 found two startup ownership gaps. The native HTTPS owner discarded the identity listenPublishedUnixSocket returned and only recorded one after lstat, chmod and lstat of the path. A failure after publication then skipped listener and endpoint cleanup, and a replacement in that window could be chmodded or recorded as the endpoint. The owner now keeps the published identity at once, compares its first observation against it, and never changes the endpoint by path. Failure cleanup always closes the listener, which can no longer unlink anything but the retired staging name, and then removes the endpoint only while it is this listener's inode; a replacement is kept. The helper awaited listen() outside its cleanup, so a listen that bound the staging name and then failed could leak that socket and listener. Listening is now inside the cleanup, and a failed start removes only a staging socket this user created. The helper also sets the endpoint mode (0600) on the staging inode before publication, so the MCP backend, the HTTPS owner and the owner challenge no longer chmod the public path. Controls: bind-then-fail, mode-preparation failure, an endpoint at the AF_UNIX path limit (and one byte over: refused cleanly on Bun 1.3.9, bound in full on 1.4.2, never truncated), and an owner fault or replacement between publication and first observation, on Bun 1.3.9 and 1.4.2.
The private-bind fixture hooked chmod on the public mcp.sock to prove the socket is private before its mode is set and that the creation mask is restored. Since the endpoint's mode is now set on its staging socket before publication, the hook never fired and the check failed. It now observes that staging chmod with the same privacy and mask checks, requires mode 0600, and fails if the published endpoint is ever chmodded by path.
Stand-in invocations ran concurrently (a foreground `up` while the driver polls `ps`) and each did an unlocked read-modify-write of state.json. A `ps` that loaded the state before the second `up` saved its foreground token then saved its stale copy over it, so `up` saw its token gone and exited 0 before readiness; lost call records failed the acceptance test as well. That failed 26 of 40 local runs, and Runtime state models on CI. Each invocation now holds an exclusive flock across its read-modify-write and releases it only before its long waits (the foreground loop and the ps-hang fault). 40 of 40 local runs pass.
roodboi
force-pushed
the
claude/hack-1211-socket-startup-ownership
branch
from
September 30, 2026 16:53
ea0e0d0 to
70c78f7
Compare
added 2 commits
September 30, 2026 13:31
…a socket A second review of the Unix-socket publication found three staging-path windows where an entry this attempt could not prove was its own could be changed or removed: - After an ambiguous partial bind (no recorded identity), cleanup unlinked any same-uid socket at the staging name. - The mode was set by path after an awaited lstat, so a replacement in that gap could be chmodded before the postcheck refused it. - On success the staging name was unlinked unconditionally after the awaited link and endpoint lstat. The socket is now created with exactly its mode (the umask during the synchronous bind), so nothing is ever chmodded, and its identity is recorded in the same tick. Publication links only while the staging name is still that socket, and retirement removes it only while it is (check and removal back to back). An entry that is not this socket, or cannot be proven to be, is never removed: on failure it is moved aside while the server closes, so the runtime's close-time unlink by name cannot reach it, and then put back with the same inode. The owner challenge's chmod dependency becomes an afterOwnerSocketPublish test seam, and the private-bind fixture now requires the socket to be created private with mode 0600 and no socket name to be chmodded.
…ng entry The failure close moved an unproven staging entry to one random holding name: if that name was occupied the close went ahead unprotected, and a holding entry created between the check and the rename was overwritten. The entry is now moved with link(2), which never replaces a holding entry, over a bounded list of fresh holding names, and the staging name is dropped only while it is still the linked entry. An entry that cannot be moved aside (every holding name occupied, or not hard-linkable) leaves the close to proceed, now documented as a residual. The helper's documentation now states its scope: accidental and concurrent entries in a private directory, not an adversarial process of the same user, with the remaining path-based and post-return residuals.
This was referenced Sep 30, 2026
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.
Summary
This PR closes startup ownership gaps in Unix-socket publication that reviews of hack-dance/hack#109 found (HACK-1211). It is based on
nextata7d09599. hack-dance/hack#108 (the Bun 1.4.2 pin) is held as a draft until this lands.This summary describes the current head,
83571a61. The sections below record each review round in order; the first round'schmod-based mode handling was replaced in the second round.1. The native HTTPS owner keeps its published identity through startup (
src/backends/native-https-owner-server.ts)Before, the owner discarded the identity
listenPublishedUnixSocketreturned. It recordedsocketIdentityonly afterlstat,chmod,lstatofcontrol.sock.failOwnerneedssocketIdentity). The public link and the listener stayed alive.chmoded and compared only to itself, so a foreign endpoint could be recorded.Now:
0600.chmoded: the socket is created with mode0600.afterPublishis a documented optional test seam between publication and first observation; production passes none. The owner challenge (src/backends/native-project-https.ts) has the same seam asafterOwnerSocketPublish.2. The publication helper (
src/lib/unix-socket-publish.ts)listen()is inside the cleanup, so a runtime that binds the staging name and then fails leaves no listener. The helper removes only this attempt's socket, by recorded identity. An entry it cannot prove is its own (an ambiguous partial bind) is not removed by the helper.chmodanything.EEXIST). The staging name is retired only while it is still this socket.link. Holding names are tried by no-clobberlinkover 8 fresh names, never replacing a holding entry.Scope: the staging-name guarantees are best-effort within a directory private to this user. They cover accidental and concurrent entries, not an adversarial process of the same user. Residuals:
Public endpoint contract unchanged: same paths, socket, mode
0600, recorded device/inode; published without replacing an existing file, removed only by identity, neverchmoded.Verification at
83571a61Refs HACK-1211.
Review history
First round (
67891ff6), superseded in partThe first round kept a helper-side
chmodof the staging inode (injectablesetMode). The second round (5d99981d) replaced it with creation-time mode. The first round's controls are carried forward:Its mutation checks were: the identity recorded after the first observation, a public
chmod, paths checked before the listener closes, and no cleanup after bind-then-fail.CI at
67891ff6and fix (ea0e0d07)67891ff6, seven checks and Graphite passed.testfailed 1 of 1,771 tests, with no Bun crash:tests/mcp-publication-readiness.test.ts, "socket is private before chmod without changing later file permissions".tests/fixtures/mcp/private-bind.tshookedchmodon the publicmcp.sockto prove the socket is private before its mode is set and that the creation mask is restored. This PR moves thatchmodonto the staging socket before publication, so the hook never fired.chmodwith the same privacy and mask checks (since the second round it observes creation mode throughlstatand refuses anychmod);0600;chmoded by path.chmod("chmodded by path"), no private umask at bind ("public before chmod"), and the umask never restored ("mask leaked").CI at
ea0e0d07: Runtime state models, and the stand-in fix (70c78f77)ea0e0d07,testpassed. Runtime state models failed in the frontend acceptance controls (added by test: accept prepared-base startup through ordinary native hack up #99, merged meanwhile, which PR CI runs through the merge withnext):test_stale_source_after_the_edit_is_refusedgot "up-second exited 0 before readiness".upwhile the driver pollsps) each did an unlocked read-modify-write ofstate.json. Apsthat loaded the state before the secondupsaved its foreground token then saved its stale copy over it; lost call records failed the positive acceptance test the same way. It reproduced in 26 of 40 local runs.test: serialize the frontend acceptance stand-in's shared state): each stand-in invocation holds an exclusiveflockacross its read-modify-write and releases it only before its long waits (the foreground loop and theps-hangfault). 0 of 40 local runs fail. No assertion changed.next(a7d09599, including test: accept prepared-base startup through ordinary native hack up #99) so that the fixed file is part of this PR.Second review: staging-path ownership (
5d99981d)At
70c78f77all eight checks passed. A second review found three windows insrc/lib/unix-socket-publish.tswhere an entry this attempt could not prove was its own could be changed or removed:lstat, so a replacement in that gap could bechmoded before the postcheck refused it.lstat.Fix:
chmoded. Its identity is recorded in the same tick.linkwith the same inode, bytes and mode. The scope and residuals are listed below; this is not an unconditional guarantee.afterBind/afterLinkhooks (production passes none) let tests inject the exact windows.chmodOwnerSocketdependency becomes anafterOwnerSocketPublishseam, keeping its preparation-failure test.0600and no socket name to bechmoded.Window controls, all passing on Bun 1.3.9 and 1.4.2:
chmoded, adopted or removed, including across the helper's own server close);Mutation check: five mutations fail these controls on both versions:
chmodby path after bind;Verification: 18 socket-related test files 143/143 on 1.3.9 and on 1.4.2; full TypeScript suite on 1.3.9: 1,773 tests, 1,706 pass, 0 fail; frontend acceptance controls pass; privacy ok; no new host crash reports.
Residuals: see the scope section below.
Third review: holding-name move and explicit scope (
83571a61)Fix: the failure close moved an unproven staging entry to one random holding name. If that name was occupied, the close went ahead unprotected, and a holding entry created between the check and the
renamewas overwritten. The entry is now moved withlink(2), which never replaces a holding entry, over a bounded list of 8 fresh holding names, and the staging name is dropped only while it is still the linked entry.New control (Bun 1.3.9 and 1.4.2): the first holding name is pre-occupied. It keeps its bytes, mode and inode; the staging entry goes to the next free name, survives the helper's close, and comes back unchanged.
Mutations:
renameto the first holding name fails the control on both versions;Scope, stated plainly (also in the helper's documentation). The staging-name guarantees are best-effort within a directory private to this user. They cover accidental and concurrent entries, not an adversarial process of the same user. Residuals:
The public endpoint guarantees are unchanged: the endpoint is published without replacing an existing file, removed only by identity, and never
chmoded by path.Verification: 18 socket-related test files 144/144 on 1.3.9 and on 1.4.2; full TypeScript suite on 1.3.9: 1,774 tests, 1,707 pass, 0 fail; privacy ok; no new host crash reports.