Skip to content

Commit

Permalink
Fix GH-11830: ParentNode methods should perform their checks upfront
Browse files Browse the repository at this point in the history 
Closes GH-11887.
  • Loading branch information
@nielsdos
nielsdos committed Aug 7, 2023
1 parent 08c4db7 commit dddd309
Show file tree
Hide file tree
Showing 6 changed files with 330 additions and 70 deletions.
2 changes: 2 additions & 0 deletions NEWS
View file
Expand Up @@ -18,6 +18,8 @@ PHP NEWS
(nielsdos)
. Fix json_encode result on DOMDocument. (nielsdos)
. Fix manually calling __construct() on DOM classes. (nielsdos)
. Fixed bug GH-11830 (ParentNode methods should perform their checks
upfront). (nielsdos)

- FFI:
. Fix leaking definitions when using FFI::cdef()->new(...). (ilutov)
Expand Down
149 changes: 79 additions & 70 deletions ext/dom/parentnode.c
View file
Expand Up @@ -141,107 +141,84 @@ static bool dom_is_node_in_list(const zval *nodes, int nodesc, const xmlNodePtr
return false;
}

static xmlDocPtr dom_doc_from_context_node(xmlNodePtr contextNode)
{
if (contextNode->type == XML_DOCUMENT_NODE || contextNode->type == XML_HTML_DOCUMENT_NODE) {
return (xmlDocPtr) contextNode;
} else {
return contextNode->doc;
}
}

xmlNode* dom_zvals_to_fragment(php_libxml_ref_obj *document, xmlNode *contextNode, zval *nodes, int nodesc)
{
int i;
xmlDoc *documentNode;
xmlNode *fragment;
xmlNode *newNode;
zend_class_entry *ce;
dom_object *newNodeObj;
int stricterror;

if (document == NULL) {
php_dom_throw_error(HIERARCHY_REQUEST_ERR, 1);
return NULL;
}

if (contextNode->type == XML_DOCUMENT_NODE || contextNode->type == XML_HTML_DOCUMENT_NODE) {
documentNode = (xmlDoc *) contextNode;
} else {
documentNode = contextNode->doc;
}
documentNode = dom_doc_from_context_node(contextNode);

fragment = xmlNewDocFragment(documentNode);

if (!fragment) {
return NULL;
}

stricterror = dom_get_strict_error(document);

for (i = 0; i < nodesc; i++) {
if (Z_TYPE(nodes[i]) == IS_OBJECT) {
ce = Z_OBJCE(nodes[i]);
newNodeObj = Z_DOMOBJ_P(&nodes[i]);
newNode = dom_object_get_node(newNodeObj);

if (instanceof_function(ce, dom_node_class_entry)) {
newNodeObj = Z_DOMOBJ_P(&nodes[i]);
newNode = dom_object_get_node(newNodeObj);

if (newNode->doc != documentNode) {
php_dom_throw_error(WRONG_DOCUMENT_ERR, stricterror);
goto err;
}
if (newNode->parent != NULL) {
xmlUnlinkNode(newNode);
}

if (newNode->parent != NULL) {
xmlUnlinkNode(newNode);
}
newNodeObj->document = document;
xmlSetTreeDoc(newNode, documentNode);

newNodeObj->document = document;
xmlSetTreeDoc(newNode, documentNode);
/* Citing from the docs (https://gnome.pages.gitlab.gnome.org/libxml2/devhelp/libxml2-tree.html#xmlAddChild):
* "Add a new node to @parent, at the end of the child (or property) list merging adjacent TEXT nodes (in which case @cur is freed)".
* So we must take a copy if this situation arises to prevent a use-after-free. */
bool will_free = newNode->type == XML_TEXT_NODE && fragment->last && fragment->last->type == XML_TEXT_NODE;
if (will_free) {
newNode = xmlCopyNode(newNode, 1);
}

if (newNode->type == XML_ATTRIBUTE_NODE) {
goto hierarchy_request_err;
if (newNode->type == XML_DOCUMENT_FRAG_NODE) {
/* Unpack document fragment nodes, the behaviour differs for different libxml2 versions. */
newNode = newNode->children;
while (newNode) {
xmlNodePtr next = newNode->next;
xmlUnlinkNode(newNode);
if (!xmlAddChild(fragment, newNode)) {
goto err;
}
newNode = next;
}

/* Citing from the docs (https://gnome.pages.gitlab.gnome.org/libxml2/devhelp/libxml2-tree.html#xmlAddChild):
* "Add a new node to @parent, at the end of the child (or property) list merging adjacent TEXT nodes (in which case @cur is freed)".
* So we must take a copy if this situation arises to prevent a use-after-free. */
bool will_free = newNode->type == XML_TEXT_NODE && fragment->last && fragment->last->type == XML_TEXT_NODE;
} else if (!xmlAddChild(fragment, newNode)) {
if (will_free) {
newNode = xmlCopyNode(newNode, 1);
}

if (newNode->type == XML_DOCUMENT_FRAG_NODE) {
/* Unpack document fragment nodes, the behaviour differs for different libxml2 versions. */
newNode = newNode->children;
while (newNode) {
xmlNodePtr next = newNode->next;
xmlUnlinkNode(newNode);
if (!xmlAddChild(fragment, newNode)) {
goto hierarchy_request_err;
}
newNode = next;
}
} else if (!xmlAddChild(fragment, newNode)) {
if (will_free) {
xmlFreeNode(newNode);
}
goto hierarchy_request_err;
xmlFreeNode(newNode);
}
} else {
zend_argument_type_error(i + 1, "must be of type DOMNode|string, %s given", zend_zval_type_name(&nodes[i]));
goto err;
}
} else if (Z_TYPE(nodes[i]) == IS_STRING) {
} else {
ZEND_ASSERT(Z_TYPE(nodes[i]) == IS_STRING);

newNode = xmlNewDocText(documentNode, (xmlChar *) Z_STRVAL(nodes[i]));

xmlSetTreeDoc(newNode, documentNode);

if (!xmlAddChild(fragment, newNode)) {
xmlFreeNode(newNode);
goto hierarchy_request_err;
goto err;
}
} else {
zend_argument_type_error(i + 1, "must be of type DOMNode|string, %s given", zend_zval_type_name(&nodes[i]));
goto err;
}
}

return fragment;

hierarchy_request_err:
php_dom_throw_error(HIERARCHY_REQUEST_ERR, stricterror);
err:
xmlFreeNode(fragment);
return NULL;
Expand All @@ -264,17 +241,39 @@ static void dom_fragment_assign_parent_node(xmlNodePtr parentNode, xmlNodePtr fr
fragment->last = NULL;
}

static zend_result dom_hierarchy_node_list(xmlNodePtr parentNode, zval *nodes, int nodesc)
static zend_result dom_sanity_check_node_list_for_insertion(php_libxml_ref_obj *document, xmlNodePtr parentNode, zval *nodes, int nodesc)
{
if (document == NULL) {
php_dom_throw_error(HIERARCHY_REQUEST_ERR, 1);
return FAILURE;
}

xmlDocPtr documentNode = dom_doc_from_context_node(parentNode);

for (int i = 0; i < nodesc; i++) {
if (Z_TYPE(nodes[i]) == IS_OBJECT) {
zend_uchar type = Z_TYPE(nodes[i]);
if (type == IS_OBJECT) {
const zend_class_entry *ce = Z_OBJCE(nodes[i]);

if (instanceof_function(ce, dom_node_class_entry)) {
if (dom_hierarchy(parentNode, dom_object_get_node(Z_DOMOBJ_P(nodes + i))) != SUCCESS) {
xmlNodePtr node = dom_object_get_node(Z_DOMOBJ_P(nodes + i));

if (node->doc != documentNode) {
php_dom_throw_error(WRONG_DOCUMENT_ERR, dom_get_strict_error(document));
return FAILURE;
}

if (node->type == XML_ATTRIBUTE_NODE || dom_hierarchy(parentNode, node) != SUCCESS) {
php_dom_throw_error(HIERARCHY_REQUEST_ERR, dom_get_strict_error(document));
return FAILURE;
}
} else {
zend_argument_type_error(i + 1, "must be of type DOMNode|string, %s given", zend_zval_type_name(&nodes[i]));
return FAILURE;
}
} else if (type != IS_STRING) {
zend_argument_type_error(i + 1, "must be of type DOMNode|string, %s given", zend_zval_type_name(&nodes[i]));
return FAILURE;
}
}

Expand All @@ -286,8 +285,7 @@ void dom_parent_node_append(dom_object *context, zval *nodes, int nodesc)
xmlNode *parentNode = dom_object_get_node(context);
xmlNodePtr newchild, prevsib;

if (UNEXPECTED(dom_hierarchy_node_list(parentNode, nodes, nodesc) != SUCCESS)) {
php_dom_throw_error(HIERARCHY_REQUEST_ERR, dom_get_strict_error(context->document));
if (UNEXPECTED(dom_sanity_check_node_list_for_insertion(context->document, parentNode, nodes, nodesc) != SUCCESS)) {
return;
}

Expand Down Expand Up @@ -329,8 +327,7 @@ void dom_parent_node_prepend(dom_object *context, zval *nodes, int nodesc)
return;
}

if (UNEXPECTED(dom_hierarchy_node_list(parentNode, nodes, nodesc) != SUCCESS)) {
php_dom_throw_error(HIERARCHY_REQUEST_ERR, dom_get_strict_error(context->document));
if (UNEXPECTED(dom_sanity_check_node_list_for_insertion(context->document, parentNode, nodes, nodesc) != SUCCESS)) {
return;
}

Expand Down Expand Up @@ -414,6 +411,10 @@ void dom_parent_node_after(dom_object *context, zval *nodes, int nodesc)

doc = prevsib->doc;

if (UNEXPECTED(dom_sanity_check_node_list_for_insertion(context->document, parentNode, nodes, nodesc) != SUCCESS)) {
return;
}

/* Spec step 4: convert nodes into fragment */
fragment = dom_zvals_to_fragment(context->document, parentNode, nodes, nodesc);

Expand Down Expand Up @@ -465,6 +466,10 @@ void dom_parent_node_before(dom_object *context, zval *nodes, int nodesc)

doc = nextsib->doc;

if (UNEXPECTED(dom_sanity_check_node_list_for_insertion(context->document, parentNode, nodes, nodesc) != SUCCESS)) {
return;
}

/* Spec step 4: convert nodes into fragment */
fragment = dom_zvals_to_fragment(context->document, parentNode, nodes, nodesc);

Expand Down Expand Up @@ -555,6 +560,10 @@ void dom_child_replace_with(dom_object *context, zval *nodes, int nodesc)

xmlNodePtr insertion_point = child->next;

if (UNEXPECTED(dom_sanity_check_node_list_for_insertion(context->document, parentNode, nodes, nodesc) != SUCCESS)) {
return;
}

xmlNodePtr fragment = dom_zvals_to_fragment(context->document, parentNode, nodes, nodesc);
if (UNEXPECTED(fragment == NULL)) {
return;
Expand Down
56 changes: 56 additions & 0 deletions ext/dom/tests/gh11830/attribute_variation.phpt
View file
@@ -0,0 +1,56 @@
--TEST--
GH-11830 (ParentNode methods should perform their checks upfront) - attribute variation
--EXTENSIONS--
dom
--FILE--
<?php
$doc = new DOMDocument;
$doc->loadXML(<<<XML
<?xml version="1.0"?>
<container x="foo">
<test/>
</container>
XML);

try {
$doc->documentElement->firstElementChild->prepend($doc->documentElement->attributes[0]);
} catch (\DOMException $e) {
echo $e->getMessage(), "\n";
}

try {
$doc->documentElement->firstElementChild->append($doc->documentElement->attributes[0]);
} catch (\DOMException $e) {
echo $e->getMessage(), "\n";
}

try {
$doc->documentElement->firstElementChild->before($doc->documentElement->attributes[0]);
} catch (\DOMException $e) {
echo $e->getMessage(), "\n";
}

try {
$doc->documentElement->firstElementChild->after($doc->documentElement->attributes[0]);
} catch (\DOMException $e) {
echo $e->getMessage(), "\n";
}

try {
$doc->documentElement->firstElementChild->replaceWith($doc->documentElement->attributes[0]);
} catch (\DOMException $e) {
echo $e->getMessage(), "\n";
}

echo $doc->saveXML();
?>
--EXPECT--
Hierarchy Request Error
Hierarchy Request Error
Hierarchy Request Error
Hierarchy Request Error
Hierarchy Request Error
<?xml version="1.0"?>
<container x="foo">
<test/>
</container>
71 changes: 71 additions & 0 deletions ext/dom/tests/gh11830/document_variation.phpt
View file
@@ -0,0 +1,71 @@
--TEST--
GH-11830 (ParentNode methods should perform their checks upfront) - document variation
--EXTENSIONS--
dom
--FILE--
<?php
$otherDoc = new DOMDocument;
$otherDoc->loadXML(<<<XML
<?xml version="1.0"?>
<other/>
XML);

$otherElement = $otherDoc->documentElement;

$doc = new DOMDocument;
$doc->loadXML(<<<XML
<?xml version="1.0"?>
<container>
<alone/>
<child><testelement/></child>
</container>
XML);

$testElement = $doc->documentElement->firstElementChild->nextElementSibling->firstElementChild;

try {
$doc->documentElement->firstElementChild->prepend($testElement, $otherElement);
} catch (\DOMException $e) {
echo $e->getMessage(), "\n";
}

try {
$doc->documentElement->firstElementChild->append($testElement, $otherElement);
} catch (\DOMException $e) {
echo $e->getMessage(), "\n";
}

try {
$doc->documentElement->firstElementChild->before($testElement, $otherElement);
} catch (\DOMException $e) {
echo $e->getMessage(), "\n";
}

try {
$doc->documentElement->firstElementChild->after($testElement, $otherElement);
} catch (\DOMException $e) {
echo $e->getMessage(), "\n";
}

try {
$doc->documentElement->firstElementChild->replaceWith($testElement, $otherElement);
} catch (\DOMException $e) {
echo $e->getMessage(), "\n";
}

echo $otherDoc->saveXML();
echo $doc->saveXML();
?>
--EXPECT--
Wrong Document Error
Wrong Document Error
Wrong Document Error
Wrong Document Error
Wrong Document Error
<?xml version="1.0"?>
<other/>
<?xml version="1.0"?>
<container>
<alone/>
<child><testelement/></child>
</container>

0 comments on commit dddd309

Please sign in to comment.