fix: DetectPostHandshakeRecordsLens background probe bugs (panic, leak, race)#36
Open
hexonal wants to merge 1 commit into
Open
fix: DetectPostHandshakeRecordsLens background probe bugs (panic, leak, race)#36hexonal wants to merge 1 commit into
hexonal wants to merge 1 commit into
Conversation
Three bugs found while running this in production (invoked unconditionally
from NewListener() for every REALITY inbound):
1. PostHandshakeRecordDetectConn.Read() did `data = data[length:]` with no
bounds check. A truncated final record from the disguise dest (network
cut/RST/read-deadline firing mid-record) can legitimately report a
length longer than what actually arrived - io.ReadAll's error is
discarded, so `data` may be a partial read. Without the check,
`data[length:]` panics with "slice bounds out of range", and since this
runs unrecovered in a background goroutine spawned by NewListener, it
takes down the entire process - on every single REALITY inbound, not
just this calibration probe. Fixed with a length-vs-len(data) check
before slicing.
2. Neither background probe goroutine recovers from a panic (this one or
any other). Since NewListener spawns them with no caller to propagate
an error to, an unrecovered panic here is a process-wide crash triggered
by nothing more than normal TLS behavior from the disguise dest. Added
`defer func() { recover() }()` to both.
3. Smaller issues found while reviewing the same function:
- The two `net.Dial`'d probe connections were never Close()'d - a
bounded (one per unique dest/SNI/alpn key, gated by the existing
LoadOrStore) but real leak of a goroutine+socket on every REALITY
listener startup. Added `defer target.Close()` to both.
- CCSDetectConn.Write's background reader goroutine assigned into
`err` - Write's own named return value - racing unsynchronized
against every `return c.Conn.Write(b)` in the same method (flagged by
`go test -race`). Switched to a local variable; nothing here needs to
surface the read error to the caller.
Verified XTLS#1 and XTLS#2 with a standalone repro (not included in this diff):
a net.Pipe()-backed PostHandshakeRecordDetectConn fed a truncated final
record panics on the unpatched code and returns a clean EOF after the fix.
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
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.
Summary
DetectPostHandshakeRecordsLens(record_detect.go) is invoked unconditionally fromNewListener()for every REALITY inbound, spawning two background calibration probes per unique(dest, serverName, alpn)key against the disguise destination. Found and fixed three bugs while running this in production:1. Slice-bounds panic (crashes the whole process)
PostHandshakeRecordDetectConn.Read()does:with no check that
length <= len(data). A truncated final record from the disguise dest (network cut/RST/read-deadline firing mid-record) can legitimately report a length longer than what actually arrived —io.ReadAll's error is discarded a few lines up, sodatamay be a partial read. Without a bounds check,data[length:]panics withslice bounds out of range. Since this runs unrecovered in a background goroutine spawned byNewListener(), this takes down the entire process — reachable on every single REALITY inbound listener, not just this one calibration probe, purely from ordinary network conditions against the disguise site (nothing adversarial required).Fix: bounds-check
lengthagainstlen(data)before slicing.2. No panic recovery in either probe goroutine
Neither background goroutine in
DetectPostHandshakeRecordsLensrecovers from a panic (this one or any other). SinceNewListenerspawns them with no caller able to observe or handle an error, an unrecovered panic here is a process-wide crash triggered by nothing more than unusual-but-legitimate TLS behavior from the disguise destination.Fix: added
defer func() { recover() }()to both goroutines — this is a best-effort calibration probe with no caller to report to, so swallowing a panic here is the correct failure mode (same as the existing pattern of silently returning on anynet.Dial/handshake error a few lines above).3. Two smaller issues found while reviewing the same function
net.Dial'd probe connections (target) were neverClose()'d — a bounded (one per unique key, gated by the existingLoadOrStore) but real leak of a goroutine+socket on every REALITY listener startup. Addeddefer target.Close()to both.CCSDetectConn.Write's background reader goroutine does_, err = c.Conn.Read(buf), assigning intoerr—Write's own named return value — which races unsynchronized against everyreturn c.Conn.Write(b)in the same method (go test -raceflags this). Switched to a localreadErrvariable; nothing here needs to surface the read error to the caller.Testing
Verified #1 and #2 with a standalone repro (a
net.Pipe()-backedPostHandshakeRecordDetectConnfed a truncated final record): panics on the unpatched code, returns a cleanEOFafter the fix.go build ./...passes.