Fix Io\Poll memory-safety 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 NULL;
+}
+
+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 NULL;
+}
+
 /* 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/poll_core.c

@@ -191,7 +191,8 @@ PHPAPI php_poll_ctx *php_poll_create_by_name(const char *preferred_backend, uint
 /* Set event capacity hint (optional optimization) */
 PHPAPI zend_result php_poll_set_max_events_hint(php_poll_ctx *ctx, int max_events)
 {
-	if (UNEXPECTED(!ctx || max_events <= 0)) {
+	ZEND_ASSERT(ctx);
+	if (UNEXPECTED(max_events <= 0)) {
 		php_poll_set_error(ctx, PHP_POLL_ERR_INVALID);
 		return FAILURE;
 	}
@@ -243,7 +244,8 @@ PHPAPI void php_poll_destroy(php_poll_ctx *ctx)
 /* Add file descriptor */
 PHPAPI zend_result php_poll_add(php_poll_ctx *ctx, int fd, uint32_t events, void *data)
 {
-	if (UNEXPECTED(!ctx || !ctx->initialized || fd < 0)) {
+	ZEND_ASSERT(ctx);
+	if (UNEXPECTED(!ctx->initialized || fd < 0)) {
 		php_poll_set_error(ctx, PHP_POLL_ERR_INVALID);
 		return FAILURE;
 	}
@@ -259,7 +261,8 @@ PHPAPI zend_result php_poll_add(php_poll_ctx *ctx, int fd, uint32_t events, void
 /* Modify file descriptor */
 PHPAPI zend_result php_poll_modify(php_poll_ctx *ctx, int fd, uint32_t events, void *data)
 {
-	if (UNEXPECTED(!ctx || !ctx->initialized || fd < 0)) {
+	ZEND_ASSERT(ctx);
+	if (UNEXPECTED(!ctx->initialized || fd < 0)) {
 		php_poll_set_error(ctx, PHP_POLL_ERR_INVALID);
 		return FAILURE;
 	}
@@ -275,7 +278,8 @@ PHPAPI zend_result php_poll_modify(php_poll_ctx *ctx, int fd, uint32_t events, v
 /* Remove file descriptor */
 PHPAPI zend_result php_poll_remove(php_poll_ctx *ctx, int fd)
 {
-	if (UNEXPECTED(!ctx || !ctx->initialized || fd < 0)) {
+	ZEND_ASSERT(ctx);
+	if (UNEXPECTED(!ctx->initialized || fd < 0)) {
 		php_poll_set_error(ctx, PHP_POLL_ERR_INVALID);
 		return FAILURE;
 	}
@@ -292,7 +296,8 @@ PHPAPI zend_result php_poll_remove(php_poll_ctx *ctx, int fd)
 PHPAPI int php_poll_wait(php_poll_ctx *ctx, php_poll_event *events, int max_events,
 		const struct timespec *timeout)
 {
-	if (UNEXPECTED(!ctx || !ctx->initialized || !events || max_events <= 0)) {
+	ZEND_ASSERT(ctx);
+	if (UNEXPECTED(!ctx->initialized || !events || max_events <= 0)) {
 		php_poll_set_error(ctx, PHP_POLL_ERR_INVALID);
 		return -1;
 	}