Skip to content

train: x/crypto 0.55.0 (both modules), emergency-wipe fix, Windows race soak - #55

Merged
deadpoets merged 7 commits into
mainfrom
train/xcrypto-fix-soak
Aug 18, 2026
Merged

train: x/crypto 0.55.0 (both modules), emergency-wipe fix, Windows race soak#55
deadpoets merged 7 commits into
mainfrom
train/xcrypto-fix-soak

Conversation

@deadpoets

Copy link
Copy Markdown
Owner

Integration train for #51, #52, #53 and #54, rebased into one linear stack so
CI verifies the exact commits that will land on main.

Order From Commit
1 #52 build(deps): bump golang.org/x/crypto ... in /examples
2 #51 build(deps): bump golang.org/x/crypto in /secmem-crypto
3 #53 fix(registry): wait on each deferred region independently
4 #53 test(sealcipher): plant the overflow with corruptCanary
5 #53 test(registry): make the hostage regression deterministic
6 #54 ci: add a dispatchable Windows race soak

Why the order matters

#51 cannot land before #52. On its own it fails the examples job with
go: updates to go.mod needed. examples pins secmem-crypto with a replace,
but a replace does not exempt the require line from minimum version selection:
once secmem-crypto requires x/crypto v0.55.0, MVS selects v0.55.0 for
examples too, while examples/go.mod still asks for v0.54.0. CI runs
readonly, so that is a hard error before any package loads.

Stacking /examples first makes every intermediate commit a valid tree, which
keeps git bisect meaningful across the train.

Verified locally before pushing (Go 1.26.6)

Signing

All six commits are SSH-signed with the maintainer's key (%G? = G), and
dependabot remains the author of its two commits. A GitHub squash-merge would
have re-signed them with GitHub's web-flow PGP key instead, which is why this
lands as a fast-forward rather than through the merge button.

main will be fast-forwarded to this branch's tip once CI is green, so the
commits that land are byte-identical to the ones tested here.

dependabot Bot and others added 7 commits August 18, 2026 19:02
Bumps [golang.org/x/crypto](https://github.com/golang/crypto) from 0.54.0 to 0.55.0.
- [Commits](golang/crypto@v0.54.0...v0.55.0)

---
updated-dependencies:
- dependency-name: golang.org/x/crypto
  dependency-version: 0.55.0
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <support@github.com>
Bumps [golang.org/x/crypto](https://github.com/golang/crypto) from 0.54.0 to 0.55.0.
- [Commits](golang/crypto@v0.54.0...v0.55.0)

---
updated-dependencies:
- dependency-name: golang.org/x/crypto
  dependency-version: 0.55.0
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <support@github.com>
…gency wipe

WipeAllSecrets runs two passes: the first wipes every region whose lock is
free at that instant, the second blocks on what is left. The second pass was a
sequential loop over the deferred slice, and that slice is built from Go map
iteration, so its order is random.

tryWipeInPlace reports "not done" for a lock held at the moment it looks,
including a momentary one. A buffer that was merely mid-WithBytes during the
first pass therefore landed in the deferred set next to a genuinely stuck one,
and when it sorted behind that one it kept its plaintext for as long as the
unrelated borrow ran. That is the exact hostage situation the two-pass split
exists to prevent, reintroduced by the second pass itself — and the guarantee
the surrounding comment stated was consequently false.

Each deferred region now gets its own goroutine, so a borrow that never
returns blocks only its own buffer and the ordering cannot matter. The cost is
one goroutine per region still locked when the first pass ends; that set is
normally empty and is bounded by the number of live registrations. A worker
pool would reintroduce the bug as soon as the stuck borrows outnumbered the
workers, so the count is deliberately unbounded.

The regression test forces the interleaving rather than racing for it: every
idle buffer is held borrowed from before the call until well after the first
pass has run, which guarantees the demotion, and the stuck borrows are never
released while the assertion runs, so an idle buffer can only be zeroed if the
wipe waited on it independently. Map iteration order cannot be forced, so six
buffers on each side reduce the unfixed code's chance of passing to one
ordering in C(12,6) = 924; that trade is documented in the test.

Verified: 3/3 fail against the unfixed registry.go, 5/5 pass with the fix.

Found by stressing the race-built suite at GOMAXPROCS=4 to match CI's runners,
where the pre-existing sibling test failed 1 process in 40.
…l 0x00

TestSealCipher_RealOverflowStillDetected wrote a fixed 0x00 over the first
canary byte to simulate an overflow. The canary pattern is 16 bytes of
crypto/rand, generated once per process, so on roughly 1 run in 256 that byte
is already 0x00 — the write plants nothing, Destroy correctly reports no
violation, and the test fails claiming the seal/unseal round trip masked a
real overflow.

Observed at 1 process in 250 while stressing the race-built suite at
GOMAXPROCS=4, which matches 1/256 closely enough to be the whole story.

corruptCanary exists for exactly this and is already used by the other
overflow proofs; its own comment names the 1/256 no-op as the hazard to avoid.
This call site simply predated or missed it. No product behaviour is involved:
the canary check was right every time, including on the failing runs.
The first version of this test defeated map-iteration order statistically:
six buffers on each side, so the unfixed code passed only on 1 ordering in
C(12,6). That works, but it is a probabilistic guard for a security property,
and it needed a 250 ms sleep to place the first pass.

The repo already had the primitive to do this properly. waitForWritersWaiting
observes writersWaiting on a specific bufferRWLock, which distinguishes the two
implementations exactly: the fixed wipe waits on every deferred region
independently, so each has a writer queued on its own lock, while a sequential
second pass can only ever have one queued at a time. Asserting a queued writer
on BOTH locks therefore fails against the unfixed code no matter which key map
iteration put first.

Now 2 buffers instead of 12, no sleep, and the failure is deterministic:
3/3 fail against the pre-fix registry.go with "timed out waiting for 1 queued
writers (got 0)" — landing on stuck.mu or idle.mu depending on map order, but
always on one of them — and 10/10 pass with the fix, in 2.1s total.
…crash

CI saw `fatal error: stack not a power of 2` once on windows-latest under
-race: the runtime found a cached goroutine on its free list whose stack size
is not a power of two, reached through gfget from testing.(*T).Run. A re-run of
the same job passed, roughly 500 local processes across Go 1.26.5 and 1.26.6
never reproduced it, and no upstream Go issue matches.

For a library that writes to memory the runtime does not manage, "seen once,
went away" is not a conclusion. One CI run per PR samples this once; this
workflow samples it a few hundred times on the runner image where it actually
appeared, with GOTRACEBACK=system so the runtime frames survive into the log —
the default traceback elides exactly the frames that are the evidence.

Diagnostic tooling, not a gate: nothing depends on it and no branch protection
references it, so a red run is a finding to look at rather than a broken build.
Inputs cover iteration count, GOMAXPROCS, GODEBUG (clobberfree=1 and friends)
and the module directory, so a hypothesis can be tested without editing the
file. Daily at 09:00 UTC, clear of the 07:00 fuzz run.

A green soak is reported honestly as bounding the rate rather than clearing the
defect, because a green tick on a workflow named "soak" is exactly the kind of
thing that later gets cited as proof it was fixed.

The header says to delete this once the crash is understood. A soak nobody
reads is worse than none, because it looks like coverage.
… move together

Dependency changes are release-relevant in this repo — secmem-crypto/v0.3.1 was
a dependency-only release with its own entry — because a require change in
secmem-crypto raises the floor for everyone importing it. Two x/crypto bumps
were landing with nothing in Unreleased.

Also records the coupling, since it is the kind of thing that is obvious only
while you are looking at it: examples pins secmem-crypto with a replace, but a
replace does not exempt the require line from minimum version selection, so
bumping only secmem-crypto makes MVS select 0.55.0 for examples while its go.mod
still asks for 0.54.0. CI runs readonly, so that is a hard error before any
package loads — which is exactly how #51 failed on its own.

Maintenance rather than a security fix: the vuln job was green against 0.54.0.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant