Skip to content

Fix GH-24488: GC double-frees the SimpleXMLElement properties table - #24494

Closed
evankford wants to merge 1 commit into
php:PHP-8.4from
evankford:fix-gh-24488
Closed

Fix GH-24488: GC double-frees the SimpleXMLElement properties table#24494
evankford wants to merge 1 commit into
php:PHP-8.4from
evankford:fix-gh-24488

Conversation

@evankford

@evankford evankford commented Oct 21, 2026

Copy link
Copy Markdown
Contributor

Fixes #24488.

The explanation below was written with an LLM (Claude) and checked by me.

Since the GH-24050 fix (7b75d71), gc_collect_white() adds the table returned by get_gc to the garbage buffer. sxe_get_gc() returns sxe->properties, and sxe_object_free_storage() releases it with zend_hash_release(). Unlike zend_array_release(), that does not call GC_REMOVE_FROM_BUFFER(), so the table is freed while still linked in the GC buffer, and the GC then reads and frees it again.

sxe->properties is a plain non-persistent zval array, so zend_array_release() is the right release function. SimpleXML is the only bundled extension whose get_gc returns a table it frees itself.

Tested on 8.4.27 built from the release tarball: the new test fails without the patch and passes with it. ext/simplexml/tests, Zend/tests/gc_*.phpt and Zend/tests/gh24050*.phpt all pass (215 passed, 3 skipped).

🤖 Generated with Claude Code

@evankford @claude
Fix phpGH-24488: GC double-frees the SimpleXMLElement properties table
1cf7720 
Since the phpGH-24050 fix, gc_collect_white() adds the table returned by
get_gc to the garbage buffer. sxe_object_free_storage() released
sxe->properties with zend_hash_release(), which does not remove the
table from the GC buffer, so the GC later read and freed it again.
Use zend_array_release(), which goes through zend_array_destroy().

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@evankford
evankford requested a review from devnexen as a code owner October 21, 2026 18:15
@evankford evankford mentioned this pull request Oct 21, 2026
Closed
devnexen
devnexen approved these changes Oct 21, 2026
View reviewed changes

@devnexen devnexen 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.

lgtm

@jogoossens jogoossens mentioned this pull request Oct 22, 2026
Open
@devnexen devnexen closed this in f963484 Oct 22, 2026
This was referenced Oct 22, 2026
Open
Closed
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@devnexen devnexen devnexen approved these changes

Assignees

No one assigned

Labels

Extension: simplexml

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@evankford @devnexen