From e49d732a8341c54b6f3b3addb380070983096341 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Tim=20D=C3=BCsterhus?= <tim@tideways-gmbh.com>
Date: Wed, 9 Oct 2024 09:37:13 +0200
Subject: [PATCH] curl: Prevent a CurlMultiHandle from holding onto a
 CurlHandle if `add_handle` fails (#16302)

* curl: Prevent a CurlMultiHandle from holding onto a CurlHandle if `add_handle` fails

As a user I expect `curl_multi_add_handle` to not have any effect if it returns
an error and I specifically do not expect that it would be necessary to call
`curl_multi_remove_handle`.

* NEWS
---
 NEWS                                          |  4 ++
 ext/curl/multi.c                              | 14 ++++--
 .../curl_multi_add_handle_lifecycle.phpt      | 49 +++++++++++++++++++
 3 files changed, 62 insertions(+), 5 deletions(-)
 create mode 100644 ext/curl/tests/curl_multi_add_handle_lifecycle.phpt

diff --git a/NEWS b/NEWS
index ac01709f717..c25f71fa0b4 100644
--- a/NEWS
+++ b/NEWS
@@ -2,6 +2,10 @@ PHP                                                                        NEWS
 |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
 ?? ??? ????, PHP 8.2.26
 
+- Curl:
+  . Fixed bug GH-16302 (CurlMultiHandle holds a reference to CurlHandle if
+    curl_multi_add_handle fails). (timwolla)
+
 - XMLReader:
   . Fixed bug GH-16292 (Segmentation fault in ext/xmlreader/php_xmlreader.c).
     (nielsdos)
diff --git a/ext/curl/multi.c b/ext/curl/multi.c
index 70cc7e03664..41df6bcd7b1 100644
--- a/ext/curl/multi.c
+++ b/ext/curl/multi.c
@@ -97,12 +97,14 @@ PHP_FUNCTION(curl_multi_add_handle)
 
 	_php_curl_cleanup_handle(ch);
 
-	Z_ADDREF_P(z_ch);
-	zend_llist_add_element(&mh->easyh, z_ch);
-
 	error = curl_multi_add_handle(mh->multi, ch->cp);
 	SAVE_CURLM_ERROR(mh, error);
 
+	if (error == CURLM_OK) {
+		Z_ADDREF_P(z_ch);
+		zend_llist_add_element(&mh->easyh, z_ch);
+	}
+
 	RETURN_LONG((zend_long) error);
 }
 /* }}} */
@@ -164,9 +166,11 @@ PHP_FUNCTION(curl_multi_remove_handle)
 	error = curl_multi_remove_handle(mh->multi, ch->cp);
 	SAVE_CURLM_ERROR(mh, error);
 
-	RETVAL_LONG((zend_long) error);
-	zend_llist_del_element(&mh->easyh, z_ch, (int (*)(void *, void *))curl_compare_objects);
+	if (error == CURLM_OK) {
+		zend_llist_del_element(&mh->easyh, z_ch, (int (*)(void *, void *))curl_compare_objects);
+	}
 
+	RETURN_LONG((zend_long) error);
 }
 /* }}} */
 
diff --git a/ext/curl/tests/curl_multi_add_handle_lifecycle.phpt b/ext/curl/tests/curl_multi_add_handle_lifecycle.phpt
new file mode 100644
index 00000000000..3027f0c3315
--- /dev/null
+++ b/ext/curl/tests/curl_multi_add_handle_lifecycle.phpt
@@ -0,0 +1,49 @@
+--TEST--
+curl_multi_add_handle does not hold onto the handle on failure
+--EXTENSIONS--
+curl
+--FILE--
+<?php
+
+class MyClass {
+    public function __destruct() {
+        echo __METHOD__, PHP_EOL;
+    }
+}
+
+$urls = [
+    "file://".__DIR__."/curl_testdata1.txt",
+    "file://".__DIR__."/curl_testdata2.txt",
+];
+
+$mh = curl_multi_init();
+
+$toRemove = [];
+foreach ($urls as $url) {
+    $ch = curl_init($url);
+    curl_setopt($ch, CURLOPT_PRIVATE, new MyClass());
+
+    if (curl_multi_add_handle($mh, $ch) == CURLM_OK) {
+        $toRemove[] = $ch;
+    }
+    if (curl_multi_add_handle($mh, $ch) == CURLM_OK) {
+        $toRemove[] = $ch;
+    }
+
+    unset($ch);
+}
+
+echo "Removing", PHP_EOL;
+foreach ($toRemove as $i => $ch) {
+    curl_multi_remove_handle($mh, $ch);
+    unset($ch);
+    unset($toRemove[$i]);
+}
+echo "Removed", PHP_EOL;
+
+?>
+--EXPECTF--
+Removing
+MyClass::__destruct
+MyClass::__destruct
+Removed