Skip to content

fix: DetectPostHandshakeRecordsLens background probe bugs (panic, leak, race)#36

Open
hexonal wants to merge 1 commit into
XTLS:mainfrom
hexonal:pr/record-detect-fixes
Open

fix: DetectPostHandshakeRecordsLens background probe bugs (panic, leak, race)#36
hexonal wants to merge 1 commit into
XTLS:mainfrom
hexonal:pr/record-detect-fixes

Conversation

@hexonal

@hexonal hexonal commented Jul 16, 2026

Copy link
Copy Markdown

Summary

DetectPostHandshakeRecordsLens (record_detect.go) is invoked unconditionally from NewListener() 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:

length := int(binary.BigEndian.Uint16(data[3:5])) + 5
...
postHandshakeRecordsLens = append(postHandshakeRecordsLens, length)
data = data[length:]

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, so data may be a partial read. Without a bounds check, data[length:] panics with slice bounds out of range. Since this runs unrecovered in a background goroutine spawned by NewListener(), 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 length against len(data) before slicing.

2. No panic recovery in either probe goroutine

Neither background goroutine in DetectPostHandshakeRecordsLens recovers from a panic (this one or any other). Since NewListener spawns 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 any net.Dial/handshake error a few lines above).

3. Two smaller issues found while reviewing the same function

  • The two net.Dial'd probe connections (target) were never Close()'d — a bounded (one per unique 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 does _, err = c.Conn.Read(buf), assigning into errWrite's own named return value — which races unsynchronized against every return c.Conn.Write(b) in the same method (go test -race flags this). Switched to a local readErr variable; nothing here needs to surface the read error to the caller.

Testing

Verified #1 and #2 with a standalone repro (a net.Pipe()-backed PostHandshakeRecordDetectConn fed a truncated final record): panics on the unpatched code, returns a clean EOF after the fix. go build ./... passes.

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.
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.

2 participants