Fix O(N) performance of DOMNode::replaceChild() and DOMNode::removeCh…

…ild() Don't loop over all children to determine if the target node really is a child, just trust the parent pointer. Add tests.
php · Sep 14, 2021 · 781e6b4 · 781e6b4
1 parent a827742
commit 781e6b4
Show file tree

Hide file tree

Showing 4 changed files with 86 additions and 47 deletions.
diff --git a/ext/dom/node.c b/ext/dom/node.c
@@ -1000,9 +1000,9 @@ PHP_METHOD(DOMNode, insertBefore)
 PHP_METHOD(DOMNode, replaceChild)
 {
 	zval *id, *newnode, *oldnode;
-	xmlNodePtr children, newchild, oldchild, nodep;
+	xmlNodePtr newchild, oldchild, nodep;
 	dom_object *intern, *newchildobj, *oldchildobj;
-	int foundoldchild = 0, stricterror;
+	int stricterror;
 
 	int ret;
 
@@ -1020,8 +1020,7 @@ PHP_METHOD(DOMNode, replaceChild)
 	DOM_GET_OBJ(newchild, newnode, xmlNodePtr, newchildobj);
 	DOM_GET_OBJ(oldchild, oldnode, xmlNodePtr, oldchildobj);
 
-	children = nodep->children;
-	if (!children) {
+	if (!nodep->children) {
 		RETURN_FALSE;
 	}
 
@@ -1043,42 +1042,32 @@ PHP_METHOD(DOMNode, replaceChild)
 		RETURN_FALSE;
 	}
 
-	/* check for the old child and whether the new child is already a child */
-	while (children) {
-		if (children == oldchild) {
-			foundoldchild = 1;
-			break;
-		}
-		children = children->next;
+	if (oldchild->parent != nodep) {
+		php_dom_throw_error(NOT_FOUND_ERR, stricterror);
+		RETURN_FALSE;
 	}
 
-	if (foundoldchild) {
-		if (newchild->type == XML_DOCUMENT_FRAG_NODE) {
-			xmlNodePtr prevsib, nextsib;
-			prevsib = oldchild->prev;
-			nextsib = oldchild->next;
+	if (newchild->type == XML_DOCUMENT_FRAG_NODE) {
+		xmlNodePtr prevsib, nextsib;
+		prevsib = oldchild->prev;
+		nextsib = oldchild->next;
 
-			xmlUnlinkNode(oldchild);
+		xmlUnlinkNode(oldchild);
 
-			newchild = _php_dom_insert_fragment(nodep, prevsib, nextsib, newchild, intern, newchildobj);
-			if (newchild) {
-				dom_reconcile_ns(nodep->doc, newchild);
-			}
-		} else if (oldchild != newchild) {
-			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);
+		newchild = _php_dom_insert_fragment(nodep, prevsib, nextsib, newchild, intern, newchildobj);
+		if (newchild) {
 			dom_reconcile_ns(nodep->doc, newchild);
 		}
-		DOM_RET_OBJ(oldchild, &ret, intern);
-		return;
-	} else {
-		php_dom_throw_error(NOT_FOUND_ERR, stricterror);
-		RETURN_FALSE;
+	} else if (oldchild != newchild) {
+		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);
 	}
+	DOM_RET_OBJ(oldchild, &ret, intern);
 }
 /* }}} end dom_node_replace_child */
 
@@ -1088,7 +1077,7 @@ PHP_METHOD(DOMNode, replaceChild)
 PHP_METHOD(DOMNode, removeChild)
 {
 	zval *id, *node;
-	xmlNodePtr children, child, nodep;
+	xmlNodePtr child, nodep;
 	dom_object *intern, *childobj;
 	int ret, stricterror;
 
@@ -1113,23 +1102,13 @@ PHP_METHOD(DOMNode, removeChild)
 		RETURN_FALSE;
 	}
 
-	children = nodep->children;
-	if (!children) {
+	if (!nodep->children || child->parent != nodep) {
 		php_dom_throw_error(NOT_FOUND_ERR, stricterror);
 		RETURN_FALSE;
 	}
 
-	while (children) {
-		if (children == child) {
-			xmlUnlinkNode(child);
-			DOM_RET_OBJ(child, &ret, intern);
-			return;
-		}
-		children = children->next;
-	}
-
-	php_dom_throw_error(NOT_FOUND_ERR, stricterror);
-	RETURN_FALSE;
+	xmlUnlinkNode(child);
+	DOM_RET_OBJ(child, &ret, intern);
 }
 /* }}} end dom_node_remove_child */
 

diff --git a/ext/dom/tests/DOMNode_removeChild_error1.phpt b/ext/dom/tests/DOMNode_removeChild_error1.phpt
@@ -0,0 +1,20 @@
+--TEST--
+removeChild() where the node is not a child
+--EXTENSIONS--
+dom
+--FILE--
+<?php
+$document = new DOMDocument();
+$real_parent = $document->createElement('real');
+$parent = $document->createElement('p');
+$child1 = $document->createElement('child1');
+$child2 = $document->createElement('child2');
+$real_parent->appendChild($child1);
+$parent->appendChild($child2);
+try {
+	$parent->removeChild($child1);
+} catch (DOMException $e) {
+	echo "DOMException: " . $e->getMessage();
+}
+--EXPECT--
+DOMException: Not Found Error
diff --git a/ext/dom/tests/DOMNode_replaceChild_error1.phpt b/ext/dom/tests/DOMNode_replaceChild_error1.phpt
@@ -0,0 +1,21 @@
+--TEST--
+replaceChild() where the old node is not a child
+--EXTENSIONS--
+dom
+--FILE--
+<?php
+$document = new DOMDocument();
+$real_parent = $document->createElement('real');
+$parent = $document->createElement('p');
+$child1 = $document->createElement('child1');
+$child2 = $document->createElement('child2');
+$child3 = $document->createElement('child3');
+$real_parent->appendChild($child1);
+$parent->appendChild($child2);
+try {
+	$parent->replaceChild($child3, $child1);
+} catch (DOMException $e) {
+	echo "DOMException: " . $e->getMessage();
+}
+--EXPECT--
+DOMException: Not Found Error
diff --git a/ext/dom/tests/DOMNode_replaceChild_error2.phpt b/ext/dom/tests/DOMNode_replaceChild_error2.phpt
@@ -0,0 +1,19 @@
+--TEST--
+replaceChild() where the new node is a grandparent of the old node 
+--EXTENSIONS--
+dom
+--FILE--
+<?php
+$document = new DOMDocument();
+$a = $document->createElement('a');
+$b = $document->createElement('b');
+$c = $document->createElement('c');
+$a->appendChild($b);
+$b->appendChild($c);
+try {
+	$b->replaceChild($a, $c);
+} catch (DOMException $e) {
+	echo "DOMException: " . $e->getMessage();
+}
+--EXPECT--
+DOMException: Hierarchy Request Error