From 6614d5e2004eb5db2aac4571c836d67b9409a16f 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.
---
 ext/dom/node.c             | 92 +++++++++++++++++++++++++-------------
 ext/dom/tests/gh16150.phpt | 27 +++++++++++
 ext/dom/tests/gh16152.phpt | 27 +++++++++++
 3 files changed, 116 insertions(+), 30 deletions(-)
 create mode 100644 ext/dom/tests/gh16150.phpt
 create mode 100644 ext/dom/tests/gh16152.phpt

diff --git a/ext/dom/node.c b/ext/dom/node.c
index e061b13e0137b..48e3c5cc2a747 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)
 
 /* }}} */
 
-static xmlNodePtr _php_dom_insert_fragment(xmlNodePtr nodep, xmlNodePtr prevsib, xmlNodePtr nextsib, xmlNodePtr fragment, dom_object *intern, dom_object *childobj) /* {{{ */
+/* 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)
 {
-	xmlNodePtr newchild, node;
+	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;
+		}
+	}
+}
 
-	newchild = fragment->children;
+static xmlNodePtr _php_dom_insert_fragment(xmlNodePtr nodep, xmlNodePtr prevsib, xmlNodePtr nextsib, xmlNodePtr fragment, dom_object *intern, dom_object *childobj) /* {{{ */
+{
+	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 0000000000000..dd1096d6812cc
--- /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--
+<?php
+
+function test($fname) {
+    $e1 = new DOMElement("E1");
+    $e2 = new DOMElement("E2");
+    $e3 = new DOMElement("E3");
+    $doc = new DOMDocument(); // Must be placed here so it is destroyed first
+    $doc->{$fname}($e3);
+    $e2->append($e1);
+    $e3->{$fname}($e2);
+    echo $doc->saveXML();
+}
+
+test('appendChild');
+test('insertBefore');
+
+?>
+--EXPECT--
+<?xml version="1.0"?>
+<E3><E2><E1/></E2></E3>
+<?xml version="1.0"?>
+<E3><E2><E1/></E2></E3>
diff --git a/ext/dom/tests/gh16152.phpt b/ext/dom/tests/gh16152.phpt
new file mode 100644
index 0000000000000..604980b855e2f
--- /dev/null
+++ b/ext/dom/tests/gh16152.phpt
@@ -0,0 +1,27 @@
+--TEST--
+GH-16152 (Memory leak in DOMProcessingInstruction/DOMDocument)
+--EXTENSIONS--
+dom
+--FILE--
+<?php
+
+function test($fname) {
+    $doc = new DOMDocument();
+    $instr = new DOMProcessingInstruction("tr", "r");
+    $frag = new DOMDocumentFragment();
+    $frag2 = new DOMDocumentFragment();
+    $frag2->append($instr);
+    $frag->append($frag2);
+    $doc->{$fname}($frag);
+    echo $doc->saveXML();
+}
+
+test('insertBefore');
+test('appendChild');
+
+?>
+--EXPECT--
+<?xml version="1.0"?>
+<?tr r?>
+<?xml version="1.0"?>
+<?tr r?>