Skip to content

reactor: a write must not be truncated, and an awaited writev must hold its handle - #263

Open
EdmondDantes wants to merge 7 commits into
mainfrom
writev-slot-overflow
Open

reactor: a write must not be truncated, and an awaited writev must hold its handle#263
EdmondDantes wants to merge 7 commits into
mainfrom
writev-slot-overflow

Conversation

@EdmondDantes

Copy link
Copy Markdown
Contributor

Two defects in the write path, found while reviewing the awaited vectored write (#262).

A write longer than 32 bits was truncated and reported as complete

uv_buf_init() takes an unsigned int length, while uv_buf_t itself carries a
size_t on POSIX and a ULONG on Windows. Every write site went through
uv_buf_init() and clamped: libuv_io_write at INT_MAX, libuv_io_writev at
UINT_MAX per slot, libuv_udp_sendto by a plain cast that wraps. req->max_size
kept the caller's full length, and io_pipe_write_cb / io_pipe_writev_cb report
max_size as transferred on success — so an awaiter comparing transferred
against what it asked for was told a truncated write had finished. On the HTTP/1
frame path that means a chunk header announcing one length followed by a shorter
body.

async_uv_buf_set() fills the fields instead: POSIX carries the whole length, and
libuv walks its own per-buffer size_t on a short writev(2). A length the
platform field cannot hold (Windows only) is refused before submit, with nothing
on the wire, rather than sent as a prefix.

An awaited vectored write did not hold its handle

The awaiter reads the status after its resume and disposes the request there, and
libuv_io_req_dispose reads req->io. Between the completion and the resume the
handle could be closed and freed — io_close_cb runs in the same loop turn. A
fire-and-forget write needs no reference, because uv_close runs the pending write
callbacks before io_close_cb; an awaited one does. It now takes
ZEND_ASYNC_EVENT_ADD_REF after a successful submit and releases it in dispose,
and ASYNC_IO_REQ_F_AWAITED moved past the submit so a failed submit never
releases a reference it did not take.

Also here

  • A ZSTR batch of more than 65535 buffers is refused: writev_nbufs is uint16_t,
    and a wrapped count made the completion release nothing — one leaked reference per
    buffer.
  • libuv_writev_release() replaces three copies of the pre-submit release.

Evidence

  • ext/async/tests: 1104 passed, 0 failed (188 skipped).
  • true-async/server phpt suite against this reactor: 281 passed, 0 failed.
  • The truncation itself has no test: reaching it needs a buffer above 2 GiB, which
    does not belong in CI.

@EdmondDantes

Copy link
Copy Markdown
Contributor Author

Second review round, two commits on top.

A write past libuv's own limit is a short write, not a refusal. uv_write()
returns UV_EINVAL for any batch summing above UV__IO_MAX_BYTES (0x7ffff000) —
src/unix/stream.c:1318, src/win/stream.c:125 — so filling uv_buf_t.len with a
3 GiB size_t did not send 3 GiB, it failed the submit. libuv_io_write now clamps
to that limit and stores the clamped size, which is what the completion reports, so
the caller's write loop continues instead of catching an exception. The vectored path
keeps the refusal, where a frame prefix on the wire is worse than nothing.

The awaited single-buffer write pins its handle too. io_pipe_write_cb with
free_cb == NULL notifies and leaves the request to its awaiter, which disposes it
after the resume — the same dereference the vectored pin was added for. The comment
claiming no pin was needed described the callback, not the awaiter.

A failed uv_udp_send submit no longer defers its request forever.
libuv_udp_sendto sets send_req.data = req before the call, and udp_req_dispose
reads that as "a callback is in flight, free later". On a submit error no callback
ever runs, so the request and its buffer were never freed.

Suites after the round: ext/async 1104 passed / 0 failed, server phpt 281 passed /
0 failed.

@codecov

codecov Bot commented Aug 20, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@EdmondDantes

Copy link
Copy Markdown
Contributor Author

The read side of the same hazard, one commit.

An awaited read holds no reference on its handle, and libuv_io_req_dispose ends by
reading req->io. When the owner closes the handle while a reader is parked,
libuv_io_close detaches the request and wakes it, io_close_cb frees the io in the
same loop turn, and the reader's own dispose then reads freed memory.

A reference would not do here: a read request is sometimes freed by the handle's own
teardown backstop rather than by its consumer, so pinning the handle would make those
paths leak the whole io instead. The detach now clears the request's back-pointer —
both dispose paths already guard on req->io != NULL, and the completions reach the
handle through uv_handle_t::data, not through the request.

Suites: ext/async 1104 passed / 0 failed, server phpt 281 passed / 0 failed.

@EdmondDantes

Copy link
Copy Markdown
Contributor Author

A maximum-effort review round killed the handle reference this PR added two rounds ago.

The reference had one unbalanced exit. io_pipe_write_cb's free_cb branch frees the
request itself, so it never released the reference — and free_cb is a public field a
caller may set after submit, which is exactly what this product does at
src/log/http_log.c:1857 when a stream log sink is torn down with a write in flight.
Every such teardown would have leaked the async_io_t, its callbacks vector and the fd.

What replaces it costs nothing. A finished write hands the request to its awaiter and
clears req->io on the way; a handle that is closing clears the same pointer in the
requests it detaches. Both dispose paths already guard on req->io != NULL, and the
completions reach the handle through uv_handle_t::data, not through the request. No
reference, no deferred close, no promise about handle lifetime for the ABI to carry — the
header now says only that disposal is safe after the handle is gone.

Also from that round: a fire-and-forget write above ASYNC_IO_WRITE_MAX_BYTES is now
refused rather than clamped, because free_cb carries no length and a clamp there would
drop the tail in silence; libuv_udp_try_send goes through the same buffer helper; and
the helper's docblock no longer blames Windows for a narrowing that uv_buf_init performs
everywhere.

Suites after the round: ext/async 1104 passed / 0 failed, server phpt 281 passed / 0
failed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant