Skip to content

#144 libuv: notify parked reader on close (no-wake hang)#148

Closed
EdmondDantes wants to merge 2 commits into
mainfrom
144-proc-pipe-no-eof-wake
Closed

#144 libuv: notify parked reader on close (no-wake hang)#148
EdmondDantes wants to merge 2 commits into
mainfrom
144-proc-pipe-no-eof-wake

Conversation

@EdmondDantes
Copy link
Copy Markdown
Contributor

Bug

`libuv_io_close` called `uv_read_stop` without notifying the parked `active_req`. Any STREAM-type IO handle (PIPE/TCP/TTY) closed while a coroutine was parked in `fread()` via `uv_read_start` silently dropped the read watcher — the reader hung forever on an event that would never fire again. Deadlock detector eventually aborted.

Why it surfaces with proc_open + proc_close

Reproduced cleanly outside the chaos harness in ~25 lines:

```php
$proc = proc_open([$php, '-r', 'usleep(200000);'],
[0 => ['pipe', 'r'], 1 => ['pipe', 'w'], 2 => ['pipe', 'w']], $pipes);

spawn(fn() => fread($pipes[1], 4096)); // parks
spawn(function() use ($proc) {
delay(50);
proc_terminate($proc, 15);
proc_close($proc); // → resource dtor → fclose pipes
});
```

`proc_open_rsrc_dtor` closes every pipe stream → `php_stdiop_on_async_detach` → `libuv_io_close` — and the parked reader hangs.

Probe (added `fprintf` to `io_pipe_read_cb`) confirmed the callback is never called in this path, while a self-exiting child (no external close) triggers `UV_EOF` correctly. So the bug isn't in libuv's POLLHUP detection — it's that the close path silently stops watching.

Fix

In `libuv_io_close`, before `uv_read_stop`, mark the parked `active_req` completed, set the EOF state bit, and `ZEND_ASYNC_CALLBACKS_NOTIFY` — the reader wakes with a clean EOF, same shape as the `io_pipe_read_cb` `UV_EOF` path.

11 lines, NULL-guarded, no ABI changes.

Audit

Grepped other `uv_read_stop` callsites:

  • `libuv_io_event_stop` (line 3897) has the same pattern, but the event ref-count keeps stop unreachable while a coroutine is parked on the read req. Theoretical follow-up; not the bug here.
  • All non-STREAM types (UDP / FILE) take different paths that don't share this race.

Test

Fixes #144

libuv_io_close ran uv_read_stop without notifying the parked active_req,
so closing a STREAM-type IO handle (PIPE/TCP/TTY) while another coroutine
was parked in fread() (via uv_read_start) silently dropped the read
watcher. The reader waited forever on an event that never fired again;
the deadlock detector eventually aborted the request.

Repro: proc_open + a coroutine parked in fread($pipes[1]); killer calls
proc_terminate + proc_close — proc_open's resource dtor closes the pipe
through the PHP stream → php_stdiop_on_async_detach → libuv_io_close.

Fix: before uv_read_stop, mark the parked req completed (transferred=0),
set the EOF state bit, and ZEND_ASYNC_CALLBACKS_NOTIFY so the reader
wakes with a clean EOF — same shape as the io_pipe_read_cb UV_EOF path.

Regression test tests/exec/025-proc_close_wakes_parked_fread.phpt.
Full ext/async/tests io/exec/stream suite green on NTS + ASAN-ZTS.

Fixes #144
@EdmondDantes
Copy link
Copy Markdown
Contributor Author

CI catches a real heap-use-after-free this fix introduces (LINUX_X64_DEBUG_ZTS_ASAN):

```
==89119==ERROR: AddressSanitizer: heap-use-after-free
SUMMARY: …php_stdiop_read +697 → stream->eof = 1
FAIL proc_close wakes parked fread on child stdout pipe
```

Root analysis: my notify wakes the parked reader, but proc_closeproc_open_rsrc_dtorzend_list_closestream_resource_regular_dtorphp_stream_free(stream, PHP_STREAM_FREE_CLOSE) unconditionally pefrees the php_stream (and its php_stdio_stream_data) regardless of resource refcount. By the time the parked coroutine resumes inside php_stdiop_read, both stream and data are freed. Every post-suspend access in that function (stream->eof, stream->flags, req->dispose) is UAF.

Both code paths after the suspend touch stream — the EOF branch (stream->eof = 1) AND the error/cancellation branch (stream->flags & PHP_STREAM_FLAG_SUPPRESS_ERRORS). Waking with EOF, with an exception, or with cancellation — all UAF.

The right fix is architectural: php_stream_free must defer the pefree while a parked reader holds a reference (i.e. while data->async_io->active_req != NULL). That's a stream-lifecycle change spanning main/streams/streams.c, php_stdiop_close, and the async-IO contract — much larger than a single-callsite notify.

Closing this PR; reopening the underlying issue with the deeper analysis. The four cancel-mid-read scenarios in fuzzy-tests/exec/proc_open_chaos.feature remain commented under # Blocked: #144 pending the redesign.

Locally (debug+ASAN+ZTS) the test passed all 8 reruns — the freed slot wasn't immediately reused, so ASAN didn't catch. CI's allocator pressure reproduces it deterministically.

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.

Reactor does not wake parked fread() on child pipe when proc is terminated + reaped

1 participant