From 1c68f9bd5d7ac8903dc70f725d62df680916e4d9 Mon Sep 17 00:00:00 2001 From: Niels Dossche <7771979+nielsdos@users.noreply.github.com> Date: Fri, 21 Feb 2025 19:25:35 +0100 Subject: [PATCH 1/3] Fix GH-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. --- ext/dom/document.c | 69 +++++--------------------------------- ext/dom/tests/gh17847.phpt | 52 ++++++++++++++++++++++++++++ 2 files changed, 61 insertions(+), 60 deletions(-) create mode 100644 ext/dom/tests/gh17847.phpt diff --git a/ext/dom/document.c b/ext/dom/document.c index d1437f1f42923..7b99655002319 100644 --- a/ext/dom/document.c +++ b/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) +static void dom_xinclude_strip_fallback_references(xmlDocPtr docp, 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; @@ -1701,24 +1661,13 @@ PHP_METHOD(DOMDocument, xinclude) DOM_GET_OBJ(docp, id, xmlDocPtr, intern); - dom_xinclude_strip_fallback_references((const xmlNode *) docp); + dom_xinclude_strip_fallback_references(docp, (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) { diff --git a/ext/dom/tests/gh17847.phpt b/ext/dom/tests/gh17847.phpt new file mode 100644 index 0000000000000..e983f66efc66d --- /dev/null +++ b/ext/dom/tests/gh17847.phpt @@ -0,0 +1,52 @@ +--TEST-- +GH-17847 (xinclude destroys live node) +--EXTENSIONS-- +dom +--FILE-- +loadXML(<< + + fallback

garbage

 +

garbage

+ + + +

garbage

+ + + +XML); + +$xpath = new DOMXPath($doc); + +$garbage = []; +foreach ($xpath->query('//p') as $entry) + $garbage[] = $entry; + +$doc->xinclude(); + +var_dump($garbage); +?> +--EXPECTF-- +Warning: DOMDocument::xinclude(): I/O warning : failed to load "%sthisisnonexistent"%s + +Warning: DOMDocument::xinclude(): could not load /run/media/niels/MoreData/php-8.3/thisisnonexistent, and no fallback was found in %s on line %d +array(3) { + [0]=> + object(DOMElement)#3 (1) { + ["schemaTypeInfo"]=> + NULL + } + [1]=> + object(DOMElement)#4 (1) { + ["schemaTypeInfo"]=> + NULL + } + [2]=> + object(DOMElement)#5 (1) { + ["schemaTypeInfo"]=> + NULL + } +} From 53c74ac4aa4c733050d0c1dcacea1227b73a38a4 Mon Sep 17 00:00:00 2001 From: Niels Dossche <7771979+nielsdos@users.noreply.github.com> Date: Fri, 21 Feb 2025 19:43:12 +0100 Subject: [PATCH 2/3] drop docp --- ext/dom/document.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/ext/dom/document.c b/ext/dom/document.c index 7b99655002319..0388249766e1f 100644 --- a/ext/dom/document.c +++ b/ext/dom/document.c @@ -1623,7 +1623,7 @@ static void dom_xinclude_strip_references(xmlNodePtr basep) * 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(xmlDocPtr docp, const xmlNode *basep) +static void dom_xinclude_strip_fallback_references(const xmlNode *basep) { xmlNodePtr current = basep->children; @@ -1661,7 +1661,7 @@ PHP_METHOD(DOMDocument, xinclude) DOM_GET_OBJ(docp, id, xmlDocPtr, intern); - dom_xinclude_strip_fallback_references(docp, (const xmlNode *) docp); + dom_xinclude_strip_fallback_references((const xmlNode *) docp); flags |= XML_PARSE_NOXINCNODE; PHP_LIBXML_SANITIZE_GLOBALS(xinclude); From f583670cb607d3cf5d21b06d4c7f79ad68b32865 Mon Sep 17 00:00:00 2001 From: Niels Dossche <7771979+nielsdos@users.noreply.github.com> Date: Fri, 21 Feb 2025 20:04:11 +0100 Subject: [PATCH 3/3] screw this --- ext/dom/tests/gh17847.phpt | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/ext/dom/tests/gh17847.phpt b/ext/dom/tests/gh17847.phpt index e983f66efc66d..5d5df0b3be05f 100644 --- a/ext/dom/tests/gh17847.phpt +++ b/ext/dom/tests/gh17847.phpt @@ -25,14 +25,11 @@ $garbage = []; foreach ($xpath->query('//p') as $entry) $garbage[] = $entry; -$doc->xinclude(); +@$doc->xinclude(); var_dump($garbage); ?> ---EXPECTF-- -Warning: DOMDocument::xinclude(): I/O warning : failed to load "%sthisisnonexistent"%s - -Warning: DOMDocument::xinclude(): could not load /run/media/niels/MoreData/php-8.3/thisisnonexistent, and no fallback was found in %s on line %d +--EXPECT-- array(3) { [0]=> object(DOMElement)#3 (1) {