-
Notifications
You must be signed in to change notification settings - Fork 7.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix GH-14638: null dereference after XML parsing failure. #14640
Conversation
9bbb231
to
b7884e3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why was this check added?
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you factor this chunk out to a separate method, such that this code is shared with the freeing code for this class?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure
php_libxml_node_decrement_resource((php_libxml_node_object *)sxe); | ||
|
||
if (sxe->xpath) { | ||
xmlXPathFreeContext(sxe->xpath); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes true I forgot
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't this done already by the sxe_object_free_iterxpath
code?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not think so
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks
Merged in 2edf12e, closing manually |
object document is null if the parsing had failed prior to cast to string.