From d4a4d2e7a92481210f7541db6b760928a7509873 Mon Sep 17 00:00:00 2001 From: Niels Dossche <7771979+nielsdos@users.noreply.github.com> Date: Wed, 2 Oct 2024 22:48:16 +0200 Subject: [PATCH] Fix bugs GH-16150 and GH-16152: intern document mismanagement The reference counts of the internal document pointer are mismanaged. In the case of fragments the refcount may be increased too much, while for other cases the document reference may not be applied to all children. This bug existed for a long time and this doesn't reproduce (easily) on 8.2 due to other bugs. Furthermore 8.2 will enter security mode soon, and this change may be too risky. Fixes GH-16150. Fixed GH-16152. Closes GH-16178. --- NEWS | 3 ++ ext/dom/node.c | 92 +++++++++++++++++++++++++------------- ext/dom/tests/gh16150.phpt | 27 +++++++++++ ext/dom/tests/gh16152.phpt | 27 +++++++++++ 4 files changed, 119 insertions(+), 30 deletions(-) create mode 100644 ext/dom/tests/gh16150.phpt create mode 100644 ext/dom/tests/gh16152.phpt diff --git a/NEWS b/NEWS index 8f753045237..d6e268b4b2f 100644 --- a/NEWS +++ b/NEWS @@ -19,6 +19,9 @@ PHP NEWS DOMElement->getAttributeNames()). (nielsdos) . Fixed bug GH-16151 (Assertion failure in ext/dom/parentnode/tree.c). (nielsdos) + . Fixed bug GH-16150 (Use after free in php_dom.c). (nielsdos) + . Fixed bug GH-16152 (Memory leak in DOMProcessingInstruction/DOMDocument). + (nielsdos) - JSON: . Fixed bug GH-15168 (stack overflow in json_encode()). (nielsdos) diff --git a/ext/dom/node.c b/ext/dom/node.c index e061b13e013..48e3c5cc2a7 100644 --- a/ext/dom/node.c +++ b/ext/dom/node.c @@ -820,11 +820,62 @@ int dom_node_text_content_write(dom_object *obj, zval *newval) /* }}} */ +/* Returns true if the node was changed, false otherwise. */ +static bool dom_set_document_ref_obj_single(xmlNodePtr node, xmlDocPtr doc, php_libxml_ref_obj *document) +{ + dom_object *childobj = php_dom_object_get_data(node); + if (childobj && !childobj->document) { + childobj->document = document; + document->refcount++; + return true; + } + return false; +} + +/* TODO: on 8.4 replace the loop with the tree walk helper function. */ +static void dom_set_document_pointers(xmlNodePtr node, xmlDocPtr doc, php_libxml_ref_obj *document) +{ + /* Applies the document to the entire subtree. */ + xmlSetTreeDoc(node, doc); + + if (!dom_set_document_ref_obj_single(node, doc, document)) { + return; + } + + xmlNodePtr base = node; + node = node->children; + while (node != NULL) { + ZEND_ASSERT(node != base); + + if (!dom_set_document_ref_obj_single(node, doc, document)) { + break; + } + + if (node->type == XML_ELEMENT_NODE) { + if (node->children) { + node = node->children; + continue; + } + } + + if (node->next) { + node = node->next; + } else { + /* Go upwards, until we find a parent node with a next sibling, or until we hit the base. */ + do { + node = node->parent; + if (node == base) { + return; + } + } while (node->next == NULL); + node = node->next; + } + } +} + static xmlNodePtr _php_dom_insert_fragment(xmlNodePtr nodep, xmlNodePtr prevsib, xmlNodePtr nextsib, xmlNodePtr fragment, dom_object *intern, dom_object *childobj) /* {{{ */ { - xmlNodePtr newchild, node; - - newchild = fragment->children; + xmlNodePtr newchild = fragment->children; if (newchild) { if (prevsib == NULL) { @@ -840,17 +891,10 @@ static xmlNodePtr _php_dom_insert_fragment(xmlNodePtr nodep, xmlNodePtr prevsib, nextsib->prev = fragment->last; } - node = newchild; + /* Assign parent node pointer */ + xmlNodePtr node = newchild; while (node != NULL) { node->parent = nodep; - if (node->doc != nodep->doc) { - xmlSetTreeDoc(node, nodep->doc); - if (node->_private != NULL) { - childobj = node->_private; - childobj->document = intern->document; - php_libxml_increment_doc_ref((php_libxml_node_object *)childobj, NULL); - } - } if (node == fragment->last) { break; } @@ -930,8 +974,7 @@ PHP_METHOD(DOMNode, insertBefore) } if (child->doc == NULL && parentp->doc != NULL) { - childobj->document = intern->document; - php_libxml_increment_doc_ref((php_libxml_node_object *)childobj, NULL); + dom_set_document_pointers(child, parentp->doc, intern->document); } php_libxml_invalidate_node_list_cache(intern->document); @@ -949,9 +992,6 @@ PHP_METHOD(DOMNode, insertBefore) if (child->type == XML_TEXT_NODE && (refp->type == XML_TEXT_NODE || (refp->prev != NULL && refp->prev->type == XML_TEXT_NODE))) { - if (child->doc == NULL) { - xmlSetTreeDoc(child, parentp->doc); - } new_child = child; new_child->parent = refp->parent; new_child->next = refp; @@ -1003,9 +1043,6 @@ PHP_METHOD(DOMNode, insertBefore) } if (child->type == XML_TEXT_NODE && parentp->last != NULL && parentp->last->type == XML_TEXT_NODE) { child->parent = parentp; - if (child->doc == NULL) { - xmlSetTreeDoc(child, parentp->doc); - } new_child = child; if (parentp->children == NULL) { parentp->children = child; @@ -1111,6 +1148,10 @@ PHP_METHOD(DOMNode, replaceChild) RETURN_FALSE; } + if (newchild->doc == NULL && nodep->doc != NULL) { + dom_set_document_pointers(newchild, nodep->doc, intern->document); + } + if (newchild->type == XML_DOCUMENT_FRAG_NODE) { xmlNodePtr prevsib, nextsib; prevsib = oldchild->prev; @@ -1127,11 +1168,6 @@ PHP_METHOD(DOMNode, replaceChild) xmlDtdPtr intSubset = xmlGetIntSubset(nodep->doc); replacedoctype = (intSubset == (xmlDtd *) oldchild); - if (newchild->doc == NULL && nodep->doc != NULL) { - xmlSetTreeDoc(newchild, nodep->doc); - newchildobj->document = intern->document; - php_libxml_increment_doc_ref((php_libxml_node_object *)newchildobj, NULL); - } xmlReplaceNode(oldchild, newchild); dom_reconcile_ns(nodep->doc, newchild); @@ -1216,8 +1252,7 @@ PHP_METHOD(DOMNode, appendChild) } if (child->doc == NULL && nodep->doc != NULL) { - childobj->document = intern->document; - php_libxml_increment_doc_ref((php_libxml_node_object *)childobj, NULL); + dom_set_document_pointers(child, nodep->doc, intern->document); } if (child->parent != NULL){ @@ -1226,9 +1261,6 @@ PHP_METHOD(DOMNode, appendChild) if (child->type == XML_TEXT_NODE && nodep->last != NULL && nodep->last->type == XML_TEXT_NODE) { child->parent = nodep; - if (child->doc == NULL) { - xmlSetTreeDoc(child, nodep->doc); - } new_child = child; if (nodep->children == NULL) { nodep->children = child; diff --git a/ext/dom/tests/gh16150.phpt b/ext/dom/tests/gh16150.phpt new file mode 100644 index 00000000000..dd1096d6812 --- /dev/null +++ b/ext/dom/tests/gh16150.phpt @@ -0,0 +1,27 @@ +--TEST-- +GH-16150 (Use after free in php_dom.c) +--EXTENSIONS-- +dom +--FILE-- +{$fname}($e3); + $e2->append($e1); + $e3->{$fname}($e2); + echo $doc->saveXML(); +} + +test('appendChild'); +test('insertBefore'); + +?> +--EXPECT-- + + + + diff --git a/ext/dom/tests/gh16152.phpt b/ext/dom/tests/gh16152.phpt new file mode 100644 index 00000000000..604980b855e --- /dev/null +++ b/ext/dom/tests/gh16152.phpt @@ -0,0 +1,27 @@ +--TEST-- +GH-16152 (Memory leak in DOMProcessingInstruction/DOMDocument) +--EXTENSIONS-- +dom +--FILE-- +append($instr); + $frag->append($frag2); + $doc->{$fname}($frag); + echo $doc->saveXML(); +} + +test('insertBefore'); +test('appendChild'); + +?> +--EXPECT-- + + + +