fix: acquire both read locks in a fixed order in ConstantTimeEqual - #65
Merged
Conversation
Secret.ConstantTimeEqual and X25519Key.ConstantTimeEqual both took their two read locks in argument order, so a.ConstantTimeEqual(b) and b.ConstantTimeEqual(a) running concurrently acquired them in opposite directions. Read locks are shared, so this needs a writer queued on each buffer to become a cycle — which a writer-preferring lock makes routine, since a Destroy or an emergency wipe is enough to queue one. Once wedged, the two buffers are also unreachable for Destroy and WipeAllSecrets, so one reversed comparison takes the emergency wipe down with it. Ordering keys differ by module, deliberately. The core orders by janitorKey, a process-unique counter that assumes nothing about where the GC keeps objects. secmem-crypto orders by buffer address: that counter is unexported and this module builds against a RELEASED core tag, so reaching for it would have meant a core release plus a floor raise before the deadlock could be fixed at all. Address ordering is sound while the Go GC does not relocate heap objects, which it has never done, and the comment says to switch if an exported identity lands. Testing, and what could not be tested -------------------------------------- The deadlock itself is not what the test asserts. Reproducing it needs both goroutines paused BETWEEN their two acquires and there is no hook to pause them: a stress version passed just as happily against the unfixed code, which makes it worthless as a guard. It was written, shown to be vacuous, and deleted rather than kept for the look of coverage. The ORDER is directly observable, and it is the property that makes the cycle impossible. Holding an exclusive lock on the lower-keyed buffer and comparing with the higher-keyed one as receiver puts argument order and key order in conflict: ordered code blocks on the lower key and never touches the higher one, while argument-order code read-locks the receiver first and holds it. A reader appearing on the higher-keyed buffer is exactly the bug. Deterministic — fails in 0.9s against the unfixed code. secmem-crypto has no equivalent test. bufferRWLock is unexported, so a test in that module cannot observe which lock was taken, and nothing exported holds a secmem buffer's exclusive lock long enough to arrange the conflict. Stating that rather than shipping a stress test that would prove nothing.
This was referenced Aug 19, 2026
deadpoets
added a commit
that referenced
this pull request
Aug 19, 2026
…rting The comment added in #63 contains a literal empty `${{ }}`. Actions expands expressions in `run:` before any shell sees them, so the `#` protects nothing and the file has been unparseable since f6218b4 — zero-job runs against pushes, nothing on any PR's checks, so #64/#65/#66 all looked green.
deadpoets
added a commit
that referenced
this pull request
Aug 19, 2026
## The soak has not run since #63 The comment I added in #63 contains a literal empty `${{ }}`. Actions expands expressions in `run:` before any shell sees them, so the `#` protects nothing — the file has been unparseable ever since. ``` last success 2026-08-19T00:27:43Z workflow_dispatch 704806f first failure 2026-08-19T01:54:52Z push f6218b4 (#63) ``` It fails invisibly: a zero-job run against a **push**, never against the PR. #64, #65 and #66 each reported 22/22 green while the soak was dead. ## Commits 1. **`8b18b5a`** — drop the delimiter. Verified: dispatched run [32210902399](https://github.com/deadpoets/secmem/actions/runs/32210902399) completed successfully, 20 iterations. 2. **`390695d`** — `actionlint` in the `lint` job, pinned to v1.7.12. Verified to exit 1 on the pre-fix file and 0 on the fixed one. 3. **`ca25dbc`** — fingerprint the runner on every soak run. ## Fingerprint, first result | | workstation | `windows-latest` | |---|---|---| | OS | Win 11 Insider 26220 (client) | **Server 2025** Datacenter 26100 | | CPU | Intel Core Ultra 7 265KF, 20 logical | **AMD EPYC 7763**, 4 logical | | Mandatory ASLR | **ON** | off | | HVCI | **running** | not running | | Defender realtime | **on** | off | | Scheduler quantum | 2 | 2 — *same* | Two differences I expected turned out not to exist: both hosts are build 261xx, and both run the client quantum. Worth having measured rather than assumed. Separately: **`GOMAXPROCS=4` does not emulate a 4-CPU machine.** It caps Go's Ps while the OS still spreads sysmon, GC workers and the race detector's threads across all cores — so the ~1190 local samples were never the runner's contention profile. --------- Co-authored-by: Chris Fink <7587613+deadpoets@users.noreply.github.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.
Both two-buffer comparisons —
Secret.ConstantTimeEqual(secret.go:91) andX25519Key.ConstantTimeEqual(secmem-crypto/x25519.go:174) — took their readlocks in argument order, so
a.ConstantTimeEqual(b)andb.ConstantTimeEqual(a)running concurrently acquired them in oppositedirections.
Read locks are shared, so this only becomes a cycle once a writer is queued on
each buffer — which a writer-preferring lock makes routine, since a
Destroyor an emergency wipe is enough to queue one. Once wedged, both buffers are also
unreachable for
DestroyandWipeAllSecrets: one reversed comparison takesthe emergency wipe down with it.
Both now acquire in a fixed global order.
Two different ordering keys, deliberately
janitorKeysecmem-cryptoUsing the counter from crypto would have meant a core release plus a floor
raise before this deadlock could be fixed at all. Address ordering is sound
while the Go GC does not relocate heap objects — which it has never done — and
the comment says to switch if an exported identity ever lands.
What I could not test, and why I am not pretending otherwise
The deadlock itself is not what the test asserts. Reproducing it needs both
goroutines paused between their two acquires, and there is no hook to pause
them. I wrote the stress version first; it passed just as happily against the
unfixed code, which makes it worthless as a guard. It was deleted rather than
kept for the look of coverage.
The order is observable, and it is the property that makes the cycle
impossible. Holding an exclusive lock on the lower-keyed buffer and comparing
with the higher-keyed one as receiver puts argument order and key order in
conflict:
blocked.
So a reader appearing on the higher-keyed buffer is the bug. Deterministic —
fails in 0.9s against the unfixed code, passes with the fix.
secmem-cryptohas no equivalent test.bufferRWLockis unexported, so atest in that module cannot observe which lock was taken, and nothing exported
holds a secmem buffer's exclusive lock long enough to arrange the conflict. That
is a consequence of crypto depending on released core API, and I would rather
state it than ship a stress test that proves nothing.
Verification
core + redact
-race,secmem-crypto-raceunderGOWORK=off,gofmtclean.