train: external-review fixes — janitor key aliasing, memfd CLOEXEC, release gate, lint false negatives, redact gaps

.github/scripts/release.sh

@@ -101,6 +101,32 @@ esac
 # Parsed via `go mod edit -json` rather than by reading go.mod as text — the
 # require block has several legal shapes and a regex over it is exactly the
 # kind of thing that quietly matches nothing and reports success.
+# latest_version extracts .Version from a proxy @latest JSON body on stdin.
+latest_version() {
+  python3 -c 'import json,sys
+try:
+    print(json.load(sys.stdin).get("Version", ""))
+except Exception:
+    print("")' 2>/dev/null || printf ""
+}
+
+# version_lt reports whether $1 is an older semver than $2, i.e. whether this
+# module's go.mod still points at a superseded release. SECMEM_ALLOW_STALE_DEP=1
+# disables the refusal for a deliberately older floor.
+#
+# An unparseable version on either side reports "not older": pseudo-versions and
+# +incompatible are shapes this comparison does not understand, and refusing on
+# something it cannot read would be a different kind of wrong answer.
+version_lt() {
+  [ "${SECMEM_ALLOW_STALE_DEP:-0}" = "1" ] && return 1
+  python3 -c 'import re, sys
+def parse(v):
+    m = re.match(r"^v(\d+)\.(\d+)\.(\d+)", v or "")
+    return tuple(int(x) for x in m.groups()) if m else None
+a, b = parse(sys.argv[1]), parse(sys.argv[2])
+sys.exit(0 if (a and b and a < b) else 1)' "$1" "$2"
+}
+
 repo_root_module=$(GOWORK=off go list -m)
 deps=$(cd "$dir" && GOWORK=off go mod edit -json | python3 -c '
 import json,sys
@@ -123,6 +149,38 @@ else
              Tagging now would publish a release pointing at a version that
              cannot be resolved. Release $req $reqver first, then re-run this."
     ok "in-repo dependency $req $reqver is published"
+
+    # "Published" is NOT the invariant this gate exists for. The failure that
+    # actually happened — secmem-crypto/v0.3.0, permanently inert — had a
+    # go.mod requiring a version that was published just fine. It was simply
+    # the OLD one, because the tag was cut before the floor-raise PR merged.
+    # Checking only for publication passes that case and reports success, which
+    # is the one outcome this script must never produce.
+    #
+    # So the required version must also be the newest published one. If the
+    # dependency has moved ahead, this module's go.mod has not caught up and the
+    # tag would claim a dependency it does not have.
+    latestbody=$(curl -s -w '
+%{http_code}' "https://proxy.golang.org/${reqesc}/@latest" || printf '
+000')
+    latestcode=$(printf '%s' "$latestbody" | tail -1)
+    latest=$(printf '%s' "$latestbody" | sed '$d' | latest_version)
+    if [ "$latestcode" != "200" ] || [ -z "$latest" ]; then
+      # Fail closed. An unreachable proxy is exactly when a human is most
+      # tempted to shrug and tag anyway.
+      die "cannot determine the latest published $req (HTTP $latestcode).
+             Refusing to tag: this is the check that catches a stale go.mod, and
+             it does not get skipped because the proxy is unreachable. Retry
+             when proxy.golang.org answers."
+    fi
+    if version_lt "$reqver" "$latest"; then
+      die "$dir/go.mod requires $req $reqver, but $latest is published.
+             This is the ordering footgun: the tag would claim a dependency it
+             does not have, permanently. Merge the go.mod floor raise and
+             re-run. If the older floor is deliberate, re-run with
+             SECMEM_ALLOW_STALE_DEP=1 and record why in CHANGELOG.md."
+    fi
+    ok "in-repo dependency $req $reqver is the newest published"
   done <<EOF
 $deps
 EOF

.github/workflows/soak-windows.yml

@@ -25,6 +25,22 @@ name: Windows Race Soak
 # GOTRACEBACK=system is the point of the exercise — the default traceback elides
 # runtime frames, and the runtime frames are the evidence.
 #
+# SECOND SIGHTING, 2026-08-19 (CI run 32201035150, PR #59):
+#
+#     fatal error: acquireSudog: found s.elem != nil in cache
+#
+# Different message, same class, and together they make the signature specific:
+# both are per-P runtime FREE-LIST CACHES found in an impossible state — the
+# goroutine free list with a non-power-of-two stack size, and the sudog cache
+# holding a released sudog whose elem is not nil. In both the crashing frame is
+# innocuous (TestCapabilities_String; t.Parallel), so the goroutine is merely
+# tripping over damage done earlier. That is a stray WRITE into memory the Go
+# runtime owns.
+#
+# One useful discriminator: the sudog case needs a write that makes a field
+# NON-nil, which a plain zero-wipe cannot do. Canary fill (a random pattern) and
+# ordinary buffer writes can.
+#
 # Delete this workflow once the crash is understood. A soak that nobody reads is
 # worse than no soak, because it looks like coverage.
 
@@ -45,6 +61,9 @@ on:
       package:
         description: "Module directory to soak"
         default: "."
+      mode:
+        description: "gotest = `go test -race ./...` exactly as ci.yml runs it (reproduces); binary = loop one prebuilt package binary (faster, has NOT reproduced)"
+        default: "gotest"
 
 permissions:
   contents: read
@@ -74,17 +93,38 @@ jobs:
           ITERATIONS: ${{ github.event.inputs.iterations || '200' }}
           GOMAXPROCS: ${{ github.event.inputs.gomaxprocs || '4' }}
           GODEBUG: ${{ github.event.inputs.godebug || '' }}
+          MODE: ${{ github.event.inputs.mode || 'gotest' }}
           GOTRACEBACK: system
         run: |
           set -uo pipefail
-          go test -race -c -o soak.exe .
-          echo "soaking $ITERATIONS processes at GOMAXPROCS=$GOMAXPROCS, GODEBUG='${GODEBUG:-}'"
+
+          # Two shapes, and the difference turned out to matter.
+          #
+          # `binary` builds the package test binary once and loops it. It is
+          # several times faster per iteration, and it is what this workflow did
+          # originally — for 1250 iterations, finding nothing, while two
+          # ordinary CI runs reproduced the crash.
+          #
+          # `gotest` runs `go test -race ./...`, exactly as ci.yml does. That
+          # compiles and runs the core and redact test binaries CONCURRENTLY,
+          # which is a materially different address-space and allocator story —
+          # and for a bug whose signature is a stray write into runtime
+          # free-list caches, "what the allocator hands back next" is the whole
+          # question. It is the default because it is the only shape that has
+          # actually reproduced.
+          if [ "$MODE" = "binary" ]; then
+            go test -race -c -o soak.exe .
+            run_once() { ./soak.exe -test.count=1 2>&1; }
+          else
+            run_once() { go test -race -count=1 ./... 2>&1; }
+          fi
+          echo "soaking $ITERATIONS iterations, mode=$MODE, GOMAXPROCS=$GOMAXPROCS, GODEBUG='${GODEBUG:-}'"
 
           failures=0
           # Pure-bash counter: seq is coreutils and not guaranteed on the
           # runner's Git bash.
           for ((i = 1; i <= ITERATIONS; i++)); do
-            if ! out=$(./soak.exe -test.count=1 2>&1); then
+            if ! out=$(run_once); then
               failures=$((failures + 1))
               echo "::group::FAILURE on iteration $i"
               echo "$out"

CHANGELOG.md

@@ -15,6 +15,16 @@ mark the stability commitment.
 
 ### Changed
 
+- **`release.sh` now refuses a stale in-repo dependency instead of passing it.**
+  The ordering check only asked whether the required version was *published*.
+  The failure it exists to prevent — the permanently inert
+  `secmem-crypto/v0.3.0` — required a version that was published perfectly well;
+  it was simply the previous one, because the tag was cut before the floor-raise
+  PR merged. The gate therefore reported success on exactly the case it was
+  written to catch. It now also requires that version to be the newest published
+  one, fails closed when the proxy cannot be reached, and honours
+  `SECMEM_ALLOW_STALE_DEP=1` for a deliberately older floor.
+
 - **`secmem-crypto`, `examples`: `golang.org/x/crypto` 0.54.0 → 0.55.0.**
   Maintenance, not a fix: the `vuln` job was green against 0.54.0, so nothing
   outstanding was reachable. Recorded because a `require` change in
@@ -30,6 +40,114 @@ mark the stability commitment.
 
 ### Fixed
 
+- **The emergency wipe could zero a different, live buffer without holding its
+  lock.** The janitor keyed each registration by its mapping's base address.
+  That is unique for a mapping's lifetime but not across lifetimes: 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* — so
+  a wipe blocked on a buffer that was destroyed during the wait could wake,
+  resolve the key to the buffer now occupying that address, and zero it with
+  `lockHeld=true`, i.e. with no lock on it at all. That races the new buffer's
+  accessors and its `Seal`, which flips the pages to `PAGE_NOACCESS` mid-write.
+
+  Reachable from the exact API combination the package documents as
+  concurrency-safe: `WipeAllSecrets` running while one buffer is destroyed and
+  another is allocated. Registrations are now identified by a counter, which
+  cannot be reused, and the re-resolution additionally matches on the lock the
+  caller actually holds — so the wipe is safe even if the key scheme changes
+  again. Found by an external adversarial review; the reporter also flags it as
+  a candidate mechanism for the unexplained one-off `windows/amd64` runtime
+  corruption, since a stray write into re-handed address space lands in whatever
+  the allocator gave that range next.
+
+- **`memfd_secret` descriptors are now close-on-exec.** The fd was created with
+  no flags and stayed inheritable across `ftruncate`, the guard reservation and
+  the `MAP_FIXED` — so a `fork`+`exec` from any other goroutine in that window
+  handed the child a live descriptor to the secret pages, making the strongest
+  allocation tier the one that leaked across `exec`. The flag is `O_CLOEXEC`,
+  not the `FD_CLOEXEC` the man page names: the kernel tests `flags & O_CLOEXEC`
+  and `EINVAL`s anything else, so the wrong bit would have silently dropped
+  every allocation to the weaker anon path. An `EINVAL` is retried bare with
+  `fcntl(F_SETFD)` instead, so a kernel that disagrees still gets close-on-exec
+  and never a tier downgrade.
+
+- **`InstallTerminationWipe` no longer claims to terminate the process on
+  Windows.** `os.Process.Signal` there supports only `os.Kill` and rejects both
+  `os.Interrupt` and SIGTERM, so the re-raise was a guaranteed no-op whose error
+  was discarded: the process ran on past Ctrl-C with every secret already
+  zeroed, while the documentation said it exited. The failure is now logged and
+  the platform limit is documented. The behaviour is deliberately not escalated
+  to a forced `os.Exit`, because the installer promises never to take the exit
+  away from a co-installed graceful shutdown.
+
+- **`secmem-lint` no longer certifies code it never examined.** Eight
+  false-negative classes, every one of which reported clean rather than
+  reporting a limitation:
+
+  - Assignment escapes matched only a bare identifier, so `s.field = b`,
+    `m[k] = b`, `out[i] = b` and `*p = b` were all silently clean — stashing a
+    borrowed slice in a struct field being the most natural way to leak one.
+    Targets are now classified by shape, with a local struct or array value
+    still correctly treated as staying inside the lease.
+  - Logging **methods** could never match. The sink table is keyed by
+    `import/path.Func`, so `sl.Info(b)`, `l.Printf("%s", b)` and
+    `slog.Default().Warn(…, b)` all resolved their receiver to a variable or a
+    call result rather than to a package name. Now matched by receiver type, so
+    an unrelated `Info` method is still not swept in.
+  - `append(dst, b[:n]...)` escaped unflagged, because only a bare identifier
+    was matched where the rest of the file already used `refersToParam`.
+  - `panic(b)` was not flagged, though the value lands in the runtime traceback
+    and in any `recover()`.
+  - Sink table omissions: `log.Panicln` (both its siblings were present),
+    `log/slog.Log`, `log/slog.LogAttrs`, `fmt.Append`, `fmt.Appendf`,
+    `fmt.Appendln`.
+  - The reentrancy set omitted `SetByteAt`, which takes the **exclusive** lock
+    and is therefore an unconditional self-deadlock, and the `rLock`
+    inspectors `Len`, `MappedLen`, `IsSealed`, `IsDestroyed` — a nested read
+    acquire deadlocks as soon as a writer queues between the two, because the
+    lock is writer-preferring.
+
+  All fifteen new fixture cases were verified to fail against the previous
+  analyzer, so none of them is a vacuous assertion.
+
+  The improved analyzer immediately caught a real instance in 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.
+  Fixed to use `len(b)`, which the borrowed slice already carries and which
+  needs no lock — the pattern the example should have been demonstrating.
+
+- **`redact` credential rules missed the shape structured logs actually emit.**
+  The Tier-1 patterns were `field[=:]\s*\S+`, which requires the separator to
+  follow the key immediately — so `{"password": "hunter2"}` matched nothing at
+  all and went to the sink in full. `\S+` also stops at the first space, so
+  `password="hunter 2 correct horse"` was only partly masked, leaving the rest
+  of the secret in the message. All five fields are now built by one helper that
+  allows a quoted key and consumes a quoted value whole.
+
+- **`redact`'s CWE-117 backstop passed the C1 controls it claimed to strip.**
+  `stripNonPrintable` tested `r >= 32 && r != 127`, which lets every C1 code
+  point (U+0080–U+009F) through while the doc comment said C0/C1. That includes
+  U+009B, the single-character CSI, which a terminal decoding the stream as
+  Latin-1/ISO-2022 acts on exactly as it would on the two-byte `ESC [` the ansi
+  rule strips — so the backstop was bypassable by spelling the escape
+  differently. Invalid UTF-8 bytes are now reported as redacted rather than
+  silently becoming U+FFFD.
+
+- **`redact`'s allowlist switched itself off when its label appeared twice.**
+  `isAllowlisted` used `FindStringIndex`, which returns the EARLIEST match, and
+  compared that match's end against the credential's start. A message mentioning
+  the label anywhere earlier therefore failed the comparison and redacted a
+  value the allowlist existed to exempt. All occurrences are now considered.
+
+- **`redact.Handler` misfiled `WithAttrs` attributes into groups opened later.**
+  It held them and re-added them to every record, so the inner handler emitted
+  them at whatever nesting it had reached — meaning
+  `log.With("req", id).WithGroup("db").Info(...)` produced
+  `{"db":{"req":…}}` instead of `{"req":…,"db":{…}}`, violating the positional
+  guarantee in `slog.Handler`'s contract. They are now handed to the inner
+  handler at the point they are added, which is where that decision belongs.
+
 - `WipeAllSecrets` no longer lets one borrowed buffer strand another's secret.
   The emergency wipe's second pass blocked on the deferred regions sequentially,
   in map-iteration order. Because `tryWipeInPlace` defers a region whose lock is

example_test.go

@@ -39,7 +39,13 @@ func ExampleScope() {
 			return err
 		}
 		return buf.WithBytesErr(func(b []byte) error {
-			fmt.Println("filled", buf.Len(), "bytes")
+			// len(b), not buf.Len(): the borrowed slice already carries the
+			// length and needs no lock, whereas Len takes the read lock a
+			// second time from inside the borrow. That is not reentrant — the
+			// lock is writer-preferring, so the nested acquire waits behind any
+			// queued writer and deadlocks the moment a Destroy or an emergency
+			// wipe arrives between the two.
+			fmt.Println("filled", len(b), "bytes")
 			return nil
 		})
 	})

mlock_linux.go

@@ -174,7 +174,36 @@ func allocMemfdSecret(pageSize, rounded, total int) (region secRegion, noFork bo
 	if unsafe.Sizeof(uintptr(0)) != 8 {
 		return secRegion{}, false, errors.New("memfd_secret: requires a 64-bit architecture")
 	}
-	fd, _, errno := unix.Syscall(sysMemfdSecret, 0, 0, 0)
+	// Close-on-exec, asked for at creation. Without it the descriptor stays
+	// inheritable for the whole window below — ftruncate, the guard
+	// reservation, and the MAP_FIXED — and a fork+exec from any other
+	// goroutine during it hands the child a live descriptor to the secret
+	// pages. secretmem is readable through that fd, so the strongest tier
+	// would be the one that leaks across exec.
+	//
+	// The bit is O_CLOEXEC, not FD_CLOEXEC. memfd_secret(2)'s man page names
+	// FD_CLOEXEC, but the kernel tests `flags & O_CLOEXEC` and returns EINVAL
+	// for anything outside SECRETMEM_FLAGS_MASK|O_CLOEXEC. Passing FD_CLOEXEC
+	// (bit 0) would therefore not merely fail to set close-on-exec — it would
+	// fail the syscall outright and silently drop every allocation to the
+	// weaker L3 path, which is worse than the leak being fixed.
+	//
+	// That distinction is kernel-version sensitive and this file cannot be
+	// executed from the maintainer's platform, so EINVAL is not trusted to
+	// mean "flag unsupported" and nothing else: it retries bare and sets
+	// close-on-exec with fcntl instead. Slightly larger window than the
+	// atomic form, still far smaller than none, and a tier downgrade is
+	// impossible either way.
+	fd, _, errno := unix.Syscall(sysMemfdSecret, uintptr(unix.O_CLOEXEC), 0, 0)
+	if errno == unix.EINVAL {
+		fd, _, errno = unix.Syscall(sysMemfdSecret, 0, 0, 0)
+		if errno == 0 {
+			if _, ferr := unix.FcntlInt(fd, unix.F_SETFD, unix.FD_CLOEXEC); ferr != nil {
+				_ = unix.Close(int(fd))
+				return secRegion{}, false, fmt.Errorf("memfd_secret: set FD_CLOEXEC: %w", ferr)
+			}
+		}
+	}
 	if errno != 0 {
 		return secRegion{}, false, errno // ENOSYS = kernel too old / not built; EPERM = lockdown
 	}