Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
iliaal wants to merge 1 commit into
base: PHP-8.4
Choose a base branch
from
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions NEWS
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,9 @@ PHP NEWS

- Curl:
. Fixed curl_reset() retaining CURLOPT_PRIVATE after reset. (lacatoire)
. Fixed a leaked CurlHandle when a CURLMOPT_PUSHFUNCTION callback denies the
push or throws, and a crash using the denied handle if the callback kept
it. (Ilia Alshanetsky)

- MBString:
. Fixed stack overflow in mb_check_encoding() and mb_convert_encoding() with
Expand Down
1 change: 1 addition & 0 deletions ext/curl/curl_private.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,6 +135,7 @@ typedef struct _php_curlsh {
php_curl *init_curl_handle_into_zval(zval *curl);
void init_curl_handle(php_curl *ch);
void _php_curl_cleanup_handle(php_curl *);
void _php_curl_reinit_handle(php_curl *ch);
void _php_curl_multi_cleanup_list(void *data);
void _php_curl_verify_handlers(php_curl *ch, bool reporterror);
void _php_setup_easy_copy_handlers(php_curl *ch, php_curl *source);
Expand Down
81 changes: 63 additions & 18 deletions ext/curl/interface.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1151,9 +1151,35 @@ php_curl *init_curl_handle_into_zval(zval *curl)
return ch;
}

void init_curl_handle(php_curl *ch)
static void curl_init_to_free(php_curl *ch)
{
ch->to_free = ecalloc(1, sizeof(struct _php_curl_free));
zend_llist_init(&ch->to_free->post, sizeof(struct HttpPost *), (llist_dtor_func_t)curl_free_post, 0);
zend_llist_init(&ch->to_free->stream, sizeof(struct mime_data_cb_arg *), (llist_dtor_func_t)curl_free_cb_arg, 0);

ch->to_free->slist = emalloc(sizeof(HashTable));
zend_hash_init(ch->to_free->slist, 4, NULL, curl_free_slist, 0);

ch->clone = emalloc(sizeof(uint32_t));
*ch->clone = 1;
}

static void curl_release_to_free(php_curl *ch)
{
if (--(*ch->clone) == 0) {
zend_llist_clean(&ch->to_free->post);
zend_llist_clean(&ch->to_free->stream);

zend_hash_destroy(ch->to_free->slist);
efree(ch->to_free->slist);
efree(ch->to_free);
efree(ch->clone);
}
}

void init_curl_handle(php_curl *ch)
{
curl_init_to_free(ch);
ch->handlers.write = ecalloc(1, sizeof(php_curl_write));
ch->handlers.write_header = ecalloc(1, sizeof(php_curl_write));
ch->handlers.read = ecalloc(1, sizeof(php_curl_read));
Expand All @@ -1167,16 +1193,9 @@ void init_curl_handle(php_curl *ch)
#if LIBCURL_VERSION_NUM >= 0x075400 /* Available since 7.84.0 */
ch->handlers.sshhostkey = empty_fcall_info_cache;
#endif
ch->clone = emalloc(sizeof(uint32_t));
*ch->clone = 1;

memset(&ch->err, 0, sizeof(struct _php_curl_error));

zend_llist_init(&ch->to_free->post, sizeof(struct HttpPost *), (llist_dtor_func_t)curl_free_post, 0);
zend_llist_init(&ch->to_free->stream, sizeof(struct mime_data_cb_arg *), (llist_dtor_func_t)curl_free_cb_arg, 0);

ch->to_free->slist = emalloc(sizeof(HashTable));
zend_hash_init(ch->to_free->slist, 4, NULL, curl_free_slist, 0);
ZVAL_UNDEF(&ch->postfields);
}

Expand Down Expand Up @@ -2854,7 +2873,7 @@ static void curl_free_obj(zend_object *object)
fprintf(stderr, "DTOR CALLED, ch = %x\n", ch);
#endif

if (!ch->cp) {
if (!ch->clone) {

@bukka bukka Nov 7, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couldn't this result in ch->clone != NULL no longer implying the ch->cp != NULL? If so, is that actually safe?

@iliaal iliaal Nov 7, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but now only for a denied handle that is being released, or one kept when curl_easy_init() fails in the callback. The dtor is safe there: curl_easy_cleanup(NULL) is a no-op and _php_curl_verify_handlers() only passes the NULL handle to curl_easy_setopt(), which rejects it.

/* Can happen if constructor throws. */
zend_object_std_dtor(&ch->std);
return;
Expand All @@ -2865,15 +2884,7 @@ static void curl_free_obj(zend_object *object)
curl_easy_cleanup(ch->cp);

/* cURL destructors should be invoked only by last curl handle */
if (--(*ch->clone) == 0) {
zend_llist_clean(&ch->to_free->post);
zend_llist_clean(&ch->to_free->stream);

zend_hash_destroy(ch->to_free->slist);
efree(ch->to_free->slist);
efree(ch->to_free);
efree(ch->clone);
}
curl_release_to_free(ch);

smart_str_free(&ch->handlers.write->buf);
if (ZEND_FCC_INITIALIZED(ch->handlers.write->fcc)) {
Expand Down Expand Up @@ -3010,6 +3021,40 @@ static void _php_curl_reset_handlers(php_curl *ch)
}
/* }}} */

void _php_curl_reinit_handle(php_curl *ch)
{
ch->cp = curl_easy_init();
if (!ch->cp) {
return;
}

_php_curl_reset_handlers(ch);
if (ZEND_FCC_INITIALIZED(ch->handlers.write->fcc)) {
zend_fcc_dtor(&ch->handlers.write->fcc);
}
if (ZEND_FCC_INITIALIZED(ch->handlers.write_header->fcc)) {
zend_fcc_dtor(&ch->handlers.write_header->fcc);
}
if (ZEND_FCC_INITIALIZED(ch->handlers.read->fcc)) {
zend_fcc_dtor(&ch->handlers.read->fcc);
}
_php_curl_cleanup_handle(ch);

zval_ptr_dtor(&ch->postfields);
ZVAL_UNDEF(&ch->postfields);
zval_ptr_dtor(&ch->private_data);
ZVAL_UNDEF(&ch->private_data);
if (ch->share) {
OBJ_RELEASE(&ch->share->std);
ch->share = NULL;
}

curl_release_to_free(ch);
curl_init_to_free(ch);

_php_curl_set_default_options(ch);
}

/* {{{ Reset all options of a libcurl session handle */
PHP_FUNCTION(curl_reset)
{
Expand Down
21 changes: 13 additions & 8 deletions ext/curl/multi.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -409,15 +409,20 @@ 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;
}

if (ch->share) {
curl_easy_setopt(easy, CURLOPT_SHARE, NULL);
}
ch->cp = NULL;

@bukka bukka Nov 7, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it might be better to re-init object here as the handling between different functions (e.g. curl_reset, curl_setopt, curl_getinfo) seems quite inconsistent

@iliaal iliaal Nov 7, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reworked along those lines: a denied handle the callback keeps now gets a fresh easy handle with curl_init()'s defaults and its own option lists, so curl_reset(), curl_getinfo(), copy and clone behave as on a new handle. A handle the callback drops is freed as before.

if (Z_REFCOUNT(pz_ch) > 1) {
_php_curl_reinit_handle(ch);
}
zval_ptr_dtor(&pz_ch);

return rval;
}
Expand Down
49 changes: 49 additions & 0 deletions ext/curl/tests/curl_pushfunction_deny_leak.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
--TEST--
CURLMOPT_PUSHFUNCTION deny frees 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
$pushed_path = null;
$pushed_ref = null;
$callback = function ($parent_ch, $pushed_ch, array $headers) use (&$pushed_path, &$pushed_ref) {
$pushed_path = preg_grep('/^:path:/', $headers);
$pushed_ref = WeakReference::create($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);

do {
curl_multi_exec($mh, $active);
$info = curl_multi_info_read($mh);
} while ($info === false || $info['msg'] !== CURLMSG_DONE);

var_dump(curl_multi_getcontent($ch));
var_dump(array_values($pushed_path));
var_dump($pushed_ref->get());
?>
--EXPECT--
string(13) "main response"
array(1) {
[0]=>
string(24) ":path:/serverpush/pushed"
}
NULL
69 changes: 69 additions & 0 deletions ext/curl/tests/curl_pushfunction_deny_retain.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,69 @@
--TEST--
CURLMOPT_PUSHFUNCTION denied handle kept by the callback is a fresh 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
$kept = null;
$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_setopt($ch, CURLOPT_PRIVATE, "parent");
curl_multi_add_handle($mh, $ch);

do {
curl_multi_exec($mh, $active);
$info = curl_multi_info_read($mh);
} while ($info === false || $info['msg'] !== CURLMSG_DONE);

curl_multi_remove_handle($mh, $ch);
unset($mh, $ch, $info);

var_dump($kept instanceof CurlHandle);
var_dump(curl_getinfo($kept, CURLINFO_PRIVATE));
var_dump(curl_pause($kept, CURLPAUSE_CONT) === curl_pause(curl_init(), CURLPAUSE_CONT));

var_dump(curl_setopt($kept, CURLOPT_URL, "https://localhost/"));
var_dump(curl_exec($kept));
var_dump(curl_getinfo($kept, CURLINFO_RESPONSE_CODE));

curl_reset($kept);
curl_setopt($kept, CURLOPT_URL, "https://localhost/");
curl_setopt($kept, CURLOPT_RETURNTRANSFER, true);
var_dump(curl_exec($kept));

$copy = curl_copy_handle($kept);
var_dump(curl_exec($copy));

$clone = clone $kept;
var_dump(curl_exec($clone));
?>
--EXPECT--
bool(true)
bool(false)
bool(true)
bool(true)
Caddy is up and runningbool(true)
int(200)
string(23) "Caddy is up and running"
string(23) "Caddy is up and running"
string(23) "Caddy is up and running"
Loading