train: external-review fixes — janitor key aliasing, memfd CLOEXEC, release gate, lint false negatives, redact gaps - #61
Merged
Conversation
… 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.
…ase gate that fails closed Three findings from the external review, unrelated to each other beyond all being cases where something reported success it had not earned. memfd_secret fd was inheritable across exec -------------------------------------------- Created with no flags, and the fd stays open across ftruncate, the guard reservation and the MAP_FIXED. A fork+exec from any other goroutine in that window hands the child a live descriptor to the secret pages — making the strongest allocation tier the one with the worst exec posture. The flag is O_CLOEXEC, NOT the FD_CLOEXEC that memfd_secret(2) names: the kernel tests `flags & O_CLOEXEC` and EINVALs anything outside SECRETMEM_FLAGS_MASK|O_CLOEXEC. Passing bit 0 would not have merely failed to set close-on-exec, it would have failed the syscall and silently dropped every allocation to the weaker anon path — a worse outcome than the leak. Since this file cannot be executed from the maintainer's platform, EINVAL is not trusted to mean one specific thing: it retries bare and sets close-on-exec via fcntl(F_SETFD). Slightly wider window than the atomic form, never a downgrade. InstallTerminationWipe claimed a termination it cannot perform on Windows ------------------------------------------------------------------------- os.Process.Signal on Windows supports only os.Kill and rejects os.Interrupt and SIGTERM with "not supported by windows" — verified empirically on go1.26, windows/amd64, rather than from memory. The error was discarded, so the process ran on past Ctrl-C with every secret already zeroed while the doc comment said it terminated. Now logged at warn level with the platform limit documented. Deliberately not escalated to a forced os.Exit: the installer promises never to take the exit away from a co-installed graceful shutdown, and signal.Notify is additive, so such a handler already received the signal independently. release.sh ordering check failed open on the case it exists for --------------------------------------------------------------- It asked only whether the required in-repo version was published. The failure that motivated the whole script — secmem-crypto/v0.3.0, permanently inert — required a version that was published perfectly well. It was just the previous one, because the tag was cut before the floor-raise PR merged. The gate reported success on precisely the footgun it was written to prevent. It now also requires that version to be the newest published one, and fails CLOSED when the proxy cannot be reached, because an unreachable proxy is exactly when someone is most tempted to shrug and tag anyway. SECMEM_ALLOW_STALE_DEP=1 covers a deliberately older floor. version_lt is unit-tested against the numeric-vs-lexical trap (v0.10.0 > v0.9.0) and verified end to end against the live proxy: it refuses v0.3.0-against-v0.4.0 and passes the repo's current, correct state.
TestScrub_ScrubsShallowCallTree addresses its planted markers through a raw uintptr into this goroutine's stack. The GC neither tracks nor adjusts that value, so a stack shrink between planting and reading frees the segment back to the pool and the read lands on whatever now occupies the address. The fault case is the obvious one. The quiet case matters more: unrelated memory holds no 0xA5 markers, so countMarkers returns 0 and the assertion PASSES having observed nothing at all. A security regression test that can silently succeed without testing anything is worse than one that fails, and this one guards the reserve-then-wipe fix specifically. shrinkstack only runs while the collector is scanning the goroutine, so the collector is disabled for the window — placed before the control read so it covers that too. Also corrects countMarkers' comment, which asserted the read was safe because "stack segments are pooled, not unmapped". That holds only while the segment is still this goroutine's; after a shrink it can be scavenged. The caller owns that constraint now, and the comment says so. Flagged by the external review as a candidate for the open windows/amd64 corruption. I do not think it is: this is a READ, and it cannot corrupt the goroutine free list, while the observed crash was `stack not a power of 2` thrown from stackfree via gfget. A faulting read reports an unexpected fault address instead. Fixed on its own merits, not as a crash theory.
…d reentrancy checks
A linter that misses a sink does not merely fail to help — it certifies the
code, which is worse than not running it. All eight of these reported clean.
escape checks
-------------
Assignment escapes matched only *ast.Ident, so s.field = b, m[k] = b,
out[i] = b and *p = b were every one of them silently clean. Stashing a
borrowed slice into a struct field is the most natural way to leak one. Targets
are now classified by shape; a local struct or array VALUE is still correctly
inside the lease, while a local pointer, slice or map is not, since the variable
is inner but what it refers to need not be.
Logging methods could never match. The sink table is keyed by
"import/path.Func", which only ever matches a call qualified by a package name,
so sl.Info(b), l.Printf("%s", b) and slog.Default().Warn(..., b) all reached the
end of sinkFor unflagged. Now resolved by receiver type, so an unrelated Info
method on another type is still not swept in.
append(dst, b[:n]...) escaped because the last argument was matched as a bare
identifier, in a file whose other checks already used refersToParam — which
handles exactly this shape.
panic(b) was not flagged at all, though the value is formatted into the runtime
traceback and handed to any recover() up the stack.
Sink table: added log.Panicln (both its siblings were already listed),
log/slog.Log, log/slog.LogAttrs, fmt.Append, fmt.Appendf, fmt.Appendln.
reentrancy check
----------------
The unsafe set omitted SetByteAt, which takes the EXCLUSIVE lock and is
therefore the most certain member of the set — an unconditional self-deadlock —
while its read counterpart ByteAt was listed. It also omitted the rLock
inspectors Len, MappedLen, IsSealed and IsDestroyed: a nested read acquire looks
harmless, but this package's lock is writer-preferring, so rLock waits while any
writer is queued and a second read from inside a borrow deadlocks as soon as a
Destroy or an emergency wipe arrives between the two.
verification
------------
Fifteen fixture cases added across the escape and reentrancy testdata, and every
one was confirmed to FAIL against the previous analyzer before the fix — a lint
fixture that would pass either way proves nothing. The testdata stub gained the
inspector methods so the new cases typecheck.
Not addressed here, and still open from the review: the reentrancy check no-ops
when the receiver is not a plain identifier (accessor.recv is documented as nil
in that case), and the goroutine-capture check matches by bare name so a
shadowed variable can be flagged. Both need the accessor plumbing changed rather
than a table entry, so they are their own change.
The first thing the repaired analyzer did was fail the dogfood job on this repo's own shipped example. ExampleScope called buf.Len() from inside buf.WithBytesErr, taking the read lock a second time from within the borrow. That is not merely untidy. The lock is writer-preferring, so rLock waits while any writer is queued: the nested acquire deadlocks as soon as a Destroy or an emergency wipe arrives between the two. An example is the one place the contract most needs to be demonstrated rather than contradicted, and this one had been teaching the deadlock. len(b) is the same number, carried by the borrowed slice, and takes no lock. Independently reported by the external review as example_test.go:42, which is a useful cross-check: the analyzer change and the human reviewer found the same defect from opposite directions.
…andler grouping
Four findings from the external review. The first three are cases where the
redactor reported a clean message it had not actually cleaned.
Credential rules missed the shape structured logs emit
------------------------------------------------------
Tier-1 patterns were field[=:]\s*\S+, which requires the separator to follow the
key immediately. JSON and structured logs put a quote there, so
{"password": "hunter2"} matched NOTHING and went to the sink in full — verified
in the test output, where the sanitized message is byte-identical to the input.
\S+ also stops at the first space, so password="hunter 2 correct horse" masked
only up to the space and left the rest of the secret in the line.
All five fields now come from one helper, so the quoting rules cannot diverge
between them again: an optional quoted key, and a value alternation that tries
the quoted forms first and falls back to \S+.
The C1 backstop passed what it claimed to strip
------------------------------------------------
stripNonPrintable tested `r >= 32 && r != 127` while documenting C0/C1 coverage,
so every C1 code point passed. That matters beyond tidiness: U+009B is the
single-character CSI, and a terminal decoding the stream as Latin-1/ISO-2022
treats it exactly as the two-byte ESC-[ that the ansi rule strips. The backstop
was bypassable by spelling the escape differently. Invalid UTF-8 is now reported
as redacted instead of quietly becoming U+FFFD.
The allowlist switched itself off when its label appeared twice
---------------------------------------------------------------
isAllowlisted used FindStringIndex — the EARLIEST match — and compared its end
against the credential's start. Any message mentioning the label earlier failed
that comparison and redacted the value the allowlist existed to exempt. All
occurrences are considered now.
Handler misfiled WithAttrs attributes into later groups
--------------------------------------------------------
The wrapper held them and re-added them to every record, so the inner handler
emitted them at whatever nesting it had reached by then:
log.With("req", id).WithGroup("db").Info("query", "table", t)
-> {"db":{"req":...,"table":...}} want {"req":...,"db":{"table":...}}
slog's contract is positional — attributes added before WithGroup belong outside
it. Fixed by delegating to inner.WithAttrs at the point they are added, which
pins each attribute at its own nesting level and is the inner handler's job
rather than something this wrapper should re-decide. cloneAttrs became dead and
is removed.
Every test added here was confirmed to fail against the unfixed code first; two
of them initially passed for the wrong reason and were rewritten (the allowlist
one used a rule category that is never allowlist-gated, and the invalid-UTF-8
one had been written as U+00FF rather than a raw 0xFF byte, so it tested
nothing).
…aracters staticcheck ST1018 flagged the literal U+009B and U+0085 in the new C1 backstop test. It is right to: a source file that embeds raw control characters is exactly the hazard the code under test exists to neutralise, and the bytes are invisible in review — the reviewer sees 'before31mafter' and cannot tell what is in the middle. The escape sequence compiles to the same string and says what it is.
… reproduced The soak looped a prebuilt single-package binary. That ran 1250 iterations and found nothing, while two ordinary CI runs reproduced the crash — so the soak was measuring the wrong thing and its clean results were not the evidence they looked like. ci.yml runs `go test -race ./...`, which compiles and runs the core and redact test binaries CONCURRENTLY. For a bug whose signature is a stray write landing in runtime free-list caches, "what the allocator hands back next" is the entire question, and concurrent processes change that answer. The prebuilt-binary loop is faster per iteration but is not the same experiment. `mode` selects between them and defaults to gotest, because a default that is fast and has never reproduced is worse than a slow one that has. Header records the second sighting: acquireSudog found a released sudog with elem != nil, where the first was the goroutine free list with a non-power-of-two stack size. Both are per-P free-list caches in an impossible state, and in both the crashing frame is innocuous — the goroutine trips over earlier damage. Also notes the discriminator that fell out of sighting 2: making a field NON-nil cannot be done by a zero-wipe, so the canary fill (random pattern) and ordinary buffer writes are better suspects than secureWipe.
This was referenced Aug 19, 2026
Closed
Closed
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
1 participant
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.
Integration train for #56, #57, #58, #59 and #60, rebased into one linear stack
so CI verifies the exact commits that will land on
main.18 findings from the external adversarial review, 1 HIGH.
fix(registry): identify registrations by counter, not by mapping base addressfix: close-on-exec for memfd_secret, honest Windows termination, release gate that fails closedtest(scrub): keep the collector out of the dead-stack read windowfix(secmem-lint): close eight false-negative classesfix(example): len(b) instead of buf.Len() inside the borrowfix(redact): quoted-credential shapes, C1 backstop, allowlist scan, handler groupingtest(redact): write C1 test inputs as escapesci(soak): run go test -race ./... by defaultThe one that matters most
#56 — the emergency wipe could zero a different, live buffer without holding
its lock. The janitor keyed registrations by mapping base address, which is
unique for a mapping's lifetime but not across lifetimes.
wipeInPlaceresolvesa key, drops the janitor lock to wait, then resolves the key again — so a wipe
blocked on a destroyed buffer could wake and zero whatever now occupies that
address, with
lockHeld=true, i.e. with no lock on it at all.Reachable from the exact API combination the package documents as
concurrency-safe. Regression test fails 3/3 unfixed with the bystander's secret
replaced by zeros.
A theme worth naming
Most of these are the same failure mode rather than eighteen unrelated bugs:
something reported success it had not earned.
release.shpassed the stale-go.modcase it was written to prevent.secmem-lintcertified code it never examined (8 classes).redactreturned{"password": "hunter2"}unchanged and called it sanitized.stripNonPrintabledocumented C1 coverage and testedr >= 32.shrink.
Every fix here is paired with a test verified to fail against the unfixed
code. Two of my own tests initially passed for the wrong reason and were
rewritten rather than kept.
Verified locally on the merged tree (Go 1.26.6)
go test -race ./...,secmem-lint,secmem-crypto(GOWORK=off),examples(GOWORK=offvet + test).check that caught the shipped-example deadlock now fixed in commit 5.
gofmtclean; no conflict markers; all five CHANGELOG entries survived theunion merges intact.
CHANGELOG conflicts
Every PR added to the same
Unreleasedsubsections, so each pick conflicted.Resolved by union, ours-then-theirs — the correct resolution when both sides
are independent new entries — via a script that refuses if a conflict is any
other shape.
Signing
All eight commits are SSH-signed with the maintainer's key (
%G?=G).mainwill be fast-forwarded to this tip once CI is green, so what lands isbyte-identical to what was tested.
Still open, deliberately
The
windows/amd64runtime corruption now has two sightings (stack not a power of 2;acquireSudog: found s.elem != nil in cache) — both per-P runtimefree-list caches in impossible states. #56 is a candidate mechanism, not a
confirmed fix, and #60 exists because the soak had been running the wrong
experiment. The release train stays held.