Serialize record emission, because the sequence number is the nonce - #2
Conversation
Write releases wmu around each writeRecord so a slow socket cannot block other writers, and guards re-entry with the flushing flag. That flag only excludes Write from Write. Close reaches writeRecord through writeSized, which takes the wmu Write just freed and never consults flushing, so a close_notify could seal and write concurrently with a data flush. Both halves of writeRecord needed the same lock, for different reasons. sendSeq++ is not atomic, so two sealers could take the same sequence number -- and the sequence number IS the AEAD nonce, so that is nonce reuse under one key. For AES-GCM that is authentication key recovery and a plaintext XOR leak, not a decrypt failure that shows up as a broken connection. Separately, even sealers that take distinct numbers must reach the wire in that order, because the peer decrypts against its own monotonic counter; interleaved raw.Write calls can also split a record. wireMu covers seal, increment and socket write as one step. It cannot be folded into wmu without giving up the property Write's release was there for. The two tests pin the two halves. TestCloseNotifyCannotOvertakeAFlush parks the data record inside the socket write and requires close_notify to wait behind it, then decrypts the recorded wire with a peer Conn -- which is the invariant that actually matters, since a monotonic counter cannot authenticate a reordered record. TestConcurrentWritesAndCloseAreRaceFree runs the same overlap ungated so -race reports the unsynchronised sendSeq if wireMu is ever removed. Without the fix the first fails deterministically and the second reports a data race. Found while wiring yamux over twiddle in getlantern/lantern-box#319, which is the first caller to give one Conn a dedicated write goroutine alongside a reader that closes the session. A single-writer caller cannot reach it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017gn6KuHUL766qQNn8m1fu1
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (1)
Limit details: You’ve used the included review currently available. 📝 WalkthroughWalkthroughThe connection now serializes TLS record sealing and socket writes with ChangesTLS wire serialization
Estimated code review effort: 3 (Moderate) | ~25 minutes Merge Risk: ⚪ Minimal · up to This change serializes TLS record transmission so pending data remains ordered before close_notify, with concurrent write and close coverage. No current merge-blocking risk remains. Sequence Diagram(s)sequenceDiagram
participant Writer
participant Conn
participant Socket
Writer->>Conn: writeRecord
Conn->>Conn: acquire wireMu
Conn->>Conn: seal record and advance sequence
Conn->>Socket: write encrypted record
Conn->>Conn: release wireMu
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Usage-based review receipt
Note This review was completed with usage-based billing: files reviewed beyond your plan's included limits are billed at $0.25/file. View usage-based billing. Comment |
There was a problem hiding this comment.
🔵 Needs a closer look
It changes synchronization around cryptographic record sealing/emission (security-critical concurrency), so it warrants final human review despite the small diff and added tests.
Pull request overview
This PR fixes a concurrency hazard in Conn record emission where concurrent Write and Close paths could overlap inside writeRecord, risking AEAD nonce reuse and/or on-wire reordering. It introduces a dedicated mutex to serialize the “seal + sendSeq increment + raw.Write” step, and adds targeted tests that reproduce the issue and catch regressions (including under -race).
Changes:
- Add
wireMuto serializewriteRecordacross all call paths (WriteandCloseviawriteSized). - Guard
writeRecordwithwireMuto ensure nonce/sequence safety and preserve record ordering on the wire. - Add race/ordering regression tests (
TestCloseNotifyCannotOvertakeAFlush,TestConcurrentWritesAndCloseAreRaceFree).
File summaries
| File | Description |
|---|---|
| conn.go | Adds wireMu and uses it in writeRecord to serialize sealing and socket writes, preventing concurrent emission. |
| conn_race_test.go | Adds deterministic-ish regression tests for close-notify ordering and a -race overlap scenario. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
The ordering test proved a negative -- close_notify did NOT reach the wire -- against a 150ms window, which passes for the wrong reason if the Close goroutine simply never got scheduled inside it. A false pass there is worse than a flake: the test would go green while not exercising the bug at all. It now signals that the goroutine started, widens the window to 2s, and watches the wire rather than only the Close return. Without wireMu, Close's record is not held by the gate -- only the first write is -- so it lands in the recording directly, which is the symptom itself rather than a proxy for it. Reverting the fix now fails in 10ms on the record count instead of waiting out the window. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017gn6KuHUL766qQNn8m1fu1
There was a problem hiding this comment.
🟢 Approval recommended
The change correctly serializes record emission at the only raw.Write site and is backed by targeted tests that would fail (including under -race) on regression.
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 0 new
- Review effort level: Lite
|
Reviewed d80cb2b on top of current main 6b9909c in an isolated merge worktree. The merge is clean and preserves the full-handshake changes from #3. Validation passed: go build ./..., go vet ./..., go test -race -count=1 -timeout=5m ./..., 10 race-enabled repetitions of both PR regressions, and 30 race-enabled repetitions of a temporary blocked-write/deadline shutdown diagnostic. The temporary diagnostic was not added to the PR. wireMu correctly covers Seal, sendSeq advancement, and raw.Write together. Lock ordering introduces no cycle: writeRecord releases wireMu before Write reacquires wmu. Shutdown completes once a blocked socket write is released by a deadline. Non-blocking existing limitation: Close still attempts a synchronous close_notify without installing a write deadline, so an unresponsive peer can indefinitely delay Close if no caller deadline is set. This predates the PR and merits a separate bounded-shutdown fix; it is not a reason to retain the nonce race. No merge-blocking findings. Merging with user authorization. lantern-box #321 was not modified. |
getlantern/twiddle#2 is merged. Conn.writeRecord now holds a dedicated mutex across the seal, the sequence increment and the socket write, which it did not before: Write releases wmu around each record so a slow socket cannot block other writers, and Close reached writeRecord through writeSized on that freed lock. Two sealers could take one sendSeq, and the sequence number is the AEAD nonce, so that was nonce reuse under a single key rather than a decrypt failure -- and records could reach the wire out of the order they were sealed in, which the peer's monotonic counter cannot authenticate either. It matters here because UoT gives a twiddle Conn the ordinary sing-box shape of a bidirectional copy: one direction writing while the other closes on error is exactly the overlap involved. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017gn6KuHUL766qQNn8m1fu1
Conn.Writereleaseswmuaround eachwriteRecordso a slow socket cannot block other writers, and guards re-entry with aflushingbool. That flag only excludes Write from Write.ClosereacheswriteRecordthroughwriteSized, which takes thewmuthatWritejust freed and never consultsflushing— so a close_notify can seal and write concurrently with a data flush.sequenceDiagram autonumber participant W as Conn.Write<br/>conn.go:201 participant C as Conn.Close<br/>conn.go:358 participant R as writeRecord<br/>conn.go:236 W->>W: conn.go:213<br/>flushing = true Note over W: conn.go:221<br/>wmu released around the record ⚠️ W->>R: writeRecord, data C->>C: conn.go:261<br/>writeSized takes the freed wmu Note over C: flushing is never consulted C->>R: writeRecord, close_notify rect rgba(255, 200, 200, 0.3) Note over R: conn.go:251<br/>Seal with nonceFor sendIV, sendSeq — then sendSeq++ 🐛 end R-->>W: two sealers, one counterWhy both halves of
writeRecordneed the locksendSeq++is not atomic, so two sealers can take the same number. The sequence number is the AEAD nonce, so that is nonce reuse under one key. For AES-GCM that is authentication-key recovery and a plaintext XOR leak — not a decrypt failure that surfaces as a broken connection.raw.Writecalls can also split a record.So an atomic increment would not have been enough: the seal, the increment and the socket write have to be one step.
wireMudoes exactly that, and cannot be folded intowmuwithout giving up the propertyWrite's release exists for.Tests
Each pins one half, and each fails without the fix:
wireMuTestCloseNotifyCannotOvertakeAFlushFAIL: close_notify was emitted while a flush held the wireTestConcurrentWritesAndCloseAreRaceFreeWARNING: DATA RACEonsendSeqThe first parks the data record inside the socket write via a gate conn, requires close_notify to wait behind it, and then decrypts the recorded wire with a peer
Conn— that last step is the invariant that actually matters, since a monotonic counter cannot authenticate a reordered record. The second runs the same overlap ungated so-racecatches a regression.go build ./...,go vet ./...andgo test -race ./...are clean.How it was found
Wiring yamux over twiddle in getlantern/lantern-box#319, which is the first caller to give one
Conna dedicated write goroutine (Session.sendLoop) alongside a reader that closes the session (Session.recv→exitErr→Close). A single-writer caller cannot reach it, which is why it survived until now.🤖 Generated with Claude Code
https://claude.ai/code/session_017gn6KuHUL766qQNn8m1fu1
Summary by CodeRabbit
Bug Fixes
Tests