-
Notifications
You must be signed in to change notification settings - Fork 12
fix(connect): close TOCTOU race in connect sync lock acquisition#113
Open
omergk28 wants to merge 2 commits into
Open
fix(connect): close TOCTOU race in connect sync lock acquisition #113omergk28 wants to merge 2 commits into
omergk28 wants to merge 2 commits into
Conversation
The sync lock was a stat-then-write: two processes racing past the existence check could both acquire, both load the same LastSequence, and both write duplicate entries into .context/hub/. Replace the pair with the atomic io.SafeTryLock (O_CREATE|O_EXCL, single syscall) and release via io.SafeUnlock — the same primitive the dream pass already uses, preserving the os.ErrExist contract for callers. - LockSentinel removed: the lock is the file's existence, not its content, and the racy write was the constant's only consumer. - state_test.go regression-pins the contract: 16-way contention yields exactly one winner, release frees the next sync, a corrupt state file does not leak the lock, and the lock path stays at <ctxDir>/hub/.sync.lock. Closes ActiveMemory#93. Spec: specs/fix-connect-sync-lock-toctou.md Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Omer Kocaoglu <omergk28@gmail.com>
Stock macOS ships bash 3.2 (GPLv2 freeze), which treats
"${arr[@]}" on an empty array as an unbound variable under
set -u — bash 4.4+ does not. drift_grep expands its exclude
array unconditionally and several call sites pass no excludes,
so make lint-style (and therefore make audit, the mandatory
pre-PR gate) aborted on every stock Mac before running a single
drift check.
Guard with the parameter-expansion alternate form,
${arr[@]+"${arr[@]}"}: empty expands to zero words, populated
reproduces the original quoted expansion, bash 4+ behavior
unchanged. The other hack/ array expansions are safe today
(count-guarded or hardcoded non-empty); details in the spec.
Captures the gotcha in LEARNINGS.md so the next session does
not re-debug it.
Spec: specs/fix-lint-drift-bash32-empty-array.md
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Signed-off-by: Omer Kocaoglu <omergk28@gmail.com>
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.
Uh oh!
There was an error while loading. Please reload this page.
Summary
loadStateininternal/cli/connection/core/syncguardedctx connect syncwith a stat-then-write pair — racy by construction. Two processes racing past theos.Statcould both acquire, both load the sameLastSequence, and both write duplicate entries into.context/hub/.This PR replaces the pair with the atomic create-or-fail primitive the codebase already ships for exactly this purpose:
io.SafeTryLock(O_CREATE|O_EXCL, one syscall) +io.SafeUnlock, with prior art atinternal/cli/dream/core/pass/pass.go:62-70.!acquiredmaps to the pre-existingos.ErrExistcontract, so caller-facing behavior is unchanged.Closes #93.
Changes
internal/cli/connection/core/sync/state.go— atomic lock acquisition; release delegates toSafeUnlock(warn-on-failure logging kept)internal/config/hub/{hub.go,doc.go}—LockSentinelremoved: the lock is the file's existence, not its content, and the racy write was the constant's only consumerinternal/cli/connection/core/sync/state_test.go— new; the package previously had zero testsspecs/fix-connect-sync-lock-toctou.md— spec per the project's spec-per-commit invariantTests (mapped to the issue's Tests Required)
os.ErrExistTestLoadState_RejectsConcurrentSyncs(16-way; winners held until all results are collected, so the count is deterministic)TestLoadState_ReleaseRemovesLock+TestLoadState_ReleasesLockOnCorruptState(post-acquisition failure must not leak the lock)<ctxDir>/hub/.sync.lockTestLoadState_LockFileLocationVerification
winners = 16, want exactly 1, proving both that the race is real and that the test catches it. Against the fix: exactly 1 winner, 15 ×ばつos.ErrExist.go test -race -count=10 ./internal/cli/connection/core/sync/— clean.make audit— all checks passed (fmt, vet, golangci-lint, style, drift, docs, full test suite).Notes
.context/shared/— that's the pre-rename path; the renderer joinscfgHub.DirHub(render.go:32), so the spec and this PR say.context/hub/.flockalternative (Unix-only;SafeTryLockmatches the existing sentinel model).🤖 Generated with Claude Code
Second Commit:
make auditon Stock macOSWhile gating this PR,
make auditaborted on a stock Mac before running a single drift check:bash 3.2 (the newest Apple ships, GPLv2 freeze) treats
"${arr[@]}"on an empty array as unbound underset -u; bash 4.4+ does not. Sincemake auditis the contributing guide's mandatory pre-PR gate, every Mac contributor hits this. Commit 2 guards the expansion with the canonical${arr[@]+"${arr[@]}"}alternate form (no behavior change on bash 4+), with its own spec (specs/fix-lint-drift-bash32-empty-array.md) and a LEARNINGS.md entry. The otherhack/array expansions were checked empirically — all safe today (count-guarded or hardcoded non-empty; details in the spec).Happy to split this into its own PR if you'd rather keep #93 surgical — it's an independent commit, so a cherry-pick is clean either way.