diff --git a/CHANGELOG.md b/CHANGELOG.md index a2e3b66f..e771eeec 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -30,6 +30,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- **On HTTP/3 the server wrote packets from inside ngtcp2's packet read, which ngtcp2 forbids (#428).** A static file or `sendFile()` response finishes when nghttp3 closes its stream, and nghttp3 does that from inside `ngtcp2_conn_read_pkt` while an ACK is processed; the dispose tail that runs then called `ngtcp2_conn_writev_stream`, which `ngtcp2.h` says must not be called from a callback, and could re-enter a drain already in progress. No failure was traced to it, but a debug assertion that no drain runs inside `read_pkt` failed 9 h3 tests (`h3/039`, `052`, `053`, `054`, `090`, `091`, `099`, `101`, `102`). A drain requested there now marks the connection, and the read path flushes it once the datagram is processed; the assertion stays in debug builds, so a write from a callback fails the h3 suite. - **Over HTTP/2 and HTTP/3 a request body the server refused reached the client as a bare reset, with no status (#412).** A body past `setMaxBodySize()`, or a multipart form over `max_input_vars` or malformed, reset the stream at once: HTTP/2 sent RST_STREAM(ENHANCE_YOUR_CALM) or (CANCEL), HTTP/3 STOP_SENDING and RESET_STREAM(H3_REQUEST_REJECTED), and the 413 or 400 the handler's exception carried could not be sent on a stream already closing. A handler that caught the refusal and answered itself was lost the same way, and so was a response already decided when the upload crossed the cap: an early 403 in flight on HTTP/3 arrived as 27,925 of 1,048,576 bytes, and a finished `sendFile()` on HTTP/2 was followed by RST_STREAM 11. Now a refusal with no response decided cancels the handler with `HttpException` 413 or 400 and answers that status, and a decided response is delivered whole; then the upload is stopped with RST_STREAM(NO_ERROR) or STOP_SENDING(H3_NO_ERROR), as RFC 9113 §8.1 and RFC 9114 §4.1 allow and nginx does. Under the reactor pool the reactor answers the 413 itself. A streamed response a handler is still writing, a WebSocket stream and a gRPC call keep the error reset. A client reset after a caught refusal reaches the handler as 499, not as the refusal again. Evidence: `h2/074` to `h2/081`, `h2/083`, `h3/104` to `h3/106`, `h3/109` fail before and pass 10 of 10 after; `h2/061`, `h2/066` to `h2/068` now expect the status. - **Every refused streaming HTTP/2 or HTTP/3 body kept part of the connection's flow-control window for good (#412).** The bytes queued and not yet read when the body was refused, and the DATA chunk that crossed the cap, were never credited back to the connection, so enough refusals on one connection stalled it: with a 60 KiB cap, the 257th refused upload on an HTTP/2 connection met GOAWAY and the next request got no answer. They are credited at the refusal now, and DATA arriving after it is credited to the connection alone (`h2/081`: 300 refused uploads, then a request answered 200). - **Under the reactor pool HTTP/3 ignored `setMaxBodySize()` (#412).** A reactor thread never runs `HttpServer::start()`, so it held every HTTP/3 body to the compiled 16 MiB: a 200 KiB body against a 64 KiB limit reached the handler. The reactor context now carries the limit (`h3/106`). diff --git a/dev/PLAN.md b/dev/PLAN.md index 7c3864c9..d0d4f8a2 100644 --- a/dev/PLAN.md +++ b/dev/PLAN.md @@ -233,7 +233,7 @@ epilogue (an eventfd write per pooled response batch) and a direct drain at handoff: PR 425 merged by Edmond's word on 2026-10-19 as 44db002, every check green including `WINDOWS_X64_ZTS_RELEASE`; #424 closed by the merge. -## S6 — A refused request body answers with its status on HTTP/2 and HTTP/3 (#412, item 46) [in progress] +## S6 — A refused request body answers with its status on HTTP/2 and HTTP/3 (#412, item 46) [done] Goal: a body refused by the transport (over `setMaxBodySize()`, a multipart form over `max_input_vars` or malformed, out of memory) reaches an HTTP/2 or HTTP/3 @@ -334,10 +334,16 @@ takes the HttpException code 400-599 as H1 and H2 do, for every exception. 2 skipped, 0 failed; cmocka 24 of 24 after unit stubs for the new response calls. Reactor threads read `max_body_size` from `http3_reactor_ctx_t`. -- [~] S6.4 Docs, CHANGELOG, PR, CI green on every platform +- [x] S6.4 Docs, CHANGELOG, PR, CI green on every platform done: setMaxBodySize docs and USAGE.md describe the answer; PR merged; #412 closed; the gRPC refusal has its own issue tier: T1 · role: — + handoff: PR 427 merged as f1932f7, all 8 checks green after a second run + (the first failed `Fuzz embedded` at link: three stubs added to + `fuzz/fuzz_stubs.c`). USAGE.md has no body-limit section; the stub + docblocks of setMaxBodySize and getPost carry the change. #412 closed; + gRPC split out as #426; the Critic's pre-existing H3 `on_done` finding + is #428 (item 47). ## Next @@ -618,7 +624,18 @@ Added by the health check of 2026-09-30 (`dev/HEALTH.md`), in this order: done: PR 425 (44db002). 46. #412 — a refused HTTP/2 or HTTP/3 body reaches the client as a reset with no status. Stage S6. #413 (a query string past `max_input_vars`) waits for - it: its refusal happens after the handler started too. + it: its refusal happens after the handler started too. Done: PR 427. +47. #428 — on HTTP/3 a finished static or `sendFile()` transfer runs the full + dispose tail, `drain_out` included, from nghttp3's stream_close inside + `ngtcp2_conn_read_pkt`; ngtcp2 forbids writing from its callbacks. Found by + the Critic on #427. Edmond, 2026-10-20: its own issue, fixed next. + Done: `in_read_pkt` is set around `ngtcp2_conn_read_pkt`, and + `http3_connection_drain_out` called while it is set marks the connection + for the read path's flush instead of writing. A debug assertion of the + same condition failed 9 h3 tests without the deferral and none with it; + full phpt 629 passed, 2 skipped. T1, no Critic. +48. #426 — a refused gRPC request body resets the stream with no + `grpc-status`. Reproduce before any code. 43. #339 — a client filename with path components: done. Edmond, 2026-10-19: behave as PHP. The name is cut after the last `/` or `\` (`php_ap_basename`, static in `main/rfc1867.c`, so copied) and the file is diff --git a/src/http3/http3_connection.c b/src/http3/http3_connection.c index b44fde69..926207b4 100644 --- a/src/http3/http3_connection.c +++ b/src/http3/http3_connection.c @@ -808,10 +808,12 @@ bool http3_connection_dispatch( * pkt_info that read_pkt actually consumes today. ngtcp2 masks to * the 2 ECN bits internally; we pass the full TOS byte through. */ ngtcp2_pkt_info pi = { .ecn = ecn }; + conn->in_read_pkt = true; int pkt_rv = ngtcp2_conn_read_pkt( (ngtcp2_conn *)conn->ngtcp2_conn, &ps.path, &pi, data, datalen, http3_ts_now()); + conn->in_read_pkt = false; if (UNEXPECTED(pkt_rv != 0)) { http_log_state_t *st = conn->log_state; diff --git a/src/http3/http3_connection.h b/src/http3/http3_connection.h index fe5f9117..bebf93dd 100644 --- a/src/http3/http3_connection.h +++ b/src/http3/http3_connection.h @@ -162,6 +162,12 @@ struct _http3_connection_s { * callback from re-entering a dying conn. */ bool closed; + /* Set while ngtcp2_conn_read_pkt runs. ngtcp2 and nghttp3 call back into + * the server from inside it, and ngtcp2 forbids writing packets from its + * callbacks: http3_connection_drain_out leaves the output to the read + * path's flush while this is set. */ + bool in_read_pkt; + /* Set once http3_connection_emit_close has been called so teardown * does not emit a second CONNECTION_CLOSE on the same conn. */ bool sent_connection_close; diff --git a/src/http3/http3_io.c b/src/http3/http3_io.c index f2b0e880..b215f258 100644 --- a/src/http3/http3_io.c +++ b/src/http3/http3_io.c @@ -225,6 +225,22 @@ void http3_connection_drain_out(http3_connection_t *c) return; } + /* Called back from inside ngtcp2_conn_read_pkt (a stream close that + * finishes a static or sendFile response, a refusal answered in place): + * ngtcp2 forbids writev_stream there. The read path flushes the + * connection once the datagram is processed; inside a poll tick or a + * reactor drain the request below only links the listener. */ + if (c->in_read_pkt) { + http3_listener_mark_flush(c->listener, c); + http3_listener_request_flush(c->listener); + return; + } + + /* The debug build's check that nothing below writes from a callback; + * h3 tests that finish a static or sendFile response trip it without + * the return above. */ + ZEND_ASSERT(!c->in_read_pkt); + http3_packet_stats_t *const stats = http3_listener_packet_stats(c->listener); /* Loop until ngtcp2 has nothing to emit. writev_stream returns the @@ -663,6 +679,9 @@ void http3_connection_emit_close(http3_connection_t *c) NULL); ngtcp2_pkt_info pi = {0}; + /* ngtcp2 forbids writing from its callbacks; no caller closes from one. */ + ZEND_ASSERT(!c->in_read_pkt); + uint8_t buf[1280]; ngtcp2_ssize n = ngtcp2_conn_write_connection_close( qc, &ps.path, &pi, buf, sizeof(buf), &ccerr, http3_ts_now());