Fix phpGH-17847: xinclude destroys live node

dom_xinclude_strip_fallback_references() now also takes into account
xi:include nodes children. This now subsumes all work done normally by
the old start/end node removal, so we can remove that code and start
using XML_PARSE_NOXINCNODE.

Closes phpGH-17878.

Author: ndossche

NEWS

@@ -16,6 +16,9 @@ PHP                                                                        NEWS
   . Fixed bug GH-17797 (zend_test_compile_string crash on invalid
     script path). (David Carlier)
 
+- DOM:
+  . Fixed bug GH-17847 (xinclude destroys live node). (nielsdos)
+
 - FFI:
   . Fix FFI Parsing of Pointer Declaration Lists. (davnotdev)
 

ext/dom/document.c

@@ -1586,47 +1586,6 @@ PHP_METHOD(DOMDocument, saveXML)
 }
 /* }}} end dom_document_savexml */
 
-static xmlNodePtr php_dom_free_xinclude_node(xmlNodePtr cur) /* {{{ */
-{
-	xmlNodePtr xincnode;
-
-	xincnode = cur;
-	cur = cur->next;
-	xmlUnlinkNode(xincnode);
-	php_libxml_node_free_resource(xincnode);
-
-	return cur;
-}
-/* }}} */
-
-static void php_dom_remove_xinclude_nodes(xmlNodePtr cur) /* {{{ */
-{
-	while(cur) {
-		if (cur->type == XML_XINCLUDE_START) {
-			cur = php_dom_free_xinclude_node(cur);
-
-			/* XML_XINCLUDE_END node will be a sibling of XML_XINCLUDE_START */
-			while(cur && cur->type != XML_XINCLUDE_END) {
-				/* remove xinclude processing nodes from recursive xincludes */
-				if (cur->type == XML_ELEMENT_NODE) {
-					   php_dom_remove_xinclude_nodes(cur->children);
-				}
-				cur = cur->next;
-			}
-
-			if (cur && cur->type == XML_XINCLUDE_END) {
-				cur = php_dom_free_xinclude_node(cur);
-			}
-		} else {
-			if (cur->type == XML_ELEMENT_NODE) {
-				php_dom_remove_xinclude_nodes(cur->children);
-			}
-			cur = cur->next;
-		}
-	}
-}
-/* }}} */
-
 /* Backported from master branch xml_common.h */
 static zend_always_inline xmlNodePtr php_dom_next_in_tree_order(const xmlNode *nodep, const xmlNode *basep)
 {
@@ -1660,17 +1619,19 @@ static void dom_xinclude_strip_references(xmlNodePtr basep)
 	}
 }
 
-/* See GH-14702.
- * We have to remove userland references to xinclude fallback nodes because libxml2 will make clones of these
+/* See GH-14702 and GH-17847.
+ * We have to remove userland references to xinclude nodes because libxml2 will make clones of these
  * and remove the original nodes. If the originals are removed while there are still userland references
  * this will cause memory corruption. */
 static void dom_xinclude_strip_fallback_references(const xmlNode *basep)
 {
 	xmlNodePtr current = basep->children;
 
+	/* TODO: try to improve loop search performance */
 	while (current) {
-		if (current->type == XML_ELEMENT_NODE && current->ns != NULL && current->_private != NULL
-			&& xmlStrEqual(current->name, XINCLUDE_FALLBACK)
+		if (current->type == XML_ELEMENT_NODE
+			&& current->ns != NULL
+			&& xmlStrEqual(current->name, XINCLUDE_NODE)
 			&& (xmlStrEqual(current->ns->href, XINCLUDE_NS) || xmlStrEqual(current->ns->href, XINCLUDE_OLD_NS))) {
 			dom_xinclude_strip_references(current);
 		}
@@ -1684,7 +1645,6 @@ PHP_METHOD(DOMDocument, xinclude)
 {
 	zval *id;
 	xmlDoc *docp;
-	xmlNodePtr root;
 	zend_long flags = 0;
 	int err;
 	dom_object *intern;
@@ -1703,22 +1663,11 @@ PHP_METHOD(DOMDocument, xinclude)
 
 	dom_xinclude_strip_fallback_references((const xmlNode *) docp);
 
+	flags |= XML_PARSE_NOXINCNODE;
 	PHP_LIBXML_SANITIZE_GLOBALS(xinclude);
 	err = xmlXIncludeProcessFlags(docp, (int)flags);
 	PHP_LIBXML_RESTORE_GLOBALS(xinclude);
 
-	/* XML_XINCLUDE_START and XML_XINCLUDE_END nodes need to be removed as these
-	are added via xmlXIncludeProcess to mark beginning and ending of xincluded document
-	but are not wanted in resulting document - must be done even if err as it could fail after
-	having processed some xincludes */
-	root = (xmlNodePtr) docp->children;
-	while(root && root->type != XML_ELEMENT_NODE && root->type != XML_XINCLUDE_START) {
-		root = root->next;
-	}
-	if (root) {
-		php_dom_remove_xinclude_nodes(root);
-	}
-
 	php_libxml_invalidate_node_list_cache(intern->document);
 
 	if (err) {

ext/dom/tests/gh17847.phpt

@@ -0,0 +1,49 @@
+--TEST--
+GH-17847 (xinclude destroys live node)
+--EXTENSIONS--
+dom
+--FILE--
+<?php
+$doc = new DOMDocument();
+$doc->loadXML(<<<XML
+<root>
+    <xi:include href="thisisnonexistent" xmlns:xi="http://www.w3.org/2001/XInclude">
+        <xi:fallback>fallback<p>garbage</p></xi:fallback>
+        <p>garbage</p>
+    </xi:include>
+    <xi:test xmlns:xi="http://www.w3.org/2001/XInclude">
+      <xi:include href="thisisnonexistent">
+        <p>garbage</p>
+      </xi:include>
+    </xi:test>
+</root>
+XML);
+
+$xpath = new DOMXPath($doc);
+
+$garbage = [];
+foreach ($xpath->query('//p') as $entry)
+    $garbage[] = $entry;
+
+@$doc->xinclude();
+
+var_dump($garbage);
+?>
+--EXPECT--
+array(3) {
+  [0]=>
+  object(DOMElement)#3 (1) {
+    ["schemaTypeInfo"]=>
+    NULL
+  }
+  [1]=>
+  object(DOMElement)#4 (1) {
+    ["schemaTypeInfo"]=>
+    NULL
+  }
+  [2]=>
+  object(DOMElement)#5 (1) {
+    ["schemaTypeInfo"]=>
+    NULL
+  }
+}