Skip to content

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

Open
iliaal wants to merge 1 commit into
php:PHP-8.4from
iliaal:promote/curl-push-deny-leak
Open

iliaal wants to merge 1 commit into
php:PHP-8.4from
iliaal:promote/curl-push-deny-leak

Conversation

@iliaal

@iliaal iliaal commented Sep 22, 2026 •

Copy link
Copy Markdown
Member

When the callback denies the push, or throws and leaves the return value undefined, libcurl destroys the easy handle itself, but the wrapper kept a stale ch->cp after a throw and never released the CurlHandle it had just created, so each denied push leaked one wrapper and permanently bumped the parent's clone counter. The callback now clears ch->cp and drops its reference, and curl_free_obj() keys its constructor-failure exit on ch->clone, so the handlers are released whenever the last reference goes, including when the callback keeps the handle. The new tests need Caddy, so they only run in the lanes that have it.

@Sjord

Sjord commented Oct 13, 2026

Copy link
Copy Markdown
Contributor

Nice. Does this work correctly if the CURLMOPT_PUSHFUNCTION callback stores one of the handles? The callback adds a reference to the curl handle, but its handlers are still freed, and may be referenced later.

When the callback denies the push, or throws and leaves the return value
undefined, libcurl destroys the easy handle itself. The wrapper kept a stale
ch->cp after a throw, and in both cases never released the CurlHandle it had
just created, so each denied push leaked one wrapper and permanently bumped
the parent's clone counter. Clear ch->cp and drop the callback's reference
instead. curl_free_obj() now keys its early return on ch->clone, which is
unset only when the constructor failed, so a handle whose easy handle libcurl
already freed still releases its handlers when its last reference goes,
including when the callback kept it.
@iliaal
iliaal force-pushed the promote/curl-push-deny-leak branch from 9177fd8 to ec8ddd8 Compare October 13, 2026 11:56
@iliaal

iliaal commented Oct 13, 2026

Copy link
Copy Markdown
Member Author

Ah, it didn't: the kept handle's handlers were freed while it stayed reachable, so a later GC run could read freed memory. The callback now only clears the easy handle and drops its own reference, and curl_free_obj() releases the handlers when the last reference goes. Added a test that keeps the handle. Since its easy handle is gone, curl_reset(), curl_copy_handle() and clone already crash on a kept handle before this PR; I'll send that separately.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants