net.http: fix four h2 server correctness gaps found in #27435 review#27469
Merged
JalonSolov merged 2 commits intoJun 18, 2026
Merged
Conversation
…view 1. apply_settings overflow guard (RFC 7540 §6.5.3) SETTINGS_INITIAL_WINDOW_SIZE values above 2^31-1 must be rejected as FLOW_CONTROL_ERROR; previously they were stored unchecked, inflating every active stream send-window to ~4 GB. apply_settings now returns ! and validates the new value before applying the delta. 2. Double unmark_idle race on fd recycling (handle_conn) handle_conn's outer defer unconditionally called unmark_idle after serve_h2_conn_with_idle_tracker returned, but serve() already owns mark_idle/unmark_idle for the connection lifetime. After serve()'s defer fires, the OS can recycle the fd; the stale unmark then evicts the new connection from the tracker, preventing close_idle from shutting it down. Fixed by tracking is_h2 and skipping the outer unmark on the H2 path. 3. close_idle write truncation comment Update serve()'s comment to note that response writes, not just frame reads, may be truncated mid-DATA-frame on shutdown. 4. Extract handle_control_frame to deduplicate dispatch_frame/pump_for_window SETTINGS, PING, and WINDOW_UPDATE handling was copy-pasted verbatim in both functions. Any fix to apply_settings (e.g. finding 1) would not automatically propagate to pump_for_window. Extracted into a shared handle_control_frame helper called from both dispatch sites. Co-Authored-By: WOZCODE <contact@withwoz.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 22d335a6f7
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
…erver Three RFC 7540 correctness fixes in H2ServerConn: - apply_settings: send FLOW_CONTROL_ERROR (not PROTOCOL_ERROR) before returning when SETTINGS_INITIAL_WINDOW_SIZE exceeds 2^31-1 (S6.5.3). send_goaway now sets closing=true so the outer handler skips the duplicate PROTOCOL_ERROR GOAWAY. - apply_settings: validate SETTINGS_MAX_FRAME_SIZE is in [16384, 16777215] (S6.5.2) before storing it. An out-of-range value was previously stored and could cause an infinite loop in send_body / send_header_block if the peer set it to 0. - send_goaway: set closing=true after a successful frame write so that serve_h2_conn_with_idle_tracker can skip the redundant PROTOCOL_ERROR GOAWAY it previously always sent. Co-Authored-By: WOZCODE <contact@withwoz.com>
Collaborator
|
@codex review |
|
Codex Review: Didn't find any major issues. 👍 Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
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
2 participants
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.
Four correctness fixes identified during code review of #27435 (merged) but
not included before merge.
1.
apply_settingsoverflow guard (RFC 7540 §6.5.3)SETTINGS_INITIAL_WINDOW_SIZEvalues above 2^31−1 must be rejected asFLOW_CONTROL_ERROR; previously they were stored unchecked, inflating everyactive stream's send-window to ~4 GB.
apply_settingsnow returns!andvalidates the value before applying the delta to open streams.
2. Double
unmark_idlerace on fd recycling (handle_conn)handle_conn's outerdeferunconditionally calledunmark_idleafterserve_h2_conn_with_idle_trackerreturned, butserve()already owns themark_idle/unmark_idlelifetime for the connection. Afterserve()'sdefer fires, the OS can recycle the fd number for a new connection that
has already called
mark_idle; the stale unmark then silently evicts thenew connection from the tracker, preventing
close_idlefrom shutting itdown and leaking its reader goroutine.
Fix: track
is_h2inhandle_connand skip the outer unmark on the H2path.
3.
serve()write-truncation commentThe comment said shutdown interrupts "an h2 request in flight", implying
only the frame-read side. Added an explicit note that response writes may
also be truncated mid-DATA-frame, since the connection handle stays
registered with
TlsIdleConnTrackerfor the connection's entire lifetime(the whole point of #27435).
4. Extract
handle_control_frame— eliminate dispatch/pump duplicationSETTINGS,PING, andWINDOW_UPDATEhandling was copy-pasted verbatimin both
dispatch_frameandpump_for_window. Finding 1's change toapply_settings(!return) would not have propagated topump_for_windowautomatically; now both call the samehandle_control_framehelper and any future change propagates to bothcallers automatically.
Verification
vlib/net/http/h2_server_test.vpasses (GCC; TCC fails unrelated to thesechanges due to stale mbedtls threading objects from #27437).
Fixes items identified in the review of #27435.
🤖 Generated with Claude Code