test(x25519): pin ConstantTimeEqual's acquisition order from outside the module - #66
Merged
Merged
Conversation
…the module
The core half of this fix (secret.go) landed with a regression test that
observes the acquisition order directly. The crypto half landed bare: the
lock is unexported, this module builds against a released core tag, and no
exported call holds an exclusive lock long enough to watch the order the way
core's test does.
That is an argument against two strategies, not against testing the property.
Rig the two buffers through exported API instead:
- the higher-addressed buffer gets a held reader plus a queued writer, so
writer preference parks every new read acquire on it;
- the lower-addressed buffer is sealed, so a read acquire returns ErrSealed
without running the nested callback.
Then call ConstantTimeEqual with the higher-addressed key as the RECEIVER, so
argument order and address order disagree. The fixed order takes the sealed
buffer first, fails fast, and never touches the other one - the call returns.
Argument order takes the receiver first and parks forever. "Returns at all"
is exactly the fixed ordering.
Verified to FAIL against the unfixed code (the swap disabled) with the
intended message, to pass against the fix, and to pass under GOWORK=off
against the released core tag rather than the workspace.
No stress test: reproducing the deadlock itself needs both goroutines paused
between their two acquires and there is no hook to do that. The stress
version written for core passed just as happily against the broken code,
which is why it was deleted rather than kept.
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.
The core half of the ABBA fix (#65,
secret.go) landed with a regression testthat observes the acquisition order directly. The crypto half landed bare.
The two obstacles are real but narrow:
bufferRWLockis unexported, thismodule builds against a released core tag, and no exported call holds an
exclusive lock long enough to watch the order the way core's test does. That
rules out lock observation and deadlock reproduction — not the property itself.
The invariant that regressed is "the first lock acquired is a function of the
pair's identity, not of argument position." This rigs it through exported API:
writer preference parks every new read acquire on it;
ErrSealedwithout running the nested callback.Then
ConstantTimeEqualis called with the higher-addressed key as thereceiver, so argument order and address order disagree:
So "the call returns at all" is exactly the fixed ordering.
Verification
disabled via
if false && …, keepingunsafein use), and unwedges itselfrather than hanging the run.
GOWORK=off, i.e. against the released core tag rather than theworkspace.
go vetclean; fullgo test -race ./...green for the module.Why no stress test
Reproducing the deadlock needs both goroutines paused between their two
acquires, and there is no hook to do that. The stress version written for core
passed just as happily against the broken code, which is why it was deleted
rather than kept for coverage optics.
Note for the next core release
Crypto orders by buffer address only because the janitor counter is
unexported. That is sound today — Go's collector is non-moving, and two live
buffers can never share an address, so both goroutines derive the same total
order — but it is an assumption worth retiring. The next core release is
already a MINOR bump (
InstallTerminationWipeNoExit) and already forces acrypto floor raise; exporting a stable buffer identity in that same bump lets
the floor-raise PR switch to the counter and delete the caveat at no extra
release.