ext/curl: Free the pushed handle when the push callback denies or throws

NEWS

@@ -10,6 +10,10 @@ PHP                                                                        NEWS
   . Fix GH-21999: GC inconsistency with lazy object, var_dump(), and object
     comparison. (Arnaud)
 
+- Curl:
+  . Fixed a leaked CurlHandle and clone accounting when a CURLMOPT_PUSHFUNCTION
+    callback denies the push or throws. (Ilia Alshanetsky)
+
 - DOM:
   . Fixed use-after-free when re-constructing a DOMXPath whose php:function
     registrations are freed while still reachable from the cycle collector.

ext/curl/interface.c

@@ -2834,7 +2834,7 @@ static void curl_free_obj(zend_object *object)
 	fprintf(stderr, "DTOR CALLED, ch = %x\n", ch);
 #endif
 
-	if (!ch->cp) {
+	if (!ch->clone) {
 		/* Can happen if constructor throws. */
 		zend_object_std_dtor(&ch->std);
 		return;

ext/curl/multi.c

@@ -409,16 +409,15 @@ static int _php_server_push_callback(CURL *parent_ch, CURL *easy, size_t num_hea
 	zend_call_known_fcc(&mh->handlers.server_push, &retval, /* param_count */ 3, call_args, /* named_params */ NULL);
 	zval_ptr_dtor_nogc(&headers);
 
-	if (!Z_ISUNDEF(retval)) {
-		if (CURL_PUSH_DENY != php_curl_get_long(&retval)) {
-		    rval = CURL_PUSH_OK;
-			zend_llist_add_element(&mh->easyh, &pz_ch);
-		} else {
-			/* libcurl will free this easy handle, avoid double free */
-			ch->cp = NULL;
-		}
+	if (!Z_ISUNDEF(retval) && CURL_PUSH_DENY != php_curl_get_long(&retval)) {
+		rval = CURL_PUSH_OK;
+		zend_llist_add_element(&mh->easyh, &pz_ch);
+		return rval;
 	}
 
+	ch->cp = NULL;
+	zval_ptr_dtor(&pz_ch);
+
 	return rval;
 }
 /* }}} */

ext/curl/tests/curl_pushfunction_deny_leak.phpt

@@ -0,0 +1,80 @@
+--TEST--
+CURLMOPT_PUSHFUNCTION deny must not leak the pushed handle
+--EXTENSIONS--
+curl
+--SKIPIF--
+<?php
+include 'skipif-nocaddy.inc';
+
+$curl_version = curl_version();
+if ($curl_version['version_number'] < 0x080100) {
+    exit("skip: test may crash with curl < 8.1.0");
+}
+?>
+--FILE--
+<?php
+$push_count = 0;
+
+function do_request(): void {
+    $callback = function ($parent_ch, $pushed_ch, array $headers) {
+        $GLOBALS['push_count']++;
+        return CURL_PUSH_DENY;
+    };
+
+    $mh = curl_multi_init();
+    curl_multi_setopt($mh, CURLMOPT_PIPELINING, CURLPIPE_MULTIPLEX);
+    curl_multi_setopt($mh, CURLMOPT_PUSHFUNCTION, $callback);
+
+    $ch = curl_init();
+    curl_setopt($ch, CURLOPT_URL, "https://localhost/serverpush");
+    curl_setopt($ch, CURLOPT_HTTP_VERSION, CURL_HTTP_VERSION_2_0);
+    curl_setopt($ch, CURLOPT_RETURNTRANSFER, 1);
+    curl_multi_add_handle($mh, $ch);
+
+    $done = false;
+    do {
+        curl_multi_exec($mh, $active);
+
+        do {
+            $info = curl_multi_info_read($mh);
+            if (false !== $info && $info['msg'] == CURLMSG_DONE) {
+                $handle = $info['handle'];
+                if ($handle !== null) {
+                    curl_multi_remove_handle($mh, $handle);
+                    curl_close($handle);
+                    $done = true;
+                }
+            }
+        } while ($info);
+    } while (!$done);
+
+    curl_multi_close($mh);
+}
+
+$iterations = 200;
+$bytes_per_iteration_limit = 512;
+
+do_request();
+gc_collect_cycles();
+
+$before = memory_get_usage();
+for ($i = 0; $i < $iterations; $i++) {
+    do_request();
+}
+gc_collect_cycles();
+$growth = memory_get_usage() - $before;
+
+if ($push_count < $iterations) {
+    printf("too few pushes handled: %d\n", $push_count);
+} else {
+    echo "all pushes denied\n";
+}
+if ($growth > $iterations * $bytes_per_iteration_limit) {
+    printf("leaked %d bytes (%d bytes/iteration)\n", $growth, (int) ($growth / $iterations));
+} else {
+    echo "no leak detected\n";
+}
+?>
+--EXPECT--
+all pushes denied
+no leak detected

ext/curl/tests/curl_pushfunction_deny_retain.phpt

@@ -0,0 +1,57 @@
+--TEST--
+CURLMOPT_PUSHFUNCTION deny with the pushed handle retained by the callback
+--EXTENSIONS--
+curl
+--SKIPIF--
+<?php
+include 'skipif-nocaddy.inc';
+
+$curl_version = curl_version();
+if ($curl_version['version_number'] < 0x080100) {
+    exit("skip: test may crash with curl < 8.1.0");
+}
+?>
+--FILE--
+<?php
+$kept = [];
+$callback = function ($parent_ch, $pushed_ch, array $headers) use (&$kept) {
+    $kept[] = $pushed_ch;
+    return CURL_PUSH_DENY;
+};
+
+$mh = curl_multi_init();
+curl_multi_setopt($mh, CURLMOPT_PIPELINING, CURLPIPE_MULTIPLEX);
+curl_multi_setopt($mh, CURLMOPT_PUSHFUNCTION, $callback);
+
+$ch = curl_init();
+curl_setopt($ch, CURLOPT_URL, "https://localhost/serverpush");
+curl_setopt($ch, CURLOPT_HTTP_VERSION, CURL_HTTP_VERSION_2_0);
+curl_setopt($ch, CURLOPT_RETURNTRANSFER, 1);
+curl_multi_add_handle($mh, $ch);
+
+$done = false;
+do {
+    curl_multi_exec($mh, $active);
+    do {
+        $info = curl_multi_info_read($mh);
+        if (false !== $info && $info['msg'] == CURLMSG_DONE) {
+            curl_multi_remove_handle($mh, $info['handle']);
+            $done = true;
+        }
+    } while ($info);
+} while (!$done);
+
+curl_multi_close($mh);
+unset($ch, $info);
+gc_collect_cycles();
+
+var_dump(count($kept));
+var_dump(curl_setopt($kept[0], CURLOPT_WRITEFUNCTION, fn($h, $data) => strlen($data)));
+unset($kept);
+gc_collect_cycles();
+echo "Done\n";
+?>
+--EXPECT--
+int(1)
+bool(true)
+Done