Skip to content

Commit 42d99a3

Browse files
net.http: h2 server — close the final 7 h2spec conformance gaps (146/146) (#27627)
* net.http: h2 server — close the final 7 h2spec conformance gaps (146/146) Flow control (RFC 9113 §6.9/§6.9.1), in handle_control_frame so both the main dispatch and the pump_for_window stall path get the rules: - WINDOW_UPDATE with a zero increment: connection (stream 0) -> connection error PROTOCOL_ERROR; stream -> RST_STREAM(PROTOCOL_ERROR). - WINDOW_UPDATE growing a flow-control window past 2^31-1: connection -> GOAWAY(FLOW_CONTROL_ERROR); stream -> RST_STREAM(FLOW_CONTROL_ERROR). Validate-before-mutate: the new window is computed and checked before being committed. SETTINGS (RFC 9113 §6.5.2): - SETTINGS_ENABLE_PUSH accepts only 0 or 1; anything else is a connection error PROTOCOL_ERROR. Priority self-dependency (RFC 7540 §5.3.1, deprecated-but-validated in 9113): - A PRIORITY frame that depends on its own stream -> RST_STREAM(PROTOCOL_ERROR), sent at most once per id (locally_reset guard - no second RST on a stream we already reset). - A HEADERS frame (opening or trailer) whose priority section depends on its own stream: the block is still HPACK-decoded first (RFC 7541 §2.2 - the decoder is stateful), then the stream is reset with PROTOCOL_ERROR, via a self_dep flag mirroring the refused/over_end decode-then-RST pattern. With these, the server passes ALL 146 h2spec v2.6.0 cases (verified 5/5 deterministic runs at --timeout 5, plus 1 run after each review fix); the expected-failures baseline is now EMPTY, so CI fails on any h2spec failure. Tests: 9 new scripted-peer cases in h2_server_test.v (zero-increment conn/stream, overflow conn/stream, ENABLE_PUSH=2, HEADERS/PRIORITY/trailer self-dependency, repeated-PRIORITY single-RST). Full vlib/net/http suite green (23 passed, 2 skipped). * fix(h2): classify_stream must treat locally_reset ids as closed; sync h2spec docs RFC 9113 §5.1 defines "closed" as entered when either side sends RST_STREAM, and explicitly permits remembering a reset stream in that state for a bounded period (exactly what the locally_reset FIFO already implements). classify_stream only tested `stream_id & 1 == 1 && stream_id <= last_stream_id`, which misses any id we RST'd without it ever having advanced last_stream_id — a self-dependent PRIORITY frame (RFC 7540 §5.3.1) is legal on a stream the client never opened via HEADERS, so it can be locally_reset while last_stream_id still hasn't counted it. Consequence: an in-flight DATA/WINDOW_UPDATE/RST_STREAM for such an id, arriving after our RST but classified via the old rule, was misread as a frame on an IDLE stream (a connection PROTOCOL_ERROR) instead of drained per §6.4 — a spurious GOAWAY on legitimate late traffic. Fix: classify_stream checks c.locally_reset before the parity/last_stream_id test, so every caller (on_data, handle_control_frame's WINDOW_UPDATE arm, dispatch_frame's RST_STREAM arm) gets the correct closed/drain behavior from one place, rather than three duplicated per-branch checks that a future fourth caller could silently miss. Also syncs vlib/net/http/h2spec/{README.md,run_h2spec.sh} — both still described the pre-#27627 37-case baseline; the baseline has been empty (146/146) since #27627. Both from Codex, #27627 pullrequestreview-4618166175. * fix(h2): tighten h2spec README wording after conformance-doc sync Three follow-ups from Codex review 4619906229 on the prior doc-sync commit: - the replaced h2spec_expected_failures.txt table row was 193 chars, over the repo's 100-char markdown line limit — shortened. - "Four follow-up PRs" listed only three PR numbers (#27569, #27589, #27627) — corrected to "Three". - the empty-baseline statement conflicted with unchanged bullets below still describing "genuine conformance gaps" and instructing removal of lines as cases are fixed — reworded to note the baseline is currently empty while the tracking mechanism stays in place for future gaps. Doc-only change; verified with ./vnew check-md (0 errors) and a line-length scan. Both from Codex, #27627 pullrequestreview-4619906229. * fix(h2): self-dependent PRIORITY must not re-RST an already-closed stream The self-dependency guard for PRIORITY frames (RFC 7540 §5.3.1) exempted only `frame.stream_id in c.locally_reset` from re-sending RST_STREAM. That misses a stream that completed NORMALLY: run_request deletes it from c.streams without ever touching locally_reset, so classify_stream already correctly reports it as closed, but the old check saw "not locally_reset" and sent a fresh RST_STREAM anyway — itself a frame-on-a-closed-stream violation (RFC 9113 §5.1), since PRIORITY is permitted on closed streams but RST_STREAM is not. Fix: gate on `c.classify_stream(frame.stream_id) != .closed` instead of the locally_reset-only check. idle and active streams still get RST_STREAM (self-dependency remains a stream error per §5.3.1 regardless of whether the stream was ever opened); any already-closed stream — whether closed via locally_reset or via normal completion — is left alone. This reuses the same classify_stream generalization from the prior commit rather than adding a second ad-hoc exemption, so a future caller of this guard inherits the correct behavior automatically. Codex, #27627 pullrequestreview-4620220448. * fix(h2): self-dependent HEADERS over the concurrency limit must be PROTOCOL_ERROR finalize_headers checked s.refused (RFC 9113 §5.1.2, over the concurrency limit) before s.self_dep (RFC 7540 §5.3.1, priority self-dependency). A HEADERS frame that was both over-limit and self-dependent therefore answered RST_STREAM(REFUSED_STREAM) instead of PROTOCOL_ERROR. REFUSED_STREAM specifically tells the client the request is safe to retry unchanged elsewhere; a self-dependent request is malformed independent of the concurrency limit, so retrying it verbatim would just repeat the same error. Fix: swap the check order so self_dep is evaluated first — both branches still route through the identical decode-then-RST pattern (HPACK block already decoded above for dynamic-table sync), so there is no HPACK regression, only the reported error code changes. Codex, #27627 pullrequestreview-4620412384.
1 parent d21e26c commit 42d99a3

5 files changed

Lines changed: 520 additions & 40 deletions

File tree

vlib/net/http/h2_server.v

Lines changed: 88 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -172,6 +172,7 @@ mut:
172172
in_trailers bool // set once a trailer section (a 2nd HEADERS block) begins
173173
refused bool // §5.1.2: over the concurrency limit — decode then RST(REFUSED_STREAM)
174174
over_end bool // §5.1: HEADERS after END_STREAM — decoded for HPACK sync, then RST(STREAM_CLOSED)
175+
self_dep bool // RFC 7540 §5.3.1: HEADERS priority depends on itself — decode then RST(PROTOCOL_ERROR)
175176
}
176177

177178
// H2ServerConn drives one server-side HTTP/2 connection over a transport.
@@ -233,7 +234,11 @@ fn (mut c H2ServerConn) mark_locally_reset(id u32) {
233234
// - idle: never opened — an odd id above any we have accepted, OR any even
234235
// (server-initiated) id, since this server never opens push streams
235236
// - closed: a client (odd) id at or below the highest we have accepted that is
236-
// no longer in the map (already finished or reset)
237+
// no longer in the map (already finished or reset), OR any id we
238+
// have ourselves RST_STREAM'd (c.locally_reset) regardless of
239+
// last_stream_id — a self-dependent PRIORITY (RFC 7540 §5.3.1) is
240+
// legal on a stream the client never opened via HEADERS, so an id
241+
// can be locally-reset without ever having advanced last_stream_id
237242
enum H2StreamState {
238243
active
239244
idle
@@ -251,6 +256,17 @@ fn (c &H2ServerConn) classify_stream(stream_id u32) H2StreamState {
251256
if stream_id in c.streams {
252257
return .active
253258
}
259+
// An id we have ourselves RST_STREAM'd is closed no matter how it got reset:
260+
// checking this before the last_stream_id/parity test covers ids that were
261+
// never opened via HEADERS at all (e.g. a self-dependent PRIORITY frame on
262+
// an id the client never used) — without this, every caller of
263+
// classify_stream (on_data, handle_control_frame's WINDOW_UPDATE arm,
264+
// dispatch_frame's RST_STREAM arm) would misclassify a later in-flight frame
265+
// for that id as idle and force a connection error instead of draining it
266+
// per §6.4.
267+
if stream_id in c.locally_reset {
268+
return .closed
269+
}
254270
if stream_id & 1 == 1 && stream_id <= c.last_stream_id {
255271
return .closed
256272
}
@@ -372,7 +388,18 @@ fn (mut c H2ServerConn) dispatch_frame(frame H2Frame, mut handler Handler) ! {
372388
c.streams.delete(frame.stream_id)
373389
}
374390
H2PriorityFrame {
375-
// Priority is advisory; ignore.
391+
// Priority is advisory and otherwise ignored (deprecated in RFC 9113
392+
// §5.3), but RFC 7540 §5.3.1: a stream cannot depend on itself — a
393+
// self-dependency is a STREAM error PROTOCOL_ERROR. Skip the RST for
394+
// any ALREADY-CLOSED stream (locally reset OR completed normally via
395+
// run_request — classify_stream treats both as closed): PRIORITY is
396+
// legal on a closed stream, and RST_STREAM is not, so re-RST-ing one
397+
// would itself send a frame on a closed stream (§5.1).
398+
if frame.stream_dep == frame.stream_id && c.classify_stream(frame.stream_id) != .closed {
399+
c.send_rst_stream(frame.stream_id, .protocol_error)!
400+
c.mark_locally_reset(frame.stream_id)
401+
c.streams.delete(frame.stream_id)
402+
}
376403
}
377404
H2HeadersFrame {
378405
c.on_headers(frame, mut handler)!
@@ -417,10 +444,40 @@ fn (mut c H2ServerConn) handle_control_frame(frame H2Frame) ! {
417444
}
418445
}
419446
H2WindowUpdateFrame {
447+
inc := frame.window_size_increment
420448
if frame.stream_id == 0 {
421-
c.send_window += i64(frame.window_size_increment)
449+
// RFC 9113 §6.9: a zero increment on the connection is a connection
450+
// error PROTOCOL_ERROR. §6.9.1: growing the connection window past
451+
// 2^31-1 is a connection error FLOW_CONTROL_ERROR — send the correct
452+
// code before unwinding (serve()'s catch defaults to PROTOCOL_ERROR).
453+
if inc == 0 {
454+
return error('h2 server: connection WINDOW_UPDATE with a zero increment (RFC 9113 §6.9 PROTOCOL_ERROR)')
455+
}
456+
new_window := c.send_window + i64(inc)
457+
if new_window > i64(0x7fff_ffff) {
458+
c.send_goaway(.flow_control_error,
459+
'connection flow-control window exceeds 2^31-1') or {}
460+
return error('h2 server: connection flow-control window exceeded 2^31-1 (RFC 9113 §6.9.1 FLOW_CONTROL_ERROR)')
461+
}
462+
c.send_window = new_window
422463
} else if mut s := c.streams[frame.stream_id] {
423-
s.send_window += i64(frame.window_size_increment)
464+
// Stream-scoped versions of the same rules are STREAM errors
465+
// (RFC 9113 §6.9/§6.9.1): reset the offending stream, keep the
466+
// connection alive.
467+
if inc == 0 {
468+
c.send_rst_stream(s.id, .protocol_error)!
469+
c.mark_locally_reset(s.id)
470+
c.streams.delete(s.id)
471+
return
472+
}
473+
new_window := s.send_window + i64(inc)
474+
if new_window > i64(0x7fff_ffff) {
475+
c.send_rst_stream(s.id, .flow_control_error)!
476+
c.mark_locally_reset(s.id)
477+
c.streams.delete(s.id)
478+
return
479+
}
480+
s.send_window = new_window
424481
} else if c.classify_stream(frame.stream_id) == .idle {
425482
// RFC 9113 §5.1: a WINDOW_UPDATE on an idle stream is a connection
426483
// error PROTOCOL_ERROR. On a closed stream it is ignored.
@@ -442,6 +499,11 @@ fn (mut c H2ServerConn) apply_settings(settings []H2Setting) ! {
442499
c.encoder.pending_max_table_size = int(s.value)
443500
}
444501
h2_settings_enable_push {
502+
// RFC 9113 §6.5.2: any value other than 0 or 1 is a connection
503+
// error PROTOCOL_ERROR.
504+
if s.value > 1 {
505+
return error('h2 server: SETTINGS_ENABLE_PUSH ${s.value} is not 0 or 1 (RFC 9113 §6.5.2 PROTOCOL_ERROR)')
506+
}
445507
c.peer.enable_push = s.value != 0
446508
}
447509
h2_settings_max_concurrent_streams {
@@ -540,6 +602,7 @@ fn (mut c H2ServerConn) on_headers(frame H2HeadersFrame, mut handler Handler) !
540602
end_headers: frame.end_headers
541603
end_stream: frame.end_stream
542604
send_window: i64(c.peer.initial_window_size)
605+
self_dep: frame.has_priority && frame.stream_dep == frame.stream_id
543606
}
544607
// RFC 9113 §5.1.2: a stream that would exceed the concurrency limit we
545608
// advertised is refused. We still assemble and HPACK-decode its header block
@@ -615,6 +678,19 @@ fn (mut c H2ServerConn) finalize_headers(mut s H2ServerStream, mut handler Handl
615678
return error('h2 server: HPACK decode error (COMPRESSION_ERROR)')
616679
}
617680
s.headers_done = true
681+
// RFC 7540 §5.3.1: a stream cannot depend on itself; a HEADERS priority
682+
// self-dependency is a STREAM error PROTOCOL_ERROR. Checked BEFORE the
683+
// concurrency-limit refusal below: REFUSED_STREAM tells the client the
684+
// request is safe to retry unchanged, but a self-dependent request is
685+
// malformed regardless of the concurrency limit — retrying it would only
686+
// repeat the same error, so a stream that is both over-limit and
687+
// self-dependent must be reported as PROTOCOL_ERROR, not REFUSED_STREAM.
688+
if s.self_dep {
689+
c.send_rst_stream(s.id, .protocol_error)!
690+
c.mark_locally_reset(s.id)
691+
c.streams.delete(s.id)
692+
return
693+
}
618694
// §5.1.2: an over-limit stream was decoded only to keep HPACK in sync; refuse
619695
// it now without validating or running the request.
620696
if s.refused {
@@ -645,6 +721,10 @@ fn (mut c H2ServerConn) finalize_headers(mut s H2ServerStream, mut handler Handl
645721
fn (mut c H2ServerConn) on_trailers(mut s H2ServerStream, frame H2HeadersFrame, mut handler Handler) ! {
646722
s.in_trailers = true
647723
s.end_stream = frame.end_stream
724+
// RFC 7540 §5.3.1 applies to ANY HEADERS carrying priority, trailers included.
725+
if frame.has_priority && frame.stream_dep == frame.stream_id {
726+
s.self_dep = true
727+
}
648728
s.trailer_block << frame.fragment
649729
if !frame.end_headers {
650730
c.awaiting_cont = frame.stream_id
@@ -670,7 +750,10 @@ fn (mut c H2ServerConn) finalize_trailers(mut s H2ServerStream, mut handler Hand
670750
return
671751
}
672752
// A well-decoded but malformed trailer section is a STREAM error (§8.1.1).
673-
mut reason := if !s.end_stream {
753+
mut reason := if s.self_dep {
754+
// RFC 7540 §5.3.1: a stream cannot depend on itself.
755+
'trailing HEADERS priority depends on itself'
756+
} else if !s.end_stream {
674757
// A trailer section MUST terminate the stream (RFC 9113 §8.1).
675758
'trailing HEADERS without END_STREAM'
676759
} else {

0 commit comments

Comments
 (0)