Fix GH-14638: null dereference after XML parsing failure. #14640
Conversation
devnexen
force-pushed
the
gh14638
branch
2 times, most recently
from
9bbb231
to
b7884e3
Compare
There was a problem hiding this comment.
A couple of things.
First, please reduce tests when you add phpt files, this test reduces to very simply:
<?php
$xml = '<?xml version="1.0" encoding="utf-8" ?>
<test>
</test>';
$root = simplexml_load_string($xml);
try {
$root->__construct("malformed");
} catch (Exception $e) {
// Intentionally empty
}
echo $root;No clone or anything fancy needed.
Also, I don't think you need those extra sxe->document->ptr checks, quite a few more places assume that ptr exists when sxe->document exists.
Finally, even with this fixed, there's still a remaining bug in the constructor in case of a successful parse when manually calling __construct because the old objects (that are reference-count incremented in the constructor code) are not cleaned up by decrementing their refcount.
ext/simplexml/simplexml.c
Outdated
| @@ -2118,7 +2118,7 @@ sxe_object_clone(zend_object *object) | |||
| php_libxml_increment_doc_ref((php_libxml_node_object *)clone, docp); | |||
| } else { | |||
| clone->document = sxe->document; | |||
| if (clone->document) { | |||
| if (clone->document && clone->document->ptr) { | |||
ext/simplexml/simplexml.c
Outdated
| @@ -2378,7 +2378,7 @@ PHP_METHOD(SimpleXMLElement, __construct) | |||
| PHP_LIBXML_RESTORE_GLOBALS(read_file_or_memory); | |||
|
|
|||
| if (!docp) { | |||
| ((php_libxml_node_object *)sxe)->document = NULL; | |||
| php_libxml_decrement_node_ptr((php_libxml_node_object *)sxe); | |||
There was a problem hiding this comment.
I'm not entirely sure that calling __construct on a working SimpleXMLElement (SXE) which then fails should cause the SXE to no longer work. I'd move this decrement code to the happy path below.
Furthermore, there's still leaks here in the success case because the string duplication of nsprefix is not freed, and the fact you don't decrement the document reference.
Also I bet that iteration and xpath data etc should be reset as well on the happy path, so I think you should factor out part of the sxe_object_free_storage function and call that factored out code on the happy path here.
ext/simplexml/simplexml.c
Outdated
| zend_throw_exception(zend_ce_exception, "String could not be parsed as XML", 0); | ||
| RETURN_THROWS(); | ||
| } | ||
|
|
||
| if (!Z_ISUNDEF(sxe->iter.data)) { |
There was a problem hiding this comment.
Can you factor this chunk out to a separate method, such that this code is shared with the freeing code for this class?
| php_libxml_node_decrement_resource((php_libxml_node_object *)sxe); | ||
|
|
||
| if (sxe->xpath) { | ||
| xmlXPathFreeContext(sxe->xpath); |
There was a problem hiding this comment.
You should reset the pointer to NULL otherwise this will lead to a UAF. I think it's easy to test this by calling a method afterwards that accesses ->xpath internally.
ext/simplexml/simplexml.c
Outdated
| zend_throw_exception(zend_ce_exception, "String could not be parsed as XML", 0); | ||
| RETURN_THROWS(); | ||
| } | ||
|
|
||
| sxe_object_free_iterxpath(sxe); | ||
|
|
||
| php_libxml_decrement_node_ptr((php_libxml_node_object *)sxe); |
There was a problem hiding this comment.
Isn't this done already by the sxe_object_free_iterxpath code?
There was a problem hiding this comment.
in the freeing code you release the node, here you decrement it, I think just releasing should be enough.
object document is null if the parsing had failed prior to cast to string.
|
Merged in 2edf12e, closing manually |
object document is null if the parsing had failed prior to cast to string.