fix(h2,sse,eventloop): close out the HTTP/2 review issues (#234-#238), SSE id sanitization (#265) and a growth-stable connection table (#343) - #384
Merged
Conversation
…t end to end The control-frame budget audit of #234 was mostly applied in #242 and reworked into the credit/decay model in #335, but the issue stayed open and nothing in tests/ exercised it. Re-reading every vector against the current codec found three charges still missing, all of the same shape: an overhead frame that reaches its reply or its early return before noteControlFrame ever runs. A SETTINGS ACK returned unbudgeted, exactly as a PING ACK once did. We emit our SETTINGS once, so at most one ACK is ever solicited and the rest are pure overhead, so they are now charged like the PING ACK they mirror. A zero-increment WINDOW_UPDATE naming a stream id we have seen answered with a 13-byte RST_STREAM and returned before the charge. On an open stream that is self-limiting (the reply tears the stream down), but on an already-closed id it tears nothing down, so the peer could repeat it for the life of the connection, one RST per frame, against a receive buffer it never drains. It is charged before the reply, and the reply is skipped once the charge has closed the connection. A stream-level WINDOW_UPDATE was never charged at all. That left the cheapest flood of the whole set: 13 bytes a frame against a stream the server owes no bytes on (a request whose announced body never arrives, or a producer that has not written yet), where the scheduler pass the update forces can emit nothing. Such an update now goes through the same credit-then-charge path as the connection-level and closed-stream ones, so a client returning flow-control credit for response DATA it consumed still rides the credit those bytes earned (#335) while a peer we send nothing to trips the budget. An update that genuinely unblocks a send is untouched, and the stream is enqueued either way so a later connection WINDOW_UPDATE still finds it ready. tests/test_http2_budget.nim is the regression suite the issue never had. With maxControlFrames lowered to 20 it floods, one vector per test, PING ACKs, SETTINGS ACKs, received GOAWAYs, unknown frame types, stream-level WINDOW_UPDATEs with increment 1, zero-increment WINDOW_UPDATEs on a closed stream, self-dependent PRIORITY on a used stream and DATA on a closed stream, and asserts GOAWAY(ENHANCE_YOUR_CALM) plus a bounded RST_STREAM count where the frame draws a reply. A self-dependent PRIORITY on an idle stream id asserts the RFC 9113 5.1 rule the issue also flagged: GOAWAY(PROTOCOL_ERROR) and no RST_STREAM for an id that was never opened. One SETTINGS frame packed with 40 INITIAL_WINDOW_SIZE entries covers the per-entry charge, and an interleave of one GET per burst of ACKs covers the counter that used to be zeroed by every accepted request. Two further tests hold the converse, that a few control frames or window updates per request are answered normally, because over- charging is the failure mode #335 had to repair. The partial-write-safe sendAll the teardown suite kept to itself moves to tests/h2client.nim, where the new suite uses it too. Fixes #234
… streaming request bodies PR #242 gave a streaming (onBody) route the bodyReceived tally the buffered path gets from st.body.len, and reconciled it against the declared content-length at END_STREAM on DATA and at a trailer section. Auditing those two sites against the issue's failure scenario leaves two holes. First, END_STREAM on the request HEADERS themselves. No DATA frame and no trailer section ever arrives, so neither of the two checks runs, and the dispatch tail's check sits in the branch after the streaming one (reqStreaming wins the elif chain), so it never runs either. A `content-length: 10` with END_STREAM on the HEADERS therefore dispatched the handler, and h2SetOnBody flushed it last=true immediately for the already-half-closed stream: a clean, complete, empty body for a request that declared ten bytes. finishHeaders now reconciles before dispatching a streaming route, so the stream is reset before the handler exists. Second, the point of the check. Reconciling only at the end of the message is enough for a buffered route, which never sees a partial body, but a streaming route is given each chunk as it arrives and that is exactly what the issue's scenario forwards upstream. With `content-length: 10` and a 1000-byte DATA frame the sink received all 1000 bytes; a route relaying them to an h1 backend under the declared Content-Length had already desynchronized that connection by the time the terminating frame revealed the mismatch, and if the client simply never sent a terminating frame there was no reset at all. handleData now fails the stream as soon as bodyReceived passes contentLength, before the bytes reach the sink. RFC 9113 8.1.1 constrains the whole message and 8.1 lets us reset as soon as we know, so the earlier rejection is the conformant one as well as the safe one. teardownStream returns the frame's connDeferred bytes, so the connection window is not leaked on this path (#231). The failure is protocol-agnostic, so the h3 backend takes the same early check in cbBody (its end-of-stream reconciliation landed with #257, and cbStreamEnd already covers the empty-body case because nghttp3 reports end_stream for a HEADERS-with-fin request). The reset there is H3_MESSAGE_ERROR and cbStreamClose's creditRemainder returns the uncredited bytes, matching the existing oversize paths. Buffered routes are deliberately left alone: they are dispatched only once the body is complete, so there is no window in which a partial body escapes, and maxBodySize already bounds the excess. Tests: tests/test_http2_request_body.nim drives a frame-level client against a live server with a streaming route and a buffered control route. It covers DATA past the declared length with and without END_STREAM, a short body, a mismatch revealed by the trailer section, END_STREAM on the HEADERS, `content-length: 0` followed by DATA, the matching cases that must still succeed (including a body ended by a trailer section, whose trailers reach the sink), and that a rejected stream still terminates the sink so a handler suspended in await req.read() cannot leak (#232). Each rejection asserts both RST_STREAM(PROTOCOL_ERROR) and the absence of a response: the handler answers from its EOF callback, so a response on the wire is the proof that the sink was told "complete" while the stream was still live. All four reconciliation sites were verified live by disabling each in turn. Fixes #237
…in the whole trailer rule set PR #242 routed decoded trailers through fieldrules.validTrailerField, so an h2 or h3 trailer section already takes the same byte rules as the request head: a lowercase-token name, no CR/LF/NUL and no edge whitespace in the value, no pseudo-header, and none of the connection-specific fields. Auditing that set against RFC 9110 6.5.1 leaves one field through: content-length. validTrailerField builds on isForbiddenResponseField, which excludes content-length deliberately and says so, because on the response side h1 generates Content-Length itself and the h1 codec forbids it separately as a framing concern. A trailer section is not the response side. There content-length is the framing field 6.5.1 names first, and it is the one the h1 parser already drops (tests/test_trailers.nim covers that), so h2/h3 were the lenient pair: a handler that logged or relayed req.trailers could emit a second Content-Length for the same message, and a client could pick which of the two a relay believed. validTrailerField now rejects it by exact name (the name is known lowercase by then, so no allocation), which resets the stream on h2 and h3 alike. The wider 6.5.1 superset the h1 parser drops (host, cache-control, authorization, set-cookie and the rest) is deliberately left out: those are routing, auth and response-control fields, not framing, h1 only drops them rather than failing the message, and resetting a stream over one would be stricter than any widely deployed server. Rejecting is the right verb for h2/h3 only where a mismatch is a smuggling primitive. Tests: the #238 suite in tests/test_http2_request_body.nim drives a trailing HEADERS block from a frame-level client, over a buffered and a streaming route. It pins every rule, not just the new one: CR/LF, a bare CR, a bare LF, NUL, leading SP and trailing HTAB values; uppercase, non-token (SP, separator, control byte) and empty names; :status/:method; connection, proxy-connection, keep-alive, transfer-encoding, upgrade and te (forbidden in a trailer section even with the "trailers" value that is legal in the head); content-length; and that one bad field poisons the whole block in either order, so the handler never runs and no leading valid field surfaces through req.trailers. The positive cases assert the trailer really is delivered, and one test pins the maxHeaderSize bound on the trailer block (COMPRESSION_ERROR over the decoded field-list cap). Proved live by replacing the validTrailerField call with the pre-#242 pseudo-header-only check: 10 of the 16 trailer tests fail, including the issue's `a\r\nSet-Cookie: pwn=1`. Fixes #238
sseSanitize dropped CR and LF, the two characters that could inject a
second field or end the event early, and nothing else. NUL is the third
character the wire format cannot carry, and it is the one with a
behavioural consequence instead of a structural one: the WHATWG
EventSource rules say that if an `id` field value contains U+0000 the
client ignores the field rather than rejecting the byte, so the client's
last event ID buffer keeps whatever the previous event put there. The
server believed it had set a resume point, the client resumed from an
older one, and the next reconnect's Last-Event-ID replayed or skipped
events with no diagnostic on either side.
The same sanitizer feeds `event` and a comment's text, so NUL is now
stripped from all three.
The second half of the same gap is an id that survives the `id.len > 0`
guard and then sanitizes to nothing: `send("x", id = "\r\n")` emitted
`id: ` with an empty value. That is not "no id" on the client, it is the
field that resets Last-Event-ID to empty, so an id made only of
characters the wire format drops silently cleared the resume point. The
id field is now emitted only when the sanitized value is non-empty, and
an `event` name that sanitizes away is dropped the same way (an empty
event-type buffer dispatches as the default "message", so the field
carried nothing).
A caller cannot therefore ask for an explicit reset, because `id = ""`
is already the "no id field" default and nothing in the API sends a bare
`id:`. That is left as is rather than grown a flag for: a reset is a
rare, client-side concern, and an API where the empty string means two
opposite things would be worse than not expressing one of them. The
docstring, the README SSE section and the changelog all say so.
Tests: tests/test_sse_streaming.nim gains a /ids route checked against
the exact wire bytes: a normal id still emits `id: abc`, "x\0y" emits
`id: xy`, "\r\n" and "\0" emit no id line, a NUL in an event name is
stripped, a comment is sanitized too, and the whole dechunked stream is
asserted to contain no NUL byte. Verified the case fails against the
unfixed sources (NUL went out verbatim, `event: ` and `id: ` were
emitted empty).
Fixes #265
handleAccept could not grow the connection table while any slot was pinned. The table was one flat seq[Connection] per loop thread, so growing it reallocated the payload and moved every Connection, and a running `blocking:` worker holds `addr core.conns[fd]` for the whole of its handler: growing under a pin would have left that pointer aimed at freed memory. The accept path therefore scanned for a pinned slot and, on finding one, accepted the connection and closed it again. #349 gave that drop a counter and a rate-limited log line, which made it visible but left it a drop: the client still saw an empty connect error, indistinguishable from a network fault or the loop stalls d2ee202 fixed, and it is the client that pays for an implementation detail of the server's memory layout. The table is now segmented. A ConnTable holds blocks of 1024 Connections (712 B each, so ~712 KiB a block); a block is sized once at creation and is never resized, moved or freed for the loop's lifetime, and only the outer block list grows. A Nim seq is a (length, payload pointer) header whose elements live in a separately allocated payload, so growing a seq[seq[Connection]] reallocates just the outer buffer: the inner headers are byte-copied to a new address and every payload pointer they carry survives. A `ptr Connection` into a block therefore stays valid across any number of later growths, which is exactly what the `blocking:` pool, the file-chunk workers and the await paths need. The reasoning is spelled out on the type, and the structural test asserts it directly rather than leaving it to the reader. So handleAccept grows unconditionally, and the pinned scan, the refusal, pinnedGrowDrops and pinnedGrowLogSec are gone. No new ceiling replaces them: the index is a file descriptor, so the fd rlimit already bounds the table, the accept path still backs off on EMFILE/ENFILE, and maxConnections is still checked first and is unchanged in meaning. The initial allocation is one block, i.e. the 1024 slots the flat seq preallocated, so startup cost and the no-growth common case are as before. Growth now adds a whole block rather than `setLen(fd + 64)`, which is coarser but allocates once per 1024 fds instead of once per 64. The slot table is reached only through initConnTable / len / at / grow / slots now, so no caller can take a pointer into a buffer that is about to be resized, and `=copy` on a ConnTable is a compile-time error: copying one would duplicate the payloads and leave every pointer a worker holds aimed at the original. The debug audit in checkBodyPause and the drain, timeout and shutdown scans all moved to the `slots` iterator; the fd-indexed semantics are untouched everywhere else. Tests: tests/test_conn_table.nim, built with -d:vortexConnBlock=8 from its sibling tests/test_conn_table.nims so the growth path is reachable with a couple of dozen connections instead of an fd rlimit above 1024 (the define is internal; nothing but this suite shrinks it, and Nim applies a file-specific config script on top of tests/config.nims, so the suite still gets orc/threads/ssl and the NIM_MM, NIM_SANITIZE and NIM_COMPRESS knobs). The structural half asserts the invariant: a slot's address and its GC'd field survive growth to 104 and then 1000 slots, the index split addresses every slot distinctly, and `slots` yields each one in fd order. The live half pins a connection on a worker that sleeps 5 s, opens 24 more while it is held, and requires all 24 to answer 200 inside that window, then requires the pinned request to complete on its own connection. Verified the live case fails with the refusal reinstated (3 of 24 served) and passes clean under NIM_SANITIZE=1 (ASan + UBSan), which is what would catch a dangling slot pointer. .gitignore gains `!tests/t*.nims` beside the existing `!tests/t*.nim`: `tests/t*` exists to keep compiled suite binaries out, and it was swallowing the per-suite config script, which is source. Fixes #343
PR #242 added the cap but cited its issues as a range, so #235 stayed open and nothing in tests/ exercised it. This adds tests/test_http2_backpressure.nim, a frame-level client that tracks the receive credit the server grants and never overruns it, so a reset in these tests is always the memory cap talking and never a FLOW_CONTROL_ERROR. An audit of every claim in the issue against the current code found no residual behaviour gap. A buffered body's flow-control bytes are still credited eagerly on both windows, which is required rather than incidental: maxBodySize may exceed h2ConnWindow, so deferring the connection credit to dispatch (the issue's first suggestion) would deadlock every upload larger than the connection window. The aggregate instead bounds un-dispatched buffered bytes at max(connRecvWindow, maxBody), so it can never refuse an upload the configuration allows, and the stream that crosses it is reset with REFUSED_STREAM, which RFC 9113 8.7 defines as retryable: the body was never dispatched, so that guarantee holds. ENHANCE_YOUR_CALM would be a connection-level verdict on what is one stream's fault. The reservation is released on dispatch, on the trailers path, and in teardownStream, which every abnormal end funnels through (peer RST_STREAM, stream error, an early response, connection close). Three cases. 32 POST streams trickled round-robin past the cap, each refused stream immediately re-opened so the connection keeps 32 bodies in flight for the whole run, assert the pinned total never exceeds the cap plus one frame. A cancelled stream's reservation comes back, so a full-size upload that would not otherwise fit still succeeds. And a single upload of exactly maxBodySize completes across a connection window half that size, which only works while the credit stays eager. Both guards were verified live by removing them: with the aggregate check gone one connection pinned 3 MiB, six times the cap, and with teardownStream's release gone the upload after the cancel was answered with REFUSED_STREAM. h2CheckCounters, the debug-only audit the loop already runs on every input event, now re-derives the aggregate from a full stream scan as well, so a teardown path added later that forgets to release a reservation fails the test suite instead of silently and permanently shrinking what the connection will accept. Fixes #235
PR #242 closed the main hole in #236: "a stream owes response bytes it cannot send because a peer send window is shut" now arms the body deadline, which is the only deadline that fits. The client owes nothing, so no read-side timeout applies, and the owed bytes sit in pendingBody rather than the write buffer, so writeTimeout does not apply either (it arms only while the socket itself is unwritable). An audit of the rest of the issue's checklist found the predicate complete: h2BlockedOnPeerWindow covers a stream blocked on its own window and a connection whose aggregate window is shut with a backlog behind it, which between them cover a buffered response, a streamed res.write producer and an h2 WebSocket stream (a ws stream never sees END_STREAM, so h2AwaitingClient arms it anyway), sweepTimeouts acts on the expiry, and the resulting close goes out as GOAWAY(NO_ERROR) so the peer can tell a server-side timeout from a network fault (#342). What was still missing is where the classification runs. h2Deadline has one call site, the deadline tail of h2Input, so it only ever runs on an inbound event. A streamed sendFile parks its bytes from somewhere else entirely: a chunk read holds a file-chunk pin, which deliberately does not pause input, and releasePin re-processes input only when bytes are already buffered, which a silent client never has, while resumeAfterRespond runs only for the final chunk. So the first chunk of a download to a client that advertised a small initial window, absorbed it and then stopped sending WINDOW_UPDATEs landed in pendingBody on a connection that had been left with deadline 0 by the request's own input pass, and nothing ever looked again: the fd, the connection slot and the parked chunks were pinned for the life of the process. A static-file route is the most likely target for exactly that read pattern. sweepTimeouts now arms the deadline itself when it finds an h2 connection with nothing timed and bytes blocked on a peer window. It is the same predicate h2Deadline uses, so it can arm nothing an inbound frame would not have armed, it just no longer needs one; the cost is one O(1) counter read per connection per second. The gate is inputPausePins rather than totalPins, matching h2Input's own gate: a file-chunk worker never touches the h2 stream table, so reading its counters here cannot race one, and gating on totalPins instead would have skipped precisely the sendFile case this fixes. Deliberately unchanged: the deadline stays idle-based, so a peer that returns a token of credit every few seconds keeps the connection alive at a trickle. That is the same contract bodyTimeout and writeTimeout already document (both re-arm on progress, because a total-duration cap kills slow links), and the memory is bounded either way; maxConnections bounds the slots. Tests: tests/test_http2_backpressure.nim gains three cases. Three GETs for large buffered responses with a 16 KiB initial window, read down to the window and then silent, must draw GOAWAY(NO_ERROR) and a FIN inside bodyTimeout; the same against a streamed sendFile; and a client that keeps granting credit every 400 ms must finish a download that takes several times bodyTimeout with no GOAWAY, so the arm cannot false-positive on a slow but honest reader. The sendFile case fails on the unfixed sources (no GOAWAY, no close), and with h2BlockedOnPeerWindow stubbed to false both stall cases fail, which pins the coverage to this guard and not to another timeout. Fixes #236
…hten the docs Review follow-ups to the #343 and #265 commits. The segmented table made growth safe for a worker's ptr Connection, but it also removed a backstop nobody had written down: the old flat table could not move while any slot was pinned, so an off-thread read of it was merely stale. Now the outer block list reallocates whenever the loop grows the table, so an off-thread len or index would race a free. Every off-thread entry point already checks the thread or reads a snapshot; conn() is the one chokepoint they all reach, so it asserts the rule in debug builds and the ConnTable docstring says why the rule is stricter than "never copy". The -d:vortexConnBlock knob gets a compile-time floor of 1 instead of silently degenerating to one slot per block. The SSE wording claimed the wire format cannot carry NUL at all; data is not stripped and a client appends a NUL in data to the payload. The docstrings and README now scope the stripping to id, event and comments, and say that an id therefore does not round-trip byte for byte. The #236 changelog entry no longer promises GOAWAY unconditionally: a deadline that lands while a file-chunk read is in flight defers the close, and the deferred close is a bare FIN. The maxConnections row states the connection table's worst-case footprint, which the fd rlimit bounds rather than the cap. Refs #343, #265, #236
… TSan Review follow-ups to the test side of the branch. test_conn_table waited a fixed 300 ms for the blocking: handler to reach its worker before growing the table, so on a slow enough host the growth could land before the pin and the case would pass against the old refusing code too. The worker now bumps an atomic as soon as it holds the pin and the test polls for it, so the overlap is asserted rather than assumed. The elapsed budget keeps the whole pin window and says why, so nobody tightens an overlap proof into a performance bound. The suite joins the ThreadSanitizer job: it is the only one that grows the table while another thread holds a slot, which is the cross-thread half of the #343 claim, and it runs in seconds. test_http2_teardown lost its only posix user when sendAll moved to the shared client, so the import goes. The buffered-body cap is documented as never below the 64 KiB protocol default receive window, which is what the code computes (connRecvWindow is clamped up at construction), and the SSE suite's row records the #265 coverage like every other issue-numbered case in the table. Refs #343, #235, #265
The helper passed flags 0, which is only SIGPIPE-safe on Linux because the server under test ignores SIGPIPE process-wide before the first send. That dependency was undocumented and would bite a suite that sends before starting a server. std/net passes MSG_NOSIGNAL on Linux for this reason; so does sendAll now, with the macOS side unchanged (SO_NOSIGPIPE).
…A frame An adversarial review of the #234 round found the credit gate wrong in both directions, and the two defects are the same mistake seen from either side: it decided what a WINDOW_UPDATE costs from the bytes we had sent, when what the peer's reply rate actually follows is the frames we chose to send them in. The issue's headline vector was still free. The new gate charges a stream-level WINDOW_UPDATE only when the stream is not sendable, and an increment of 1 always makes a stream with a backlog sendable, so after SETTINGS_INITIAL_WINDOW_SIZE=0 a flood of WINDOW_UPDATE(sid, 1) against a parked response still bought one 1-byte DATA frame and one full scheduler pass per 13-byte frame, forever. Each forced frame even banked credit on the way out. An update that dribbles is now charged straight to the control-frame budget: an increment below windowCreditBytes that leaves the window it credits below windowCreditBytes while bytes are waiting on that window, at the connection level as well as the stream level. A correct client never asks for more data in pieces that small while holding the window under 256 bytes; it has either a real window to offer or none at all. That is the CVE-2019-9511 shape. The charge goes to the budget and not to the credit pool precisely because the 1-byte frames the dribble forces would otherwise earn the credit that pays for it, and it runs before the scheduler pass, so the frame that trips the budget emits nothing after the GOAWAY. In the other direction a correct client was being torn down. Credit was earned per 256 bytes of DATA but spent per WINDOW_UPDATE received, and the server picks the frame size: an SSE handler writing 1500 16-byte events to a client that returns one stream-level and one connection-level update per frame it consumed ran the pool dry and tripped GOAWAY(ENHANCE_YOUR_CALM) after about 1070 events at the default budget. Those 256-byte chunks acked on both levels survived only by the secondary decay. Every DATA frame now earns a floor of two credits, one per level, which is the finest acknowledgement granularity a correct client can have, on top of the existing one per 256 bytes and still under the same cap. noteDataProgress takes the frame count for it, since h2WriteDirect emits a whole run of frames in one call and reported only their total size. Two smaller review notes ride along. The comment at the stream-level h2Enqueue claimed the stream was queued regardless, which h2Enqueue contradicts: it gates on h2Sendable, and what makes a stream blocked only on the connection window findable by a later connection WINDOW_UPDATE is that h2Sendable ignores that window. The code is right, the comment now says why. And DATA on a closed stream charged the budget and then reset the stream unconditionally, so the frame that tripped ENHANCE_YOUR_CALM emitted a RST_STREAM after the GOAWAY; it now takes the csClosing guard the other charged replies in that commit took, which lets the suite's RST_STREAM bound drop from budget + 1 to budget. tests/test_http2_budget.nim grows the four cases: the stream-level and connection-level dribble floods (bounded DATA frames and ENHANCE_YOUR_CALM), the small-frame producer acked per frame on both levels, and a 200 KiB download acked in 4096-byte increments at the low budget (the Go net/http2 inflowMinRefresh shape, where every update unblocks a send and none may be charged). Each was confirmed to fail with its fix reverted. Refs #234
…ply too The maxControlFrames row, the threat-model floods row and the noteControlFrame docstring all read as though every RST_STREAM sent in answer to a flood cost a second unit. The inbound frame is charged once, before the reply; only the CONTINUATION refusal charges its own reply. A maintainer sizing the budget from those rows would have double-counted. The budget suite's header also counted the audited vectors as eight while listing ten.
…s content-length The h3 backend queues a streaming route for dispatch from cbEndHeaders, by pushing the stream onto the ready list, and the event loop runs the handler only after the whole engine pump: ngReceive parses everything the read batch contains, then ngTakeReady is drained. Both content-length reconciliation sites reset the stream but deliberately leave it in h3c.streams, so cbStreamClose can still return its flow-control credit, and neither removes the entry the ready list already carries. For a request whose head, body and FIN are parsed in one batch the reset therefore happens while the handler does not exist yet, and the handler runs afterwards anyway, with every side effect (an upstream request, a database write) the reset was meant to prevent. h2 has no such window: finishHeaders reconciles and returns before it adds the stream to its ready list. The ready-list consumer now resolves the connection first and skips the stream when it is gone from the table or flagged rejected, mirroring the h3StreamAlive check the post-accept WebSocket pump a few lines below already makes. The flag is set by the two reset-but-keep paths (cbBody's early over-length check and cbStreamEnd's end-of-message reconciliation), and the alive check also covers the third resetter that drops the stream outright, the oversize pre-accept WebSocket handshake in cbBody, which had the same queued entry behind it. Skipping costs the stream nothing it needs: cbStreamClose still releases the buffered-body reservation, still fires onBodyCb(last = true) so a suspended handler cannot leak, and creditRemainder still returns the uncredited bytes to the connection window. The ready list is taken and cleared wholesale each pass, so a skipped entry is dropped rather than left to leak. A rejected stream also stops collecting body bytes. The append used to run before the over-length check and the early return skipped deliverBody, so a stream whose handler had not run yet (the case above, where there is no sink to drain st.body) kept buffering every further DATA frame until cbStreamClose. The reject path clears st.body, and further DATA on a rejected stream is counted as uncredited, so the credit is still returned at close, and discarded. How reachable the window is depends on the client. curl sends the request head in its own packet, so on loopback the handler is dispatched from the batch that carries the HEADERS, before the body it will disagree with has arrived, and nghttp3 fails an over-length DATA frame itself before cbBody sees it. Out-of- order delivery is what closes the gap: a DATA frame that arrives before the HEADERS is buffered by ngtcp2 and flushed into nghttp3 together with them, which puts the whole request, reconciliation included, in the single read batch that precedes the dispatch. Verified with a UDP relay that holds one 1-RTT packet back: cbHeaders and cbBody then run in one batch with no dispatch in between. Tests: tests/test_http3.nim pins the wire-visible property for the three mismatched shapes (10 bytes under content-length 5, 10 under 64, and no body at all under 5): none of them is ever answered, and the server still serves the next request. They deliberately do not assert on the dispatch counter, because for a client that cannot reorder packets the dispatch is legitimate at that point and asserting otherwise would pin curl's packetization rather than the server's behaviour. The positive case (content-length 10 with exactly 10 bytes) asserts the guard costs the normal path neither its single dispatch nor its clean EOF. Refs #237
…very protocol PR #242 and the commit before this one closed the inbound half of the trailer rules: a request trailer section takes the request head's byte rules plus the forbidden set, content-length included. The outbound half had three implementations and they disagreed. h1's appendLastChunk filtered with connSpecificField, which drops content-length but not te. h2's emitTrailers routed the trailer fields through encodeExtraHeader, the response-header filter, which is called with trailer = false and so emitted BOTH content-length and te in the trailing HEADERS. h3's h3StreamFinish passed trailer = true, so it dropped te but still submitted content-length, and it checked the value bytes without checking that the name was a token at all. A handler that relays an upstream's trailers verbatim is the realistic way in: res.trailers["Content-Length"] = "999" after a streamed body put a second, later Content-Length on the wire for a message the client had already framed, which is the same smuggling primitive as the inbound one with the direction reversed, and an intermediary that believes the trailer disagrees with the one that believes the head. RFC 9110 6.5.1 forbids generating Content-Length in a trailer section for exactly that reason, and RFC 9113 8.2.2 / RFC 9114 4.2 forbid te on a response at all. fieldrules gains one predicate, forbiddenResponseTrailerField, with the rationale for why content-length belongs in it here and deliberately not in isForbiddenResponseField. All three writers now use it, so the set cannot drift again, and validTrailerField (the inbound rule) is expressed in terms of it too, since it had grown its own copy of the same two additions. h1 keeps every field it dropped before, since the new set is a superset of connSpecificField, and keeps its h1 field-name case. h2 trailers now also drop a pseudo-header or non-token name and a CR/LF/NUL or edge-whitespace value, matching what h3 had already done for values (#257): HPACK carries those bytes without complaint, so the splitting happens in a relay re-serializing the section to h1. Both h2 and h3 lowercase the name, as HPACK and QPACK require. Tests: tests/test_trailers.nim grows a streamed route whose trailer section carries Content-Length, TE, Transfer-Encoding and X-Checksum, and asserts that only x-checksum arrives, over h1 (read out of the raw chunked trailer section) and over h2 (the trailing HEADERS block decoded field by field, so the assertion is the exact field list). tests/test_http3.nim takes the same route over curl's h3. Proved live by reverting each writer in turn: the h2 case fails on the leaked content-length and te, the h1 case on te. The h3 case does not distinguish, because curl's own h3 stack drops a content-length trailer before it reaches the dump; what it pins there is that the stricter shared predicate still delivers the legitimate trailer. Refs #238
…dowing sendAll copies test_http2_request_body spent eight of its twelve seconds sleeping out a 300 ms quiet period per connection to discard the server's SETTINGS. It now reads until that frame arrives, which is the deterministic form the rest of the suite already uses, and the suite finishes in two seconds. It also closes its server before exit like every other h2 suite, so process teardown does not race a live loop thread. Both new h2 suites re-declared a private sendAll identical to the one the shared client exports, and the local symbol shadowed the import; the copies go, and with them a posix import the request-body suite no longer needs. The TESTING row stops claiming the sink-termination check runs for every rejected stream (one case pins it), and the threat-model row for request trailers says the h3 path shares the rule but is reasoned rather than driven, since no client here can send a malformed h3 trailer.
cryo2010
marked this pull request as ready for review
October 1, 2026 07:00
This was referenced Oct 2, 2026
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.
Closes out the five HTTP/2 review issues that PR #242 fixed but never closed (its body cited the range in prose, so GitHub left #234 through #238 open), with a re-audit of every claim against current main, the residual gaps that audit and an adversarial review found, and the regression coverage none of them had. Also fixes the SSE
idsanitization gap and replaces the count-and-log mitigation for #343 with the growth-stable connection table the issue asked for. One commit per issue, plus review follow-ups.Fixes #234
Fixes #235
Fixes #236
Fixes #237
Fixes #238
Fixes #265
Fixes #343
HTTP/2 control-frame budget (#234)
Audit of the eight vectors: PING ACK, received GOAWAY, unknown frame types, the self-dependent PRIORITY ordering and the RST-on-idle rule, closed-stream DATA, the per-request reset and per-entry SETTINGS charging were all fixed in #242 / #335. Three charges were still missing, all the same shape (an overhead frame reaching its reply or early return before
noteControlFrame):The adversarial review then showed two things about the credit model from #335. The increment-1 data dribble (SETTINGS_INITIAL_WINDOW_SIZE=0, then
WINDOW_UPDATE(sid, 1)per frame, each forcing a 1-byte DATA frame and a scheduler pass) was still free, because an increment of 1 always makes the stream sendable, and credit is earned per 256 bytes sent but spent per update, so a 16-byte-event SSE stream whose client acked every frame tripped ENHANCE_YOUR_CALM at the default budget after about 1070 events. The model now earns two credits per DATA frame emitted (one stream-level plus one connection-level update per frame is the finest acknowledgement a correct client can send) in addition to one per 256 bytes, and a dribble (increment under 256 bytes that leaves the window under 256 bytes while bytes wait on it) is charged straight to the budget on both levels, never to the credit pool, because the 1-byte frames it forces would otherwise bank the credit that pays for it. A frame that trips the budget no longer draws a RST_STREAM after its own GOAWAY.tests/test_http2_budget.nim(17 tests) floods one vector per test at a budget of 20 and asserts GOAWAY(ENHANCE_YOUR_CALM) with a bounded RST_STREAM count, covers the RFC 9113 5.1 idle-stream rule, per-entry SETTINGS charging, the one-GET-per-burst interleave, both dribbles, and the converses: a few control frames per request, a 16-byte SSE stream acked on both levels per frame, and a 200 KiB download acked in 4096-byte increments all complete without a GOAWAY.Buffered-body memory cap (#235)
No production change needed: #242's independent aggregate of un-dispatched buffered bytes,
max(h2ConnWindow, maxBodySize)(never below the 64 KiB default receive window), is released on every teardown path and refuses the crossing stream with REFUSED_STREAM, which is retryable because the handler never ran. Credit stays eager on purpose: deferring it would deadlock any upload larger than the connection window. The debug-only counter audit now re-derives the aggregate from a full stream scan, so a later teardown path that forgets a release fails the suite instead of silently shrinking what the connection accepts.tests/test_http2_backpressure.nimtrickles 32 POST streams past a 512 KiB cap (3 MiB pinned with the check removed), proves a cancelled stream returns its reservation, and uploads 512 KiB across a 256 KiB connection window.Zero-window response stall (#236)
#242 armed the body deadline from the input path when a stream owes response bytes blocked on the peer window, but
h2Deadlinehad exactly one call site, so a stall that began outside an inbound event was never armed. A streamedsendFileparks its first chunk from the outbox under a file-chunk pin, which does not re-process input, and a silent client never has buffered bytes to trigger it: a 2 MiB download to a client that absorbed 16 KiB of window and went quiet produced no GOAWAY and no close at all (reproduced). The per-second timeout sweep now arms the same deadline from the same predicate. Covered for a buffered response and for a streamedsendFile, plus the converse that a client returning credit is never cut off.Streaming content-length reconciliation (#237)
#242 reconciled at END_STREAM and in the trailers path, but a streaming route relays as it goes, so an over-length body had already been handed to the sink chunk by chunk (and a client that never sent the terminating frame never saw a reset at all). The stream is now reset with PROTOCOL_ERROR as soon as
bodyReceivedexceeds the declared length, before the chunk reaches the sink; a declared body with END_STREAM on the request HEADERS is reset before dispatch instead of dispatching a handler that is flushed a clean empty body; and the HTTP/3 backend applies the same early check. A streaming request HTTP/3 has already rejected mid-batch is no longer dispatched from the ready queue either (reachable under packet reordering, not with curl on loopback, which the h3 tests document). Flow-control credit is balanced on every new reset path (48 rejected over-length streams pushing 6x the connection window, then a full-window upload, completes).Trailer field validation (#238)
Inbound trailer names and values already went through
validFieldName/validFieldValueand the connection-specific set since #242;content-lengthin a request trailer section was still stored and is now rejected (h1 already dropped it). The adversarial review found the outbound half was three different implementations: h2 emittedcontent-lengthandtein a trailing HEADERS, h3 emittedcontent-length, h1 emittedte. One shared predicate infieldrules.nimnow filters response trailers on all three protocols, and h2/h3 validate and lowercase trailer names as HPACK/QPACK require.tests/test_http2_request_body.nim(28 tests) covers both issues at the frame level, including the issue'sa\r\nSet-Cookie: pwn=1payload;tests/test_trailers.nimcovers the response side on h1 and h2 (h3 via curl).SSE id sanitization (#265)
sseSanitizestrips NUL alongside CR and LF forid,eventand comment text (notdata, where a client appends it to the payload), and anidoreventthat sanitizes to empty is not emitted, since an emptyid:field is aLast-Event-IDreset to a client and"\r\n"or"\0"asked for no such thing. An explicit reset stays inexpressible becauseid = ""is already the no-field default; documented in the docstring and README, along with the fact that anidtherefore does not round-trip byte for byte. Asserted against exact wire bytes intests/test_sse_streaming.nim.Growth-stable connection table (#343)
LoopCore.connswas a flatseq[Connection]; growing it for a high fd moved every element, sohandleAcceptrefused the connection whenever ablocking:worker heldaddr conns[fd], and #349 made that refusal counted and logged. It is now aConnTableof fixed 1024-slot blocks that are never resized or moved: a Nim seq is a header plus a separately allocated payload, growing the outerseq[seq[Connection]]byte-copies the inner headers and leaves every payload where it was, so a worker'sptr Connectionsurvives any number of later growths and the refusal, its counters and its log line are gone.=copyon the table is a compile error, andconn()asserts the loop-thread rule in debug builds, because the outer block list now reallocates under growth where the old flat table stood still while anything was pinned.tests/test_conn_table.nim(built with-d:vortexConnBlock=8from a per-suite.nims) pins the structural invariant and the live scenario: 24 connections served through several growth events while a worker holds a pin that an atomic proves is live, then the pinned response arrives on its own connection; the suite also runs under ASan and now in CI's ThreadSanitizer job.Follow-ups found during the audits (not changed here)
bodyTimeoutandwriteTimeoutare documented to be; a progress-based variant is a cross-protocol behaviour change worth its own issue. Memory is capped either way andmaxConnectionscaps slots.h2DeadlinereturnsdkNone;dkResponseis h1 only).failConn, a handler already queued for that stream still runs (memory-safe, response discarded). Closing it needs a shim-visible "connection failed" query, i.e. a C++ shim change.host,cache-control,authorization, ...); only the framing and connection fields are rejected. Full parity is a behaviour change worth its own issue.content-lengthheader is accepted and never reconciled on h1, h2 and h3 alike.pendingOut;writeTimeout(30 s since fix: protocol hardening round 2 (h2 GOAWAY on close, h3 WebSocket accept races, body-pause leak, writeTimeout default) #349) plus the now-complete budget boundwbuffor every vector in fix(h2): maxControlFrames budget bypasses (PING ACK, WINDOW_UPDATE, self-dep PRIORITY, closed-stream DATA, per-request reset, SETTINGS amplification) #234.Validation
NIM_COMPRESS=1 bash tests/run.sh(the CI test job's command, orc, gzip+brotli+zstd on) on the final branch: 92 suites, 747 checks, 0 failures, including the 17 budget, 6 backpressure, 28 request-body, 6 connection-table, 7 trailer and 25 HTTP/3 cases.test_thread_raceandtest_blocking_raceclean on the segmented table;test_conn_tablejoins the TSan job in this PR.VORTEX_PROTO=all VORTEX_SERVER=all VORTEX_SECONDS=10 VORTEX_REPORT_SECONDS=2 nimble stress(5 workloads x h1/h2/h3 x sync/async/async-await/chronos/chronos-await, 75 cells, canary plus chaos sidecar, 64 MiB streams): 75/75 passed, every checksum verified, no panic, assertion, crash or checksum-mismatch signature in any server log. The h2 SSE cells (374k events per cell with per-frame client acks) and the h2 streaming upload and download cells cover the credit-floor, early-reset and sweep-arm paths under load.