Skip to content

Commit 9e7117f

Browse files
LamentXU123nicolas-grekas
LamentXU123
and
nicolas-grekas
authored
ext/curl: Preserve callback lifetime without rejecting callback changes (#23863)
Keep callback receivers and closures alive until their calls return, allowing callbacks to replace or clear themselves without freeing an active receiver. This preserves existing usage such as Symfony HttpClient's write callback. Use the helper for all callback entry points and retain the in_callback guard while releasing the temporary references, as destructors may re-enter curl. Add regression tests for replacement, clearing, trampolines, exceptions and destructor re-entry. Co-authored-by: Nicolas Grekas <nicolas.grekas@gmail.com>
1 parent 8723e66 commit 9e7117f

4 files changed

Lines changed: 172 additions & 50 deletions

File tree

‎ext/curl/interface.c‎

Lines changed: 40 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -563,6 +563,34 @@ PHP_MSHUTDOWN_FUNCTION(curl)
563563
}
564564
/* }}} */
565565

566+
static void php_curl_call_callback(
567+
php_curl *ch, zend_fcall_info_cache *fcc, zval *retval, uint32_t argc, zval *argv)
568+
{
569+
/* The callback may replace or clear its own FCC. Keep its objects alive until
570+
* the call returns, without taking ownership of the FCC's trampoline. */
571+
zend_object *object = fcc->object;
572+
zend_object *closure = fcc->closure;
573+
if (object) {
574+
GC_ADDREF(object);
575+
}
576+
if (closure) {
577+
GC_ADDREF(closure);
578+
}
579+
580+
bool was_in_callback = ch->in_callback;
581+
ch->in_callback = true;
582+
zend_call_known_fcc(fcc, retval, argc, argv, NULL);
583+
584+
/* Destructors may also call back into curl, so keep the callback guard set. */
585+
if (object) {
586+
OBJ_RELEASE(object);
587+
}
588+
if (closure) {
589+
OBJ_RELEASE(closure);
590+
}
591+
ch->in_callback = was_in_callback;
592+
}
593+
566594
/* {{{ curl_write */
567595
static size_t curl_write(char *data, size_t size, size_t nmemb, void *ctx)
568596
{
@@ -594,9 +622,7 @@ static size_t curl_write(char *data, size_t size, size_t nmemb, void *ctx)
594622
ZVAL_OBJ(&argv[0], &ch->std);
595623
ZVAL_STRINGL(&argv[1], data, length);
596624

597-
ch->in_callback = true;
598-
zend_call_known_fcc(&write_handler->fcc, &retval, /* param_count */ 2, argv, /* named_params */ NULL);
599-
ch->in_callback = false;
625+
php_curl_call_callback(ch, &write_handler->fcc, &retval, /* argc */ 2, argv);
600626
if (!Z_ISUNDEF(retval)) {
601627
_php_curl_verify_handlers(ch, /* reporterror */ true);
602628
/* TODO Check callback returns an int or something castable to int */
@@ -630,9 +656,7 @@ static int curl_fnmatch(void *ctx, const char *pattern, const char *string)
630656
ZVAL_STRING(&argv[1], pattern);
631657
ZVAL_STRING(&argv[2], string);
632658

633-
ch->in_callback = true;
634-
zend_call_known_fcc(&ch->handlers.fnmatch, &retval, /* param_count */ 3, argv, /* named_params */ NULL);
635-
ch->in_callback = false;
659+
php_curl_call_callback(ch, &ch->handlers.fnmatch, &retval, /* argc */ 3, argv);
636660

637661
if (!Z_ISUNDEF(retval)) {
638662
_php_curl_verify_handlers(ch, /* reporterror */ true);
@@ -670,9 +694,7 @@ static int curl_progress(void *clientp, double dltotal, double dlnow, double ult
670694
ZVAL_LONG(&args[3], (zend_long)ultotal);
671695
ZVAL_LONG(&args[4], (zend_long)ulnow);
672696

673-
ch->in_callback = true;
674-
zend_call_known_fcc(&ch->handlers.progress, &retval, /* param_count */ 5, args, /* named_params */ NULL);
675-
ch->in_callback = false;
697+
php_curl_call_callback(ch, &ch->handlers.progress, &retval, /* argc */ 5, args);
676698

677699
if (!Z_ISUNDEF(retval)) {
678700
_php_curl_verify_handlers(ch, /* reporterror */ true);
@@ -711,9 +733,7 @@ static int curl_xferinfo(void *clientp, curl_off_t dltotal, curl_off_t dlnow, cu
711733
ZVAL_LONG(&argv[3], ultotal);
712734
ZVAL_LONG(&argv[4], ulnow);
713735

714-
ch->in_callback = true;
715-
zend_call_known_fcc(&ch->handlers.xferinfo, &retval, /* param_count */ 5, argv, /* named_params */ NULL);
716-
ch->in_callback = false;
736+
php_curl_call_callback(ch, &ch->handlers.xferinfo, &retval, /* argc */ 5, argv);
717737

718738
if (!Z_ISUNDEF(retval)) {
719739
_php_curl_verify_handlers(ch, /* reporterror */ true);
@@ -756,9 +776,7 @@ static int curl_prereqfunction(void *clientp, char *conn_primary_ip, char *conn_
756776
ZVAL_LONG(&args[3], conn_primary_port);
757777
ZVAL_LONG(&args[4], conn_local_port);
758778

759-
ch->in_callback = true;
760-
zend_call_known_fcc(&ch->handlers.prereq, &retval, /* param_count */ 5, args, /* named_params */ NULL);
761-
ch->in_callback = false;
779+
php_curl_call_callback(ch, &ch->handlers.prereq, &retval, /* argc */ 5, args);
762780

763781
if (!Z_ISUNDEF(retval)) {
764782
_php_curl_verify_handlers(ch, /* reporterror */ true);
@@ -803,9 +821,7 @@ static int curl_ssh_hostkeyfunction(void *clientp, int keytype, const char *key,
803821
ZVAL_STRINGL(&args[2], key, keylen);
804822
ZVAL_LONG(&args[3], keylen);
805823

806-
ch->in_callback = true;
807-
zend_call_known_fcc(&ch->handlers.sshhostkey, &retval, /* param_count */ 4, args, /* named_params */ NULL);
808-
ch->in_callback = false;
824+
php_curl_call_callback(ch, &ch->handlers.sshhostkey, &retval, /* argc */ 4, args);
809825

810826
if (!Z_ISUNDEF(retval)) {
811827
_php_curl_verify_handlers(ch, /* reporterror */ true);
@@ -855,9 +871,7 @@ static size_t curl_read(char *data, size_t size, size_t nmemb, void *ctx)
855871
}
856872
ZVAL_LONG(&argv[2], (int)size * nmemb);
857873

858-
ch->in_callback = true;
859-
zend_call_known_fcc(&read_handler->fcc, &retval, /* param_count */ 3, argv, /* named_params */ NULL);
860-
ch->in_callback = false;
874+
php_curl_call_callback(ch, &read_handler->fcc, &retval, /* argc */ 3, argv);
861875
if (!Z_ISUNDEF(retval)) {
862876
_php_curl_verify_handlers(ch, /* reporterror */ true);
863877
if (Z_TYPE(retval) == IS_STRING) {
@@ -907,9 +921,7 @@ static size_t curl_write_header(char *data, size_t size, size_t nmemb, void *ctx
907921
ZVAL_OBJ(&argv[0], &ch->std);
908922
ZVAL_STRINGL(&argv[1], data, length);
909923

910-
ch->in_callback = true;
911-
zend_call_known_fcc(&write_handler->fcc, &retval, /* param_count */ 2, argv, /* named_params */ NULL);
912-
ch->in_callback = false;
924+
php_curl_call_callback(ch, &write_handler->fcc, &retval, /* argc */ 2, argv);
913925
if (!Z_ISUNDEF(retval)) {
914926
// TODO: Check for valid int type for return value
915927
_php_curl_verify_handlers(ch, /* reporterror */ true);
@@ -963,9 +975,7 @@ static int curl_debug(CURL *handle, curl_infotype type, char *data, size_t size,
963975
ZVAL_LONG(&args[1], type);
964976
ZVAL_STRINGL(&args[2], data, size);
965977

966-
ch->in_callback = true;
967-
zend_call_known_fcc(&ch->handlers.debug, NULL, /* param_count */ 3, args, /* named_params */ NULL);
968-
ch->in_callback = false;
978+
php_curl_call_callback(ch, &ch->handlers.debug, NULL, /* argc */ 3, args);
969979

970980
zval_ptr_dtor(&args[0]);
971981
zval_ptr_dtor(&args[2]);
@@ -1635,14 +1645,8 @@ PHP_FUNCTION(curl_copy_handle)
16351645
}
16361646
/* }}} */
16371647

1638-
static bool php_curl_set_callable_handler(php_curl *ch, zend_fcall_info_cache *const handler_fcc, zval *callable, bool is_array_config, const char *option_name)
1648+
static bool php_curl_set_callable_handler(zend_fcall_info_cache *const handler_fcc, zval *callable, bool is_array_config, const char *option_name)
16391649
{
1640-
/* Replacing a callback would free the fcc that is still executing on the stack. */
1641-
if (ch->in_callback) {
1642-
zend_throw_error(NULL, "%s(): Attempt to set the %s option from a callback", get_active_function_name(), option_name);
1643-
return false;
1644-
}
1645-
16461650
if (ZEND_FCC_INITIALIZED(*handler_fcc)) {
16471651
zend_fcc_dtor(handler_fcc);
16481652
}
@@ -1666,7 +1670,7 @@ static bool php_curl_set_callable_handler(php_curl *ch, zend_fcall_info_cache *c
16661670

16671671
#define HANDLE_CURL_OPTION_CALLABLE_PHP_CURL_USER(curl_ptr, constant_no_function, handler_type, default_method) \
16681672
case constant_no_function##FUNCTION: { \
1669-
bool result = php_curl_set_callable_handler(curl_ptr, &curl_ptr->handlers.handler_type->fcc, zvalue, is_array_config, #constant_no_function "FUNCTION"); \
1673+
bool result = php_curl_set_callable_handler(&curl_ptr->handlers.handler_type->fcc, zvalue, is_array_config, #constant_no_function "FUNCTION"); \
16701674
if (!result) { \
16711675
curl_ptr->handlers.handler_type->method = default_method; \
16721676
return FAILURE; \
@@ -1681,7 +1685,7 @@ static bool php_curl_set_callable_handler(php_curl *ch, zend_fcall_info_cache *c
16811685

16821686
#define HANDLE_CURL_OPTION_CALLABLE(curl_ptr, constant_no_function, handler_fcc, c_callback) \
16831687
case constant_no_function##FUNCTION: { \
1684-
bool result = php_curl_set_callable_handler(curl_ptr, &curl_ptr->handler_fcc, zvalue, is_array_config, #constant_no_function "FUNCTION"); \
1688+
bool result = php_curl_set_callable_handler(&curl_ptr->handler_fcc, zvalue, is_array_config, #constant_no_function "FUNCTION"); \
16851689
if (!result) { \
16861690
return FAILURE; \
16871691
} \

‎ext/curl/tests/curl_callback_lifetime.phpt‎

Lines changed: 65 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,65 @@
1+
--TEST--
2+
GH-23814 (Curl callbacks keep their receiver alive when replacing or clearing themselves)
3+
--EXTENSIONS--
4+
curl
5+
--SKIPIF--
6+
<?php
7+
if (!in_array('file', curl_version()['protocols'], true)) {
8+
die('skip file protocol not supported');
9+
}
10+
?>
11+
--FILE--
12+
<?php
13+
class Callback {
14+
public string $value = 'Still alive';
15+
16+
public function __construct(private string $action, private CurlHandle $handle) {}
17+
18+
public function write(CurlHandle $handle, string $data): int {
19+
if ($this->action === 'clear') {
20+
curl_setopt_array($handle, [CURLOPT_WRITEFUNCTION => null]);
21+
} else {
22+
curl_setopt($handle, CURLOPT_WRITEFUNCTION, static fn($handle, $data) => strlen($data));
23+
}
24+
gc_collect_cycles();
25+
echo $this->value, "\n";
26+
if ($this->action === 'throw') {
27+
throw new Exception('Callback exception');
28+
}
29+
return strlen($data);
30+
}
31+
32+
public function __call(string $name, array $args): int {
33+
return $this->write(...$args);
34+
}
35+
36+
public function __destruct() {
37+
echo "Destroyed\n";
38+
}
39+
}
40+
41+
foreach (['replace' => 'write', 'clear' => 'missing', 'throw' => 'write'] as $action => $method) {
42+
echo "$method / $action\n";
43+
$handle = curl_init('file://' . __FILE__);
44+
curl_setopt($handle, CURLOPT_WRITEFUNCTION, [new Callback($action, $handle), $method]);
45+
try {
46+
var_dump(curl_exec($handle));
47+
} catch (Exception $e) {
48+
echo $e->getMessage(), "\n";
49+
}
50+
unset($handle);
51+
}
52+
?>
53+
--EXPECT--
54+
write / replace
55+
Still alive
56+
Destroyed
57+
bool(true)
58+
missing / clear
59+
Still alive
60+
Destroyed
61+
bool(true)
62+
write / throw
63+
Still alive
64+
Destroyed
65+
Callback exception

‎ext/curl/tests/curl_callback_lifetime_destructor.phpt‎

Lines changed: 52 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,52 @@
1+
--TEST--
2+
GH-23814 (Curl callback receivers are destroyed with the callback guard still set)
3+
--EXTENSIONS--
4+
curl
5+
--SKIPIF--
6+
<?php
7+
if (!in_array('file', curl_version()['protocols'], true)) {
8+
die('skip file protocol not supported');
9+
}
10+
?>
11+
--FILE--
12+
<?php
13+
class Callback {
14+
public function __construct(private CurlHandle $handle) {}
15+
16+
public function write(CurlHandle $handle, string $data): int {
17+
curl_setopt($handle, CURLOPT_WRITEFUNCTION, null);
18+
echo "Callback returning\n";
19+
return strlen($data);
20+
}
21+
22+
public function __destruct() {
23+
foreach (['curl_reset', 'curl_close'] as $function) {
24+
try {
25+
$function($this->handle);
26+
} catch (Error $e) {
27+
echo $e->getMessage(), "\n";
28+
}
29+
}
30+
curl_setopt($this->handle, CURLOPT_WRITEFUNCTION,
31+
static function (CurlHandle $handle, string $data): int {
32+
echo "Callback installed by destructor\n";
33+
return strlen($data);
34+
});
35+
}
36+
}
37+
38+
$handle = curl_init('file://' . __FILE__);
39+
curl_setopt($handle, CURLOPT_WRITEFUNCTION, [new Callback($handle), 'write']);
40+
var_dump(curl_exec($handle));
41+
var_dump(curl_exec($handle));
42+
curl_reset($handle);
43+
echo "Reset outside callback succeeded\n";
44+
?>
45+
--EXPECT--
46+
Callback returning
47+
curl_reset(): Attempt to reset cURL handle from a callback
48+
curl_close(): Attempt to close cURL handle from a callback
49+
bool(true)
50+
Callback installed by destructor
51+
bool(true)
52+
Reset outside callback succeeded

‎ext/curl/tests/curl_setopt_callback_reentrancy.phpt‎

Lines changed: 15 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
--TEST--
2-
GH-23814 (Setting a callback option from within a curl callback is rejected)
2+
GH-23814 / GH-23860 (A curl callback can replace itself)
33
--EXTENSIONS--
44
curl
55
--SKIPIF--
@@ -13,26 +13,27 @@ if (!in_array('file', curl_version()['protocols'], true)) {
1313

1414
$handle = curl_init('file://' . __FILE__);
1515
$callback = static function (CurlHandle $handle, string $data): int {
16-
try {
17-
curl_setopt($handle, CURLOPT_WRITEFUNCTION, static fn($handle, $data) => strlen($data));
18-
} catch (Error $error) {
19-
echo $error->getMessage(), "\n";
20-
}
21-
22-
try {
23-
curl_setopt_array($handle, [CURLOPT_WRITEFUNCTION => null]);
24-
} catch (Error $error) {
25-
echo $error->getMessage(), "\n";
26-
}
16+
echo "Original callback\n";
17+
var_dump(curl_setopt($handle, CURLOPT_WRITEFUNCTION, null));
18+
var_dump(curl_setopt_array($handle, [CURLOPT_WRITEFUNCTION =>
19+
static function (CurlHandle $handle, string $data): int {
20+
echo "Replacement callback\n";
21+
return strlen($data);
22+
},
23+
]));
2724

2825
return strlen($data);
2926
};
3027
curl_setopt($handle, CURLOPT_WRITEFUNCTION, $callback);
3128
var_dump(curl_exec($handle));
29+
var_dump(curl_exec($handle));
3230
var_dump(curl_setopt($handle, CURLOPT_WRITEFUNCTION, null));
3331
?>
3432
--EXPECT--
35-
curl_setopt(): Attempt to set the CURLOPT_WRITEFUNCTION option from a callback
36-
curl_setopt_array(): Attempt to set the CURLOPT_WRITEFUNCTION option from a callback
33+
Original callback
34+
bool(true)
35+
bool(true)
36+
bool(true)
37+
Replacement callback
3738
bool(true)
3839
bool(true)

0 commit comments

Comments
 (0)