curl: Prevent a CurlMultiHandle from holding onto a CurlHandle if add_handle fails (php#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

Author: TimWolla

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)

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

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