Skip to content

Fix GH-24731: Assertion failure on bailout in header_register_callback() - #24734

Closed
lazerg wants to merge 4 commits into
php:PHP-8.4from
lazerg:fix/gh-24731-header-callback-dtor
Closed

lazerg wants to merge 4 commits into
php:PHP-8.4from
lazerg:fix/gh-24731-header-callback-dtor

Conversation

@lazerg

@lazerg lazerg commented Oct 26, 2026

Copy link
Copy Markdown
Contributor

header_register_callback() released the previous callback while it was still stored in SG(callback_func). If a destructor run by that release bails out (fatal error, timeout), the slot keeps pointing at the half-freed closure, and the fatal error output goes through sapi_send_headers(), which calls and releases it a second time. The callback is now moved out of the global before it is released, like sapi_send_headers() already does.

Fixes #24731

@lazerg
lazerg requested a review from bukka as a code owner October 26, 2026 09:21
A destructor of the old callback may register a new one, which was then
overwritten and leaked.
@Girgias

Girgias commented Oct 27, 2026

Copy link
Copy Markdown
Member

Is still valid on master?

Also please stop adding a NEWS file. You've been told multiple times to not add it yourself.

@lazerg

lazerg commented Oct 27, 2026 •

Copy link
Copy Markdown
Contributor Author

Yes, master has the same bug: zend_fcc_dtor() releases the closure before it clears SG(send_header_fcc), and the test asserts there too. I removed the NEWS entry, sorry about that.

@Girgias

Girgias commented Oct 27, 2026

Copy link
Copy Markdown
Member

For whenever the zval was converted to only a FCC the fix should be done within zend_fcc_dtor as said in #24764

@lazerg

lazerg commented Oct 27, 2026

Copy link
Copy Markdown
Contributor Author

On 8.4 and 8.5 the callback is still a zval and not an FCC, so a zend_fcc_dtor() fix does not help there (I tried it and the test still asserts). On master I agree: drop the SAPI.c change on merge-up, because the test passes with only the zend_fcc_dtor() fix.

@Girgias Girgias left a comment

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.

@lazerg

lazerg commented Oct 28, 2026 •

Copy link
Copy Markdown
Contributor Author

good, I'll close this and leave it to the general fix.

@lazerg lazerg closed this Oct 28, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants