The reactor owns the HTTP/1 frame, so a cancelled write cannot outlive it (#179) - #186
Merged
Conversation
…utlive it (#179) A streamed chunk was written from a buffer the caller owned, and libuv does not copy: uv_write keeps the pointer until its completion callback. The caller freed it when its own wait ended, and once the handler coroutine is cancelled that moment arrives first — the wait is over, the write is still queued. Reproduced: a handler parked writing to a peer with SO_RCVBUF at 4 KiB that never reads, setShutdownTimeout(0) so stop() skips the grace window and cancels at once. The await returns with the request incomplete, dispose defers because the write is in flight, and the caller frees a 20008-byte frame; stamping the block showed the allocator hands the same address straight back. On plaintext the frame now goes out as slots through a vectored write the reactor owns — no copy of the body either, and no threshold. TLS keeps the coalesced copy: tls_push copies into the BIO ring anyway, and a vectored write aimed at the socket would put plaintext on a TLS connection. The whole path is behind ZEND_ASYNC_IO_WRITEV_AWAIT, so the extension still builds and works against a reactor without it. Needs true-async/php-src#24 and true-async/php-async#262.
Contributor
CoverageTotal lines: 82.23% → 82.13% (-0.10 pp)
❌ Regression in touched files (> 1.0 pp drop)
Add |
…d one (#179) zend_string_init_interned writes interned_strings_permanent, a process-wide table the engine treats as read-only after startup and guards with nothing. Calling it lazily from a worker thread is the shape that already produced a SEGV on the HTTP/2 header path. Two bytes per frame instead.
…oo (#179) The macro name says the header knows the flag; only the version says the reactor keeps it. A build pairing a new header with an old reactor ran the write as an ordinary fire-and-forget writev and waited for a notification nobody sends — a hang, or a dispose through a freed request once the write deadline closed the handle. h1_emit_headers_once and the empty-first-chunk branch had the same lifetime defect as the frame: send a header block the caller owns, then release it while libuv may still hold the pointer. Both go through one helper now, which hands the block to the reactor where it can.
It asserted only the exception, which the path it replaced produced just as readily — every revert in the diff passed it. It now checks the opening frames byte for byte, so a frame built in the wrong order or from a released slot fails, and checks that no terminator follows the cancellation, which is the invariant that keeps a cut frame from desynchronising the next request. The write timeout is one second so a lost wake fails instead of hanging.
…179) mark_ended refuses the terminator when the stream is dead, and the dispose path skips mark_ended entirely: it emitted 0\r\n\r\n on its own and left keep_alive true. The peer then reads the terminator as the first bytes of the chunk the orphaned size line promised, and the next request on the connection desyncs — the defect 4c7824c closed in one place and not the other. Caught by 030 on macOS, where the write still lands; on Linux the socket is gone by then and the same code passes.
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.
Depends on true-async/php-src#24 (the flag) and true-async/php-async#262 (the
reactor). Everything here is behind
#ifdef ZEND_ASYNC_IO_WRITEV_AWAIT, so theextension builds and passes against a reactor without them — CI proves that half
until they merge.
The defect
h1_stream_append_chunkwrote a streamed chunk from a buffer it owned, andlibuv does not copy:
uv_writekeeps the caller's pointer until its completioncallback. The caller freed that buffer when its own wait ended. Those are
the same moment right up until the handler coroutine is cancelled, and then the
wait is over while the write is still in libuv's queue.
Reproduced, and the recipe is why nobody had: the peer must exist and must not
read, so the write parks instead of failing (
SO_RCVBUFat 4 KiB); the cancelmust arrive while the handler is parked, which means
setShutdownTimeout(0)—stop()otherwise waits five seconds for handlers to finish and they alwayswin; and the response must be streaming. With all three, probes fire in order:
the await returns with
req->completed == false,libuv_io_req_disposedefersbecause
ASYNC_IO_REQ_F_UV_IN_FLIGHTis set, and the caller frees a 20008-byteframe. Stamping the freed block showed the allocator hands the same address
straight back.
What I did not manage to show is the disclosure: in that scenario the
connection is closed before the loop gets another writable event, so the
poisoned bytes never left. It is a dangling pointer in libuv's queue whose
dereference depends on timing, not a demonstrated leak. The rule stands either
way — a buffer an in-flight write points at must not be freed.
The fix
On plaintext the frame goes out as slots — header block, hex size line, chunk,
CRLF — through
http_connection_send_strv_awaited, a vectored write the reactorowns and the caller waits for. The reactor holds one reference per slot until
libuv is done, so nothing the caller built can be freed early. The body is no
longer copied and the size threshold does not apply.
TLS keeps the coalesced copy:
http_connection_sendroutes throughtls_push,which copies into the BIO ring anyway, and a vectored write aimed straight at
the socket would put plaintext on a TLS connection.
The CRLF slot is
zend_string_init_interned. That is not decoration: the sendreleases every slot, and an interned string ignores both the addref and the
release, while a persistent-but-not-interned literal would be decremented once
per frame and freed under the next one.
The test, and what it is for
tests/phpt/server/h1/030-h1-cancel-while-parked-in-write.phptbuilds exactlythat shape: parked in a write, cancelled, unwinding. No test in this suite did
that — which is the honest reason a lifetime defect on this path stayed green
through 391 tests. It asserts the contract rather than the defect: the
cancellation arrives as
HttpException499,isWritable()is false afterwards,and the process ends. A hang means the cancel never landed; a crash means a
frame outlived something it pointed at. Ran it five times for stability.
tests/phpt: 392 passed, 0 failed, 24 skipped — on a PHP whose header has noflag, i.e. through the fallback path.