Skip to content

fix(registry): emergency wipe could zero a different live buffer without holding its lock - #56

Closed
deadpoets wants to merge 1 commit into
mainfrom
fix/janitor-key-aliasing
Closed

fix(registry): emergency wipe could zero a different live buffer without holding its lock#56
deadpoets wants to merge 1 commit into
mainfrom
fix/janitor-key-aliasing

Conversation

@deadpoets

Copy link
Copy Markdown
Owner

Fixes the HIGH finding from the external review. Confirmed against the source,
with a deterministic regression test.

The bug

The janitor keyed each registration by its mapping's base address:

// regionKey returns a stable per-mapping key: the base address of the outer
// reservation, which is unique for the mapping's whole lifetime.

That comment is true and insufficient. A base address is unique for a mapping's
lifetime; it is not unique across lifetimes. Free a region and the OS may
hand the same base to the next allocation, which registers under the identical
key.

wipeInPlace resolves a key, drops the janitor lock to wait on that region's
lock, then resolves the key again:

peeked, ok := j.peekAny(key)   // buffer A
peeked.mu.lock()               // blocks — by design, for a long borrow
region, ok := j.takeAny(key)   // whatever lives at `key` NOW
wipeAndFree(region, true, false) // lockHeld=true → takes no lock

If A is destroyed during the wait and a new buffer B lands on the same base,
step 3 returns B and step 4 zeroes B with no lock held on it. That races B's
accessors and its Seal, which flips the pages to PAGE_NOACCESS mid-write —
so the worst case is a hardware access violation, not merely a lost secret.

Reachable from exactly the combination the package documents as concurrency
safe: WipeAllSecrets in flight while one buffer is destroyed and another
allocated. tryWipeInPlace has the same shape with a far narrower window
(tryLock does not wait) and is fixed identically.

The fix — two overlapping changes

  1. Keys come from a counter. A key cannot be reused, so the aliasing is
    unreachable in production.
  2. The re-resolution matches on the lock the caller holds (takeAnyIf).
    This makes the wipe safe locally rather than by depending on the key
    scheme, and it is the property a test can actually assert. The lock pointer
    is sound identity — the caller holds a live reference across the comparison,
    so the GC cannot recycle the address underneath.

takeAny had no remaining callers and is removed rather than left to rot.

The regression test

It forces the aliased state directly under the janitor lock rather than trying
to race the allocator into reusing an address, because the property under test
is that the wipe stays safe when handed a key that resolves to a region it never
locked — independent of how keys are minted.

3/3 fail against the unfixed registry.go, with the bystander's secret
replaced by zeros:

bystander was marked wiped by a wipe that resolved a key it never locked
bystander was ZEROED by a wipe holding a different buffer's lock —
    got 0000000000000000…, want 5a5a5a5a5a5a5a5a…

5/5 pass with the fix. Full core suite green under -race.

Relevance to the open crash

The reporter flags this as a candidate mechanism for the unexplained one-off
windows/amd64 runtime corruption (fatal error: stack not a power of 2). The
shape fits: a stray write into address space the allocator has already re-handed
lands in whatever now owns that range — which on a Go process can be the
runtime's own heap or goroutine metadata. I am not claiming this closes that
investigation; the soak stays in place and the release train stays held.

Disclosure

I audited this path during the corruption hunt and cleared it. I considered only
the case where the re-resolution finds nothing, and never the case where it
finds a different region. The review caught what I missed.

… address

The janitor keyed each registration by its mapping's base address. That is
unique for a mapping's lifetime, which the old comment said, but not across
lifetimes, which it did not: free a region and the OS may hand the same base to
the next allocation, which then registers under the identical key.

wipeInPlace resolves a key, drops the janitor lock to wait on that region's
lock, then resolves the key AGAIN. Those two resolutions were assumed to name
the same buffer. They need not. A wipe blocked on a buffer destroyed during the
wait could wake, resolve the key to whatever buffer now occupies that address,
and wipe it with lockHeld=true — that is, with no lock on it at all. The stray
wipe races the new buffer's accessors and its Seal, which flips the pages to
PAGE_NOACCESS mid-write, so the worst case is an access violation rather than
merely a lost secret.

Reachable from the exact combination the package documents as concurrency-safe:
WipeAllSecrets in flight while one buffer is destroyed and another allocated.
tryWipeInPlace has the same shape with a much narrower window, since tryLock
does not wait, and is fixed identically.

Two changes, deliberately overlapping:

  - Keys now come from a counter, so a key cannot be reused and the aliasing is
    unreachable in production.
  - The re-resolution matches on the lock pointer the caller is actually
    holding, via takeAnyIf. That makes the wipe safe LOCALLY rather than by
    depending on the key scheme, and it is the property a test can assert. The
    lock pointer is sound identity: the caller holds a live reference across the
    comparison, so the GC cannot recycle the address underneath.

takeAny had no remaining callers and is removed rather than left to rot.

The regression test forces the aliased state directly under the janitor lock
instead of trying to race the allocator into reusing an address, because the
property under test is that the wipe is safe when handed a key resolving to a
region it never locked — independent of how keys are minted. Verified: 3/3 fail
against the unfixed registry.go with the bystander's secret replaced by zeros,
5/5 pass with the fix.

Found by an external multi-agent review. I had audited this path during the
windows/amd64 corruption hunt and cleared it, having only considered the case
where the re-resolution finds NOTHING — not the case where it finds a DIFFERENT
region. The reporter flags it as a candidate mechanism for that open crash.
@deadpoets

Copy link
Copy Markdown
Owner Author

Landed on main as bff2938 via the integration train in #61.

Rebased locally rather than squash-merged so the commit carries your SSH signature (verified=true) instead of GitHub's web-flow PGP key — hence the different SHA and the manual close.

Still the highest-severity item from the review, and still only a candidate mechanism for the windows/amd64 runtime corruption, which has since had a second sighting (acquireSudog: found s.elem != nil in cache) on a tree that does not contain this fix.

@deadpoets deadpoets closed this Aug 19, 2026
@deadpoets
deadpoets deleted the fix/janitor-key-aliasing branch August 19, 2026 00:57
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