Skip to content

Fix GH-24668: zend_set_local_var() leaks the previous CV value - #24670

Open
lazerg wants to merge 1 commit into
php:PHP-8.4from
lazerg:fix/gh-24668-set-local-var-dtor
Open

lazerg wants to merge 1 commit into
php:PHP-8.4from
lazerg:fix/gh-24668-set-local-var-dtor

Conversation

@lazerg

@lazerg lazerg commented Oct 25, 2026

Copy link
Copy Markdown
Contributor

When the name matches a compiled variable, zend_set_local_var() overwrote the slot without releasing what was already there, so any refcounted value in that CV leaked. zend_set_local_var_str() and the symbol table path both dtor the old value already, this does the same.

There is no in-tree caller of zend_set_local_var(), so I checked it by temporarily switching the $http_response_header write in the http wrapper over to it: two file_get_contents() calls in one function leak 8 blocks on a debug build before this change and none after.

Fixes #24668

@devnexen

Copy link
Copy Markdown
Member

so literally reporter version ... the commit needs at least Co-Authored-by tag.

@lazerg
lazerg force-pushed the fix/gh-24668-set-local-var-dtor branch from 2a847bb to 9f01e71 Compare October 25, 2026 07:51
@lazerg

lazerg commented Oct 25, 2026

Copy link
Copy Markdown
Contributor Author

Sorry for that, added now

@ndossche

Copy link
Copy Markdown
Member

Can you add a test please, and can you check whether we need to protect the engine by using the zval garbage; trick or a safe_assign function ?

@lazerg
lazerg force-pushed the fix/gh-24668-set-local-var-dtor branch from 9f01e71 to a098ae8 Compare October 25, 2026 11:23
@lazerg
lazerg requested a review from kocsismate as a code owner October 25, 2026 11:23
@lazerg

lazerg commented Oct 25, 2026

Copy link
Copy Markdown
Contributor Author

Added a zend_test helper and a test. It does need the garbage trick: a destructor calling debug_backtrace() could read the freed old value from the CV, and zend_set_local_var_str() had the same problem (reachable through $http_response_header), so both now store the new value before releasing the old one.

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.

3 participants