ext/xml: reject changing handlers while parsing - #23818
Merged
Merged
Conversation
Girgias
approved these changes
Sep 21, 2026
Girgias
left a comment
Girgias
left a comment
Member
There was a problem hiding this comment.
Can you add a test case?
I agree that this is the correct solution.
The SAX handler setters store their callback as a zend_fcall_info_cache on the parser and xml_set_handler() releases the previous one with zend_fcc_dtor(). None of the setters checked parser->isparsing, so calling xml_set_element_handler() (or any xml_set_*_handler()) from inside a running handler freed the fcc that was still executing. For a non-closure [$object, 'method'] handler whose object is only referenced by that fcc, the zend_fcc_dtor() releases the last reference and frees $this while its method is still on the stack, leading to a use-after-free. Guard xml_set_element_handler() and the XML_SET_HANDLER_PHP_FUNCTION setters with the existing isparsing flag and throw "Cannot change handlers while parsing", mirroring the checks already used by xml_parser_set_option() and xml_parse(). The parsed fcc is released on the error path like the other failure branches.
Contributor
Author
|
Sure :) |
jorgsowa
pushed a commit
to jorgsowa/php-src
that referenced
this pull request
Sep 21, 2026
Backport PR php#23818 from master to PHP-8.4. Original commit: 522b1be The SAX handler setters store their callback as a zend_fcall_info_cache on the parser and xml_set_handler() releases the previous one with zend_fcc_dtor(). None of the setters checked parser->isparsing, so calling xml_set_element_handler() (or any xml_set_*_handler()) from inside a running handler freed the fcc that was still executing. For a non-closure [$object, 'method'] handler whose object is only referenced by that fcc, the zend_fcc_dtor() releases the last reference and frees $this while its method is still on the stack, leading to a use-after-free. Guard xml_set_element_handler() and the XML_SET_HANDLER_PHP_FUNCTION setters with the existing isparsing flag and throw "Cannot change handlers while parsing", mirroring the checks already used by xml_parser_set_option() and xml_parse(). The parsed fcc is released on the error path like the other failure branches. Closes php#23818
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The SAX handler setters store their callback as a zend_fcall_info_cache on the parser and xml_set_handler() releases the previous one with zend_fcc_dtor(). None of the setters checked parser->isparsing, so calling xml_set_element_handler() (or any xml_set_*_handler()) from inside a running handler freed the fcc that was still executing. For a non-closure [$object, 'method'] handler whose object is only referenced by that fcc, the zend_fcc_dtor() releases the last reference and frees $this while its method is still on the stack, leading to a use-after-free.
Guard xml_set_element_handler() and the XML_SET_HANDLER_PHP_FUNCTION setters with the existing isparsing flag and throw "Cannot change handlers while parsing", mirroring the checks already used by xml_parser_set_option() and xml_parse(). The parsed fcc is released on the error path like the other failure branches.