Fix timeout and panic-recovery bugs in connection processing#744
Open
Seanstoppable wants to merge 1 commit into
Open
Fix timeout and panic-recovery bugs in connection processing#744Seanstoppable wants to merge 1 commit into
Seanstoppable wants to merge 1 commit into
Conversation
Three related correctness fixes in processing.go: - TCP dialer: re-rooting the connection context at the main context dropped the per-target SessionTimeout (and leaked the previous cancel func), so the target timeout was no longer enforced after the dial. Preserve SessionTimeout and cancel the prior context. - TLS wrapper: the handshake timeout block was gated on ConnectTimeout but applied TLSHandshakeTimeout. This disabled the handshake timeout when ConnectTimeout was 0, and could apply a zero (already-expired) deadline. Gate on TLSHandshakeTimeout instead. - grabTarget: recovering from a scanner panic in a deferred function unwound grabTarget and returned a nil *Grab, discarding all results. Recover inside a per-scanner closure so the panic is recorded and BuildGrabFromInputResponse is still reached. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
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.
Three related correctness fixes in
processing.go, surfaced by AI code review.1. TCP dialer drops the per-target timeout (High)
After dialing, the connection's context was reset to the bare parent context:
This discarded the
SessionTimeout(target timeout) deadline thatNewTimeoutConnectionhad installed, socheckContext()no longer enforced the target timeout, and the priorcancelfunc was leaked. Now we cancel the previous context and re-root at the parent while preservingSessionTimeout.2. TLS handshake timeout gated on the wrong flag (Medium)
The handshake timeout block was gated on
baseFlags.ConnectTimeoutbut appliedtlsFlags.TLSHandshakeTimeout. WithConnectTimeout=0the handshake timeout was disabled entirely; withConnectTimeout>0andTLSHandshakeTimeout=0it applied an already-expired deadline. Now gated onTLSHandshakeTimeout > 0.3. Scanner panic recovery returns a nil Grab (High)
grabTargetrecovered scanner panics in a deferred function, but with an unnamed*Grabreturn this unwound the function and returnednil, discarding all results (caller then encodes a nil grab). The recovery now runs inside a per-scanner closure, so the panic is recorded as an error result andBuildGrabFromInputResponseis still reached.