Fix Io\Poll memory-safety and error-handling issues

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)
@@ -448,13 +490,19 @@ PHP_METHOD(StreamPollHandle, __construct)
 
 	php_poll_handle_object *intern = PHP_POLL_HANDLE_OBJ_FROM_ZV(getThis());
 
+	if (intern->handle_data) {
+		zend_throw_error(NULL, "StreamPollHandle object is already constructed");
+		RETURN_THROWS();
+	}
+
 	/* 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)
@@ -657,6 +705,11 @@ PHP_METHOD(Io_Poll_Context, __construct)
 
 	php_io_poll_context_object *intern = PHP_POLL_CONTEXT_OBJ_FROM_ZV(getThis());
 
+	if (intern->ctx) {
+		zend_throw_error(NULL, "Io\\Poll\\Context object is already constructed");
+		RETURN_THROWS();
+	}
+
 	php_poll_backend_type backend_type = PHP_POLL_BACKEND_AUTO;
 	if (backend_obj != NULL) {
 		backend_type = php_io_poll_backend_enum_to_type(Z_OBJ_P(backend_obj));
@@ -861,6 +914,7 @@ PHP_MINIT_FUNCTION(poll)
 	memcpy(&php_io_poll_handle_object_handlers, &std_object_handlers, sizeof(zend_object_handlers));
 	php_io_poll_handle_object_handlers.offset = offsetof(php_poll_handle_object, std);
 	php_io_poll_handle_object_handlers.free_obj = php_poll_handle_object_free;
+	php_io_poll_handle_object_handlers.clone_obj = NULL;
 	php_stream_poll_handle_class_entry->default_object_handlers = &php_io_poll_handle_object_handlers;
 
 	/* Register Watcher class */
@@ -871,6 +925,8 @@ 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_object_handlers.clone_obj = NULL;
 	php_io_poll_watcher_class_entry->default_object_handlers = &php_io_poll_watcher_object_handlers;
 
 	/* Register Context class */
@@ -881,6 +937,8 @@ 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_object_handlers.clone_obj = NULL;
 	php_io_poll_context_class_entry->default_object_handlers = &php_io_poll_context_object_handlers;
 
 	/* Register exception hierarchy */

ext/standard/tests/poll/poll_clone_not_allowed.phpt

@@ -0,0 +1,26 @@
+--TEST--
+Io\Poll: Context, Watcher and StreamPollHandle are not cloneable
+--FILE--
+<?php
+require_once __DIR__ . '/poll.inc';
+
+list($r, $w) = pt_new_socket_pair();
+$ctx = pt_new_stream_poll();
+$handle = new StreamPollHandle($r);
+$watcher = $ctx->add($handle, [Io\Poll\Event::Read]);
+
+foreach ([$ctx, $handle, $watcher] as $obj) {
+    try {
+        clone $obj;
+    } catch (Error $e) {
+        echo $e->getMessage(), "\n";
+    }
+}
+
+echo "done\n";
+?>
+--EXPECT--
+Trying to clone an uncloneable object of class Io\Poll\Context
+Trying to clone an uncloneable object of class StreamPollHandle
+Trying to clone an uncloneable object of class Io\Poll\Watcher
+done

ext/standard/tests/poll/poll_double_construct.phpt

@@ -0,0 +1,28 @@
+--TEST--
+Io\Poll: calling __construct() twice throws instead of leaking
+--FILE--
+<?php
+require_once __DIR__ . '/poll.inc';
+
+list($r, $w) = pt_new_socket_pair();
+
+$handle = new StreamPollHandle($r);
+try {
+    $handle->__construct($r);
+} catch (Error $e) {
+    echo $e->getMessage(), "\n";
+}
+
+$ctx = pt_new_stream_poll();
+try {
+    $ctx->__construct();
+} catch (Error $e) {
+    echo $e->getMessage(), "\n";
+}
+
+echo "done\n";
+?>
+--EXPECT--
+StreamPollHandle object is already constructed
+Io\Poll\Context object is already constructed
+done

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--
+<?php
+require_once __DIR__ . '/poll.inc';
+
+list($r, $w) = pt_new_socket_pair();
+$ctx = pt_new_stream_poll();
+$watcher = $ctx->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

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--
+<?php
+if (!is_dir('/proc/self/fd')) {
+    die("skip requires /proc/self/fd (Linux)\n");
+}
+?>
+--FILE--
+<?php
+function open_fds(): int {
+    return count(scandir('/proc/self/fd'));
+}
+
+$before = open_fds();
+for ($i = 0; $i < 100; $i++) {
+    list($r, $w) = stream_socket_pair(STREAM_PF_UNIX, STREAM_SOCK_STREAM, 0);
+    $h = new StreamPollHandle($r);
+    unset($h, $r, $w);
+}
+gc_collect_cycles();
+$delta = open_fds() - $before;
+
+// Without the fix each handle pins its stream, leaking ~100 fds.
+var_dump($delta < 10);
+?>
+--EXPECT--
+bool(true)

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--
+<?php
+require_once __DIR__ . '/poll.inc';
+
+class Canary {
+    public $ref;
+    public function __destruct() {
+        echo "Canary freed\n";
+    }
+}
+
+list($r, $w) = pt_new_socket_pair();
+$ctx = pt_new_stream_poll();
+$watcher = $ctx->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

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--
+<?php
+require_once __DIR__ . '/poll.inc';
+
+list($r, $w) = pt_new_socket_pair();
+$ctx = pt_new_stream_poll();
+$watcher = $ctx->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

main/poll/php_poll_internal.h

@@ -155,7 +155,9 @@ static inline bool php_poll_is_timeout_error(void)
 
 static inline void php_poll_set_error(php_poll_ctx *ctx, php_poll_error error)
 {
-	ctx->last_error = error;
+	if (ctx) {
+		ctx->last_error = error;
+	}
 }
 
 static inline int php_poll_timespec_to_ms(const struct timespec *timeout)

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;

main/poll/poll_backend_kqueue.c

@@ -382,12 +382,13 @@ static int kqueue_backend_wait(
 
 				if (!found) {
 					/* New FD, create new event */
-					ZEND_ASSERT(unique_events < max_events);
-					events[unique_events].fd = fd;
-					events[unique_events].events = 0;
-					events[unique_events].revents = revents;
-					events[unique_events].data = data;
-					unique_events++;
+					if (unique_events < max_events) {
+						events[unique_events].fd = fd;
+						events[unique_events].events = 0;
+						events[unique_events].revents = revents;
+						events[unique_events].data = data;
+						unique_events++;
+					}
 
 					/* Handle oneshot tracking */
 					if (is_oneshot) {

main/poll/poll_backend_poll.c

@@ -215,8 +215,11 @@ 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;
+	} else if (nfds == 0) {
+		return 0; /* timeout */
 	}
 
 	/* Process results - iterate through struct pollfd array directly */

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);
 	}