From ef34a6eaa06ad350f1766a2dc9b0ec5557badc72 Mon Sep 17 00:00:00 2001 From: Ilia Alshanetsky Date: Sun, 14 Jun 2026 19:31:12 -0400 Subject: [PATCH] Fix Io\Poll memory-safety and error-handling issues Several issues in the new Io\Poll API, found by review and confirmed under valgrind: - Watcher kept a raw pointer to its Context's php_poll_ctx with no reference, so dropping the Context while holding a Watcher left remove()/modify() dereferencing freed memory (use-after-free). The Context now neutralizes its watchers (active=false, poll_ctx=NULL) before it is destroyed, so those calls throw InactiveWatcherException. - StreamPollHandle took a reference on the stream resource in the constructor but never released it, leaking the descriptor for the rest of the request. Store the zend_resource and release it in the handle cleanup; the php_stream may already be freed by then (e.g. the user closed it), so the cleanup must not dereference it. - Watcher and Context had no get_gc handler, so reference cycles through Watcher::$data were uncollectable. Add get_gc for both. - The epoll and poll backends returned -1 from wait() without recording the error, so the thrown exception carried a stale code. Set it from errno, which maps EINTR to the interrupted error. - The kqueue grouped-event path bounded the result buffer only with a ZEND_ASSERT, allowing an out-of-bounds write in release builds when more distinct descriptors were ready than the caller's maxEvents. Cap it at runtime, matching the raw-event path. - php_poll_create_by_name() dereferenced a NULL backend name; guard it. --- ext/standard/io_poll.c | 49 ++++++++++++++++++- .../poll_stream_handle_close_then_free.phpt | 20 ++++++++ .../poll/poll_stream_handle_fd_release.phpt | 28 +++++++++++ .../tests/poll/poll_watcher_gc_cycle.phpt | 31 ++++++++++++ .../poll/poll_watcher_outlives_context.phpt | 35 +++++++++++++ main/poll/poll_backend_epoll.c | 2 + main/poll/poll_backend_kqueue.c | 4 +- main/poll/poll_backend_poll.c | 8 ++- main/poll/poll_core.c | 4 ++ 9 files changed, 176 insertions(+), 5 deletions(-) create mode 100644 ext/standard/tests/poll/poll_stream_handle_close_then_free.phpt create mode 100644 ext/standard/tests/poll/poll_stream_handle_fd_release.phpt create mode 100644 ext/standard/tests/poll/poll_watcher_gc_cycle.phpt create mode 100644 ext/standard/tests/poll/poll_watcher_outlives_context.phpt diff --git a/ext/standard/io_poll.c b/ext/standard/io_poll.c index c500247dc8be..8ed10ffb8a3b 100644 --- a/ext/standard/io_poll.c +++ b/ext/standard/io_poll.c @@ -64,6 +64,7 @@ typedef struct { /* Stream poll handle specific data */ typedef struct { php_stream *stream; + zend_resource *res; } php_stream_poll_handle_data; /* Accessor macros */ @@ -250,7 +251,9 @@ static void php_stream_poll_handle_cleanup(php_poll_handle_object *handle) { php_stream_poll_handle_data *data = (php_stream_poll_handle_data *) handle->handle_data; if (data) { - /* Don't close the stream - user still owns it */ + if (data->res) { + zend_list_delete(data->res); + } efree(data); handle->handle_data = NULL; } @@ -331,6 +334,15 @@ static void php_io_poll_context_free_object(zend_object *obj) { php_io_poll_context_object *intern = PHP_POLL_CONTEXT_OBJ_FROM_ZOBJ(obj); + if (intern->watchers) { + zval *zv; + ZEND_HASH_FOREACH_VAL(intern->watchers, zv) { + php_io_poll_watcher_object *watcher = PHP_POLL_WATCHER_OBJ_FROM_ZOBJ(Z_OBJ_P(zv)); + watcher->active = false; + watcher->poll_ctx = NULL; + } ZEND_HASH_FOREACH_END(); + } + if (intern->ctx) { php_poll_destroy(intern->ctx); } @@ -343,6 +355,36 @@ static void php_io_poll_context_free_object(zend_object *obj) zend_object_std_dtor(&intern->std); } +static HashTable *php_io_poll_watcher_get_gc(zend_object *obj, zval **table, int *n) +{ + php_io_poll_watcher_object *intern = PHP_POLL_WATCHER_OBJ_FROM_ZOBJ(obj); + zend_get_gc_buffer *gc_buffer = zend_get_gc_buffer_create(); + + zend_get_gc_buffer_add_zval(gc_buffer, &intern->data); + if (intern->handle) { + zend_get_gc_buffer_add_obj(gc_buffer, &intern->handle->std); + } + + zend_get_gc_buffer_use(gc_buffer, table, n); + return zend_std_get_properties(obj); +} + +static HashTable *php_io_poll_context_get_gc(zend_object *obj, zval **table, int *n) +{ + php_io_poll_context_object *intern = PHP_POLL_CONTEXT_OBJ_FROM_ZOBJ(obj); + zend_get_gc_buffer *gc_buffer = zend_get_gc_buffer_create(); + + if (intern->watchers) { + zval *zv; + ZEND_HASH_FOREACH_VAL(intern->watchers, zv) { + zend_get_gc_buffer_add_zval(gc_buffer, zv); + } ZEND_HASH_FOREACH_END(); + } + + zend_get_gc_buffer_use(gc_buffer, table, n); + return zend_std_get_properties(obj); +} + /* Utility functions */ static zend_always_inline zend_ulong php_io_poll_compute_ptr_key(void *ptr) @@ -451,10 +493,11 @@ PHP_METHOD(StreamPollHandle, __construct) /* Set up stream-specific data */ php_stream_poll_handle_data *data = emalloc(sizeof(php_stream_poll_handle_data)); data->stream = stream; + data->res = stream->res; intern->handle_data = data; /* Add reference to stream */ - GC_ADDREF(stream->res); + GC_ADDREF(data->res); } PHP_METHOD(StreamPollHandle, getStream) @@ -871,6 +914,7 @@ PHP_MINIT_FUNCTION(poll) sizeof(zend_object_handlers)); php_io_poll_watcher_object_handlers.offset = offsetof(php_io_poll_watcher_object, std); php_io_poll_watcher_object_handlers.free_obj = php_io_poll_watcher_free_object; + php_io_poll_watcher_object_handlers.get_gc = php_io_poll_watcher_get_gc; php_io_poll_watcher_class_entry->default_object_handlers = &php_io_poll_watcher_object_handlers; /* Register Context class */ @@ -881,6 +925,7 @@ PHP_MINIT_FUNCTION(poll) sizeof(zend_object_handlers)); php_io_poll_context_object_handlers.offset = offsetof(php_io_poll_context_object, std); php_io_poll_context_object_handlers.free_obj = php_io_poll_context_free_object; + php_io_poll_context_object_handlers.get_gc = php_io_poll_context_get_gc; php_io_poll_context_class_entry->default_object_handlers = &php_io_poll_context_object_handlers; /* Register exception hierarchy */ diff --git a/ext/standard/tests/poll/poll_stream_handle_close_then_free.phpt b/ext/standard/tests/poll/poll_stream_handle_close_then_free.phpt new file mode 100644 index 000000000000..9a97fad8dd1c --- /dev/null +++ b/ext/standard/tests/poll/poll_stream_handle_close_then_free.phpt @@ -0,0 +1,20 @@ +--TEST-- +Io\Poll: StreamPollHandle cleanup is safe when the stream is closed first +--FILE-- +add(new StreamPollHandle($r), [Io\Poll\Event::Read]); + +// Close the underlying streams before the watcher and handle are freed. +fclose($r); +fclose($w); + +unset($watcher, $ctx); +gc_collect_cycles(); +echo "ok\n"; +?> +--EXPECT-- +ok diff --git a/ext/standard/tests/poll/poll_stream_handle_fd_release.phpt b/ext/standard/tests/poll/poll_stream_handle_fd_release.phpt new file mode 100644 index 000000000000..66d042c17138 --- /dev/null +++ b/ext/standard/tests/poll/poll_stream_handle_fd_release.phpt @@ -0,0 +1,28 @@ +--TEST-- +Io\Poll: StreamPollHandle releases its stream resource (no fd leak) +--SKIPIF-- + +--FILE-- + +--EXPECT-- +bool(true) diff --git a/ext/standard/tests/poll/poll_watcher_gc_cycle.phpt b/ext/standard/tests/poll/poll_watcher_gc_cycle.phpt new file mode 100644 index 000000000000..590649acef8f --- /dev/null +++ b/ext/standard/tests/poll/poll_watcher_gc_cycle.phpt @@ -0,0 +1,31 @@ +--TEST-- +Io\Poll: cycle collector reclaims a Watcher referenced through its own data +--FILE-- +add(new StreamPollHandle($r), [Io\Poll\Event::Read]); + +$c = new Canary(); +$c->ref = $watcher; // Canary -> Watcher +$watcher->modifyData($c); // Watcher->data -> Canary (cycle) + +unset($ctx, $watcher, $c, $r, $w); + +echo "before gc\n"; +gc_collect_cycles(); +echo "after gc\n"; +?> +--EXPECT-- +before gc +Canary freed +after gc diff --git a/ext/standard/tests/poll/poll_watcher_outlives_context.phpt b/ext/standard/tests/poll/poll_watcher_outlives_context.phpt new file mode 100644 index 000000000000..2c058d5b8966 --- /dev/null +++ b/ext/standard/tests/poll/poll_watcher_outlives_context.phpt @@ -0,0 +1,35 @@ +--TEST-- +Io\Poll: Watcher operations are safe after its Context is destroyed +--FILE-- +add(new StreamPollHandle($r), [Io\Poll\Event::Read], "data"); + +// Drop the Context while still holding the Watcher it returned. +unset($ctx); +gc_collect_cycles(); + +var_dump($watcher->isActive()); + +try { + $watcher->remove(); +} catch (Io\Poll\InactiveWatcherException $e) { + echo $e->getMessage(), "\n"; +} + +try { + $watcher->modifyEvents([Io\Poll\Event::Write]); +} catch (Io\Poll\InactiveWatcherException $e) { + echo $e->getMessage(), "\n"; +} + +echo "done\n"; +?> +--EXPECT-- +bool(false) +Cannot remove inactive watcher +Cannot modify inactive watcher +done diff --git a/main/poll/poll_backend_epoll.c b/main/poll/poll_backend_epoll.c index b0dbc4c7dbcf..21d52f1c89db 100644 --- a/main/poll/poll_backend_epoll.c +++ b/main/poll/poll_backend_epoll.c @@ -195,6 +195,8 @@ static int epoll_backend_wait( events[i].revents = epoll_events_from_native(backend_data->events[i].events); events[i].data = backend_data->events[i].data.ptr; } + } else if (nfds < 0) { + php_poll_set_current_errno_error(ctx); } return nfds; diff --git a/main/poll/poll_backend_kqueue.c b/main/poll/poll_backend_kqueue.c index 9a654c716d56..83fe7ae4ce65 100644 --- a/main/poll/poll_backend_kqueue.c +++ b/main/poll/poll_backend_kqueue.c @@ -382,7 +382,9 @@ static int kqueue_backend_wait( if (!found) { /* New FD, create new event */ - ZEND_ASSERT(unique_events < max_events); + if (unique_events >= max_events) { + continue; + } events[unique_events].fd = fd; events[unique_events].events = 0; events[unique_events].revents = revents; diff --git a/main/poll/poll_backend_poll.c b/main/poll/poll_backend_poll.c index 311c48529bc7..be58c832d4e8 100644 --- a/main/poll/poll_backend_poll.c +++ b/main/poll/poll_backend_poll.c @@ -215,8 +215,12 @@ static int poll_backend_wait( int timeout_ms = php_poll_timespec_to_ms(timeout); int nfds = poll(backend_data->temp_fds, fd_count, timeout_ms); - if (nfds <= 0) { - return nfds; /* Return 0 for timeout, -1 for error */ + if (nfds < 0) { + php_poll_set_current_errno_error(ctx); + return -1; + } + if (nfds == 0) { + return 0; /* timeout */ } /* Process results - iterate through struct pollfd array directly */ diff --git a/main/poll/poll_core.c b/main/poll/poll_core.c index 8422e46c9379..41e0cc1b019d 100644 --- a/main/poll/poll_core.c +++ b/main/poll/poll_core.c @@ -168,6 +168,10 @@ PHPAPI php_poll_ctx *php_poll_create(php_poll_backend_type preferred_backend, ui /* Create new poll context */ PHPAPI php_poll_ctx *php_poll_create_by_name(const char *preferred_backend, uint32_t flags) { + if (!preferred_backend) { + return NULL; + } + if (!strcmp(preferred_backend, "auto")) { return php_poll_create(PHP_POLL_BACKEND_AUTO, flags); }