Repository navigation
fix: protocol hardening round 2 (h2 GOAWAY on close, h3 WebSocket accept races, body-pause leak, writeTimeout default) - #349
Merged
Conversation
A streaming HTTP/1 read that hits the read-ahead high-water sets c.bodyReadPaused and takes a slot in the loop's bodyPausedConns count, which caps the selector wait at 2 ms so the async consumer keeps draining. Only closeConn and resumeBodyImpl released that slot: startStreamingDispatch and resetRequest (via resetRequestState) cleared the flag on their own, so the count stayed high forever and pinned the loop thread's selector wait at the paused-body cadence for the rest of its life. The reset path is reachable: a worker's response is applied from the outbox, and resumeAfterRespond resets the request without looking at whether the socket read is still paused. A streaming route with a manualAck sink that answers from a `blocking:` worker without draining its read-ahead debt leaks a slot per request. Clear the flag only through clearBodyPause, which also gives the slot back, and audit the count against a full scan of the connection table once a tick in a debug build (in the spirit of h2CheckCounters), so a future clear that bypasses the helper trips an assertion in the test suite instead of quietly spinning the loop in production. The new test in the HTTP/1 streaming-request suite drives that worker path and then serves a plain request: before the fix the audit reports "bodyPausedConns drift: 1 vs 0" and takes the loop thread down with it. Closes #344
When an accepted fd lands beyond the connection table and any slot is pinned, handleAccept refuses the connection: growing the seq would move every Connection and dangle the `addr conns[fd]` a blocking: worker holds. That refusal was a bare posix.close, so the client saw an empty connect error (indistinguishable from a network fault or a stalled loop) and the server said nothing at all. Count the drop per loop thread and log it through the same stderr channel the other accept-time backoffs use, rate-limited to one line a second and carrying the running count so a burst stays visible without flooding. The connection cap above keeps its silent drop: that one is a configured policy, while this is an internal limit the operator has no other way to see. This is the minimal half of the fix: the connection is still dropped. A real fix needs a growth-stable table (segmented blocks, so `addr conns[i]` survives growth), which touches every `conns[...]` walk in the loop, request lookup and shutdown paths and is too large to carry here. No regression test: reaching the branch needs an accepted fd past the 1024-slot table (so over a thousand live sockets from the test process) coinciding with a running blocking: worker, which is neither cheap nor deterministic in the suite. Refs #343
An HTTP/2 connection dropped by the timeout sweep, by the receive-buffer cap, or by forceCloseAll at the end of the shutdown grace went straight to closeConn, so the peer only ever saw a bare TCP close. That is indistinguishable from a network fault (httpx reports RemoteProtocolError: Server disconnected), which leaves even an idempotent in-flight request unsafe to retry, because the client cannot know whether the server processed it. Route those three paths through closeWithGoaway, which emits the same GOAWAY the graceful drain already uses (the last stream id this server processed, so everything above it is known-unhandled) and then closes. NO_ERROR for an expired timeout and for the forced shutdown close, ENHANCE_YOUR_CALM for the receive-buffer cap, where it is the peer's own volume that forced the drop. h2Goaway grew an error-code parameter defaulting to NO_ERROR, so the drain call sites are unchanged. The write is best effort and never blocks: the frame rides whatever output is pending if the socket takes it, and the connection closes either way, so a peer that has stopped reading cannot hold the close. A connection pinned by a running blocking: worker is closed without a GOAWAY, as in markDrain: touching its h2 ref here would race the worker under ORC's non-atomic refcounts. tests/test_http2_goaway.nim covers the idle and body timeouts with a short-timeout server, asserting a GOAWAY(NO_ERROR) naming the last processed stream arrives as the final frame before EOF. The cap path has no test: it needs a megabyte of frames that cannot be compacted while a worker holds the connection, which the suite cannot stage reliably. Closes #342
An HTTP/1 streaming upload consumed asynchronously (await req.read, i.e. an onBody sink registered with manualAck) was read off the socket as fast as the kernel could deliver it and copied into the adapter's unbounded reader queue, so a single slow consumer pinned the whole body in RAM and many concurrent uploads OOM'd the server (#271). The bound that fixes it shipped with #325 (7fc9122): feedBody counts delivered-but-unacked bytes per connection, handleRead stops pulling once that debt reaches the high-water (the tail waits in the kernel as real TCP backpressure), and req.ackBody repays the debt and lets the recv loop resume below the low-water. It shipped with no regression test, so nothing pinned the behavior. Add one: a streaming route whose consumer pulls a chunk, parks, then drains, driven by a non-blocking client pushing 32 MiB at it. The test records how much the server had accepted when the socket went unwritable and asserts that it is bounded (about 1.5 MiB measured here, cap 8 MiB to cover both kernel socket buffers) and that the upload still completes byte-exactly once the consumer resumes. Both framings are covered, Content-Length and chunked. Raising the high-water out of the way makes both cases accept all 32 MiB with no stall at all, so the test genuinely pins the bound rather than the timing. Also document the HTTP/1 read-ahead bound in HARDENING.md, which described the HTTP/2 and HTTP/3 upload windows but had nothing for HTTP/1. Closes #271
The three conditions in #239 (a trailer section without END_STREAM, a HEADERS on a half-closed(remote) stream still in the table, and a HEADERS/DATA racing the server's own early final response) are already handled as RFC 9113 5.1 / 8.1 require: the first two are stream errors, and the third is answered from the bounded earlyClosed set with RST_STREAM(STREAM_CLOSED). That landed in 24d7fd7 (PR #242) without any regression coverage, so nothing pinned the *scope* of those errors, which is the whole point of the issue: escalating them to a connection error aborts every other in-flight request. Add tests/test_http2_stream_errors.nim, a frame-level suite that drives each condition on a connection that also carries an unrelated concurrent request: * trailers without END_STREAM: RST_STREAM(PROTOCOL_ERROR) on that stream, no GOAWAY, the concurrent request still answered. * HEADERS after END_STREAM on a stream still in the table (kept there by a zero SETTINGS_INITIAL_WINDOW_SIZE parking the response body): RST_STREAM(STREAM_CLOSED), no GOAWAY. * DATA plus a trailer section arriving after a streaming route answered 403 without reading the body (exactly the deletion a correct client's frames race): RST_STREAM(STREAM_CLOSED), no GOAWAY. Reverting each of the three code sites to connError makes the matching test fail with a GOAWAY and a lost concurrent response, so the suite pins the fix rather than just the current behaviour. tests/h2client.nim gains addSettingFrame (a one-entry SETTINGS frame), hasResponse and headerPayload. Closes #239
All ten follow-ups are already implemented (24d7fd7, PR #242) but that commit landed without tests, so nothing pinned them. Each sub-item below is now driven at the frame level (or, for the shared classifier, at the unit level) and was confirmed to fail when its fix is reverted: 1. even-id idle streams: DATA, WINDOW_UPDATE and RST_STREAM on stream 2 must be a connection PROTOCOL_ERROR. Each case opens stream 3 first, so the high-water mark is past 2 and only the parity check can catch it. 2. omBlockingDone flush: the flush is in place in applyBlockingDone. It cannot be isolated by a test any more, because #341's batch-tail releaseBatchFlushes also pushes the connection when the worker's response and its done message share an outbox batch; the existing "pipelined: blocking then fast" case in test_blocking.nim exercises the path. 3. connection-specific response fields: a handler that sets connection, keep-alive, transfer-encoding, upgrade and proxy-connection gets none of them on the wire, while an ordinary field still rides. 4. unknown :method: PURGE and a value carrying a space are rejected instead of running the GET handler (h2 and the h3 classifier). 5. CONNECT rules: a CONNECT carrying :scheme and :path (forbidden by RFC 9113 8.5) is malformed instead of being dispatched as a normal request; the Extended CONNECT cases in test_http3_connect.nim stay green. 6. content-length grammar: "+5" with a 5-byte body and "1_0" with a 10-byte body are rejected. The bodies matter: a loose parse reads both as the body length, so the reconciliation check alone would let them through. 7. field-value whitespace: a leading SP or trailing HTAB is malformed (h2 and the h3 classifier). 8. HPACK: after the peer sets SETTINGS_HEADER_TABLE_SIZE=0, the next response header block starts with a 0x20 dynamic-table-size update and still decodes. 9. WS over h2: with the stream send window exhausted and a close queued behind the backlog, no RST_STREAM(CANCEL) is sent; a partial then a full WINDOW_UPDATE drain the queued echo, the close frame and END_STREAM. Restoring the old cancel branch fails the test with CANCEL and no frames. 10. GOAWAY shorter than 8 octets is answered with FRAME_SIZE_ERROR. Note for 4 and 5: an unknown-but-valid-token method and a spec-legal plain CONNECT are answered with a stream PROTOCOL_ERROR, not the 501 the issue suggested. That is the shape the codebase settled on (#245 later aligned the h1 parser's method matching with h2/h3's isKnownMethod rather than the other way round), and the tests lock it in. Closes #240
Two HTTP/1 correctness follow-ups from #249. maxHeaderCount rejects with 431 (Request Header Fields Too Large), which the parser has always returned, but HARDENING.md and the README both documented it as 400. Correct both, and note that the count is an HTTP/1 limit: HTTP/2 and HTTP/3 bound the decoded header list by size (maxHeaderSize) rather than by field count, so there is no equivalent status there to align. writeTimeout shipped at 0 (disabled), so out of the box nothing reaped a slow-reading or zero-window client that stops draining a large buffered or streamed response: the write-stall machinery (armWrite arming writeDeadline on every EAGAIN, disarmWrite clearing it on a full flush, sweepTimeouts reaping it independently of c.deadline) was correct but inert. Default it to 30 s, the same as bodyTimeout, and document it in the setting's doc comment, HARDENING.md and THREAT_MODEL.md. responseTimeout stays at 0: it bounds handler time, which is an application decision. The deadline is idle, not total (every partial write re-arms it), so a response that keeps making progress is never cut off; the streaming and SSE suites are unaffected. Cover both halves in test_security_dos: a client that stops reading a 4 MiB response is reaped and gets a truncated body, while a reader that takes a quarter megabyte every 250 ms receives all of it although the server holds pending output for several times writeTimeout. Arming the deadline once instead of on every partial write fails the second test, and writeTimeout = 0 fails the first, so both pin real behavior. Closes #249
An HTTP/3 Extended CONNECT stream (RFC 9220) dispatches on headers, so the handler runs (and calls h3WsAccept) after the client may already have sent DATA in the same packet burst. Those bytes land in cbBody while st.ws is still nil and accumulate into st.body via the buffered-request-body path. h3WsAccept attached the WsConn but never moved st.body into w.inBuf, so the client's first WebSocket message(s) were silently lost whenever they coalesced with the handshake (the h2 path has done this since h2WsAccept). Seed w.inBuf from st.body in h3WsAccept (clearing st.body) as h2WsAccept does, and pump the seeded frames in h3Drive right after the handler ran, so they are dispatched only once onMessage is installed. The pump mirrors the h2 post-accept pump in dispatchH2 and resolves the WsConn ref before feeding, since wsFeed can tear the stream down. The local suite cannot drive WebSockets over QUIC (no h3 ws client exists for curl), so the regression case goes in the Docker conformance harness (conformance/h3websocket): the aioquic client now opens a stream with a text frame pipelined into the handshake burst and requires the echo back. Closes #259
WebSocket.subprotocol() resolved the WsConn ref through wsConnOf from any
thread, unlike every sibling accessor (onMessage=, onClose=, onDrain=,
bufferedAmount, isAlive), which all go through withWsConn and bail off the
loop thread. Those guards exist because resolving and copying the WsConn ref
off-loop races the loop thread's non-atomic ORC refcount, so a worker that
branched on ws.subprotocol (a natural value to read) could bump or drop the
refcount while the loop dispatched frames or tore the stream down: a
use-after-free or a leaked WsConn.
Route it through withWsConn like the others, so off-loop it reports "" and
never touches the ref. The negotiated value is not captured into the
WebSocket handle instead: the handle is four plain words deliberately
copyable across threads, and a string field is exactly the cross-thread copy
the guard prevents. The thread rule is now documented on the accessor and on
acceptWebSocket, where the value is first mentioned.
The regression test spawns a thread from inside onMessage, reads
ws.subprotocol there and reports only its length back (so the probe itself
copies no string across threads): 4 ("chat") before the fix, 0 after.
Closes #260
On HTTP/3 a client can FIN an Extended CONNECT stream before the handler accepts it (the stream dispatches on headers). cbStreamEnd only delivers wsPeerClosed when st.ws is set, and h3WsAccept never looked at the FIN it recorded, so nothing replayed it after acceptance: the application's onClose never fired and the WsConn lingered until the per-stream idle sweep timed it out. The h2 path has handled this via WsConn.preAcceptFin since h2WsAccept. Carry the recorded FIN into w.preAcceptFin in h3WsAccept, and replay it as wsPeerClosed in the h3 post-accept pump (re-checking that the stream survived the frame pump first), exactly as the h2 dispatch does. Regression case in the Docker conformance harness (conformance/h3websocket), since the local suite has no HTTP/3 WebSocket client: the aioquic client now half-closes the stream in the same burst as the handshake and requires the server's close frame, which only the replayed wsPeerClosed (the same call that delivers onClose) can produce. Closes #261
WsConn sends and closes on the loop thread end with flushHook, whose implementation resolved a ptr Connection from the fd. An h3 handle has fd < 0 (a slot encoding, never a socket), so conn() returned nil and the hook did nothing: the frame sat in the QUIC stream's send buffer until some unrelated event happened to drive the connection. Sends from inside an inbound onMessage were saved by the h3Drive that follows input processing, and off-loop sends ride the outbox (which drives h3 itself), but a loop-thread send from any other source (a chronos or asyncdispatch continuation resumed by the pump, a timeout sweep) could stall on the wire for a full idle wait. flushImpl now flags the loop for fd < 0 instead of resolving a connection, and the loop drives h3 once more after the pump and sweeps if the flag is set. A flag, not a direct h3Drive: the hook can run inside a nghttp3 callback (inbound frame dispatching onMessage, which sends) and re-entering the QUIC stack from there is not safe. h3Drive clears the flag just before ngPump, so the common in-dispatch send costs no extra pass. The local suite has no HTTP/3 WebSocket client, so the case lives in the Docker conformance harness (conformance/h3websocket): the echo server answers "later" from a 200 ms timer continuation and the aioquic client requires that reply within 700 ms, well under the loop's idle wait. Closes #262
Before an Extended CONNECT stream is accepted, cbBody treated inbound WebSocket framing as a buffered request body: it accumulated into st.body under the maxBodySize guard and added the bytes to the per-connection bufferedBytes aggregate. So pre-accept framing was bounded by maxBodySize (vs maxMessage + 1024 once wsFeed takes over), and a client flooding unaccepted CONNECT streams could pin the aggregate that exists to cap request bodies, tripping its cap and resetting an unrelated ws-connect stream with H3_MESSAGE_ERROR instead of the WebSocket 1009. Give a known ws-connect stream (the classifier decides that at headers time) its own branch in cbBody: bound the held bytes by maxWsMessage + 1024, the same ceiling wsFeed applies after acceptance, and keep them out of bufferedBytes. Credit them to QUIC flow control as tunnel bytes, exactly as the accepted path does, so the handover into the WsConn's inBuf does not double count. On overflow before acceptance there is no WsConn to carry a 1009, so the stream is reset with H3_REQUEST_REJECTED (the h3 twin of the h2 REFUSED_STREAM on the same bound) rather than the request-body error, after returning the connection-window credit for the discarded bytes. The overflow path needs more than maxWsMessage of framing to arrive before the handler runs, which the Docker harness's well-behaved client cannot produce, so there is no new conformance case; the accounting and handover this rewrites are exercised by the coalesced-handshake case added with #259, which passes (conformance/h3websocket, aioquic). Closes #263
wsStreamClosed freed the zlib contexts under `if w.pmd`, but `pmd` records that the extension was negotiated and is never cleared, so the guard was true on every later call. Nothing in the WsConn stopped a second teardown from handing the same z_stream to deflateEnd/inflateEnd again: only external discipline did, each caller nil-ing its reference (h2WsFinalize and h2WsTeardownAll clear st.ws, wsClosed clears c.ws, wsFlushH3ng clears conn.streams[sid].ws) after a call that has several reachable paths per WsConn (an h2/h3 stream reset, then the connection dying). The second deflateEnd was in the end absorbed by the `inited` flag inside Deflator.close/Inflator.close, one layer below where the invariant was written down, so the codec was relying on a defence it did not state. Move the free into wsClosePmd and gate it on a new `pmdClosed` flag, so the idempotency lives with the state it protects and a second wsStreamClosed is a no-op in both halves (notifyClose was already idempotent via closeNotified). wsStreamClosed is the single teardown path that reaches the contexts: h1 close, the h2 stream reset and finalize, the h2 connection teardown, the h3 stream and connection teardown, and the h3 flush finalize all go through it, and the idle sweep only closes frames. The one other close of a zlib context is wsSetup's negotiation-failure cleanup, which tears down a half-initialized pair before `pmd` is ever set, so it stays where it is. A true double-free is not assertable from a test (the private fields are not visible and the layer below already swallows it), so the regression test is the unit-level one the issue allows: it builds a deflate-enabled WsConn via wsSetup, calls wsStreamClosed twice and checks the ws stays closed without crashing, under `nimble testdeflate`. Closes #264
cryo2010
added a commit
that referenced
this pull request
Oct 1, 2026
…, SSE id sanitization (#265) and a growth-stable connection table (#343) (#384) * fix(h2): close the last maxControlFrames bypasses and cover the budget 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 * fix(h2,h3): close the remaining content-length reconciliation gaps on 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 * fix(h2,h3): reject content-length in a request trailer section, and pin 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 * fix(sse): sanitize NUL out of field values and never send an empty id 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 * fix(eventloop): serve a high fd by growing a segmented connection table 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 * test(h2): cover the per-connection buffered-body memory cap 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 * fix(h2): time a zero-window response stall from the timeout sweep too 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 * fix(eventloop,sse): assert the connection table's thread rule and tighten 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 * test: prove the pin overlap explicitly and run the table growth under 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 * test(h2client): send with MSG_NOSIGNAL on Linux in the shared sendAll 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). * fix(h2): charge window dribbles to the budget and earn credit per DATA 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 * docs(h2): say the budget charges a frame before its reply, not the reply 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. * fix(h3): skip dispatch of a streaming request already rejected for its 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 * fix(h1,h2,h3): drop content-length and te from response trailers on every 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 * test(h2): drain the server preface deterministically and drop the shadowing 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.
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.
Summary
One commit per issue, in the order requested (344, 343, 342, 271, 239, 240, 249, 259 to 264), plus a changelog commit.
Code fixes
fix(loop): route every body-pause clear through one helperclearBodyPauseis now the single owner of "no longer read-paused" and releases thebodyPausedConnsslot; a debug-build audit asserts the count every second. The leak turned out to be reachable: a worker or async response applied while the read was still paused (resumeAfterRespond) leaked one slot per request. Closes #344.fix(loop): count and log the accept dropped by a pinned tablehandleAcceptis counted per loop and logged at most once a second with the fd, table size and running count. The segmented connection table is not attempted here (it touches everyconns[]walk), so this isRefs #343.fix(h2): send GOAWAY before a timeout, cap or force closecloseWithGoaway: GOAWAY(NO_ERROR) before the timeout-sweep andforceCloseAllcloses, GOAWAY(ENHANCE_YOUR_CALM) at the recv-buffer cap, best-effort non-blocking flush, thencloseConn. Pinned connections skip the frame (same ORC reasoning asmarkDrain). Closes #342.fix(h1): ship writeTimeout on by default; document the 431 header capwriteTimeoutdefaults to 30 s (matchesbodyTimeout; idle-style, re-armed on every partial write). README/HARDENING now say 431 formaxHeaderCount, and that the limit is HTTP/1 only (h2/h3 bound the header list by size). Closes #249.fix(h3): keep WebSocket frames coalesced with the CONNECT handshakeh3WsAcceptseedsw.inBuffromst.bodylikeh2WsAccept;h3Drivepumps the seeded frames after the handler installsonMessage. Closes #259.fix(ws): guard subprotocol() with the loop-thread checksubprotocolgoes throughwithWsConnlike the sibling accessors; off-loop it reports "". Closes #260.fix(h3): fire onClose when a ws client half-closes before acceptw.preAcceptFin = st.finSeeninh3WsAccept; the post-accept pump replayswsPeerClosed. Closes #261.fix(h3): drive QUIC egress for a loop-thread ws send or closeflushImplfor anfd < 0handle setsloop.h3FlushPending; the run loop drivesh3Driveonce more after the pump/tick when it is set. A flag rather than a direct drive, because the hook can run inside an nghttp3 callback. Closes #262.fix(h3): bound pre-accept ws bytes as WebSocket data, not a bodymaxWsMessage + 1024, kept out ofbufferedBytes, credited as tunnel bytes, and an overflow resets with H3_REQUEST_REJECTED. Closes #263.fix(ws): make the permessage-deflate teardown idempotentwsClosePmdfrees the zlib contexts exactly once (pmdClosed), so a secondwsStreamClosedis a no-op. Closes #264.Already fixed on main, missing tests
Three of the listed issues were fixed by earlier PRs but never closed. These commits add the regression coverage that was missing (each new test was verified to fail with the fix reverted):
24d7fd7)test(h2): lock stream-level error scope and racing-frame tolerance: newtests/test_http2_stream_errors.nim. Closes #239.24d7fd7)test(h2): cover the conformance follow-ups from #240: 13 cases intest_http2_malformed.nimplus ws-over-h2 and h3 classifier cases. Sub-item 2 (omBlockingDone flush) is verified by inspection only, since #341's batch flush masks it from a client. Closes #240.7fc9122)test(h1): cover the streaming read-ahead backpressure bound: two cases intest_streaming_read.nim(Content-Length and chunked) plus HARDENING docs. Closes #271.Behaviour changes to note
writeTimeoutdefault 0 -> 30 s. Set it to 0 to restore the old behaviour.closeWithGoawayrunsflushOut, which also resumes parked h2 producers on the recv-cap path where a barecloseConnused to run.st.bodyReceived, so acontent-lengthon an Extended CONNECT is not reconciled at FIN (matches the post-accept tunnel path).Follow-ups surfaced (not in this PR)
handleAcceptnever has to drop.ackBodyfrom a non-loop thread is still silently dropped (documented loop-thread only); with HTTP/1 streaming uploads: no read backpressure for async (manualAck) consumers → unbounded memory #271's bound that leaves the read paused untilbodyTimeoutrather than being a harmless no-op.Validation
Full suite
tests/run.shon the integrated branch: 80 suites, 590 checks, 0 failures.nimble h3websocket(aioquic, Docker): all cases pass, including the three new ones (coalesced handshake + frame, half-close before accept, deferred send).nimble testdeflate,nimble testchronos: pass.Zero-dependency
-d:plainHttpbuild compiles.Stress smoke
VORTEX_PROTO=all VORTEX_SERVER=all VORTEX_SECONDS=10 VORTEX_REPORT_SECONDS=2 nimble stress:== stress smoke: all workloads passed ==. All 75 cells green (requests, ws, sse, streamupload, streamdownload; each over h1, h2, h3 for sync, async, async-await, chronos, chronos-await), chaos sidecar passed in every cell, no panics, nobodyPausedConns driftassertion from the new debug audit. The only tracebacks in the log are the known aioquic sidecar noise (_handle_timerfiring on a closed transport aftervanish).