From 78ccea4e40fac22432e1d2a7161375db8107a44b Mon Sep 17 00:00:00 2001 From: Niels Dossche <7771979+nielsdos@users.noreply.github.com> Date: Wed, 10 Apr 2024 14:35:37 +0200 Subject: [PATCH] Fix GH-13863: Removal during NodeList iteration breaks loop The list is live, so upon cache invalidation we should rewalk the tree to sync the index again with the node list. We keep the legacy behaviour for the old DOM classes. Closes GH-13934. --- ext/dom/dom_iterators.c | 15 +++- ext/dom/php_dom.h | 7 ++ ext/dom/tests/gh13863.phpt | 164 +++++++++++++++++++++++++++++++++++++ 3 files changed, 184 insertions(+), 2 deletions(-) create mode 100644 ext/dom/tests/gh13863.phpt diff --git a/ext/dom/dom_iterators.c b/ext/dom/dom_iterators.c index a6e6493ccb7..ef81ecaaf73 100644 --- a/ext/dom/dom_iterators.c +++ b/ext/dom/dom_iterators.c @@ -188,8 +188,19 @@ static void php_dom_iterator_move_forward(zend_object_iterator *iter) /* {{{ */ } else { if (objmap->nodetype == XML_ATTRIBUTE_NODE || objmap->nodetype == XML_ELEMENT_NODE) { - curnode = (xmlNodePtr)((php_libxml_node_ptr *)intern->ptr)->node; - curnode = curnode->next; + + /* Note: keep legacy behaviour for non-spec mode. */ + if (php_dom_follow_spec_intern(intern) && php_dom_is_cache_tag_stale_from_doc_ptr(&iterator->cache_tag, intern->document)) { + php_dom_mark_cache_tag_up_to_date_from_doc_ref(&iterator->cache_tag, intern->document); + curnode = dom_fetch_first_iteration_item(objmap); + zend_ulong index = 0; + while (curnode != NULL && index++ < iter->index) { + curnode = curnode->next; + } + } else { + curnode = (xmlNodePtr)((php_libxml_node_ptr *)intern->ptr)->node; + curnode = curnode->next; + } } else { /* The collection is live, we nav the tree from the base object if we cannot * use the cache to restart from the last point. */ diff --git a/ext/dom/php_dom.h b/ext/dom/php_dom.h index e64695f07b1..26035832d95 100644 --- a/ext/dom/php_dom.h +++ b/ext/dom/php_dom.h @@ -262,6 +262,13 @@ static zend_always_inline void php_dom_mark_cache_tag_up_to_date_from_node(php_l } } +static zend_always_inline void php_dom_mark_cache_tag_up_to_date_from_doc_ref(php_libxml_cache_tag *cache_tag, php_libxml_ref_obj *document) +{ + ZEND_ASSERT(cache_tag != NULL); + ZEND_ASSERT(document != NULL); + cache_tag->modification_nr = document->cache_tag.modification_nr; +} + static zend_always_inline bool php_dom_follow_spec_node(const xmlNode *node) { ZEND_ASSERT(node != NULL); diff --git a/ext/dom/tests/gh13863.phpt b/ext/dom/tests/gh13863.phpt new file mode 100644 index 00000000000..e9a7058c341 --- /dev/null +++ b/ext/dom/tests/gh13863.phpt @@ -0,0 +1,164 @@ +--TEST-- +GH-13863 (Removal during NodeList iteration breaks loop) +--EXTENSIONS-- +dom +--FILE-- + + + + + + + + EOXML; + + if ($type === DomType::Legacy) { + $doc = new DOMDocument(); + $doc->preserveWhiteSpace = false; + $doc->loadXML($xml); + } else { + $doc = DOM\XMLDocument::createFromString($xml, LIBXML_NOBLANKS); + } + + return $doc; +} + +foreach ([DomType::Legacy, DomType::Modern] as $case) { + $name = match ($case) { + DomType::Legacy => 'Legacy', + DomType::Modern => 'Modern', + }; + + echo "--- $name test remove index 2 at index 2 ---\n"; + + $doc = createTestDoc($case); + + foreach ($doc->documentElement->childNodes as $i => $node) { + var_dump($doc->documentElement->childNodes->item($i) === $node); + var_dump($i, $node->localName); + if ($i === 2) { + $node->remove(); + } + } + + echo $doc->saveXML(), "\n"; + + echo "--- $name test remove index 1 at index 2 ---\n"; + + $doc = createTestDoc($case); + + foreach ($doc->documentElement->childNodes as $i => $node) { + var_dump($doc->documentElement->childNodes->item($i) === $node); + var_dump($i, $node->localName); + if ($i === 2) { + $doc->documentElement->childNodes[1]->remove(); + } + } + + echo $doc->saveXML(), "\n"; + + echo "--- $name test remove all ---\n"; + + $doc = createTestDoc($case); + + foreach ($doc->documentElement->childNodes as $i => $node) { + var_dump($doc->documentElement->childNodes->item($i) === $node); + var_dump($i, $node->localName); + $node->remove(); + } + + echo $doc->saveXML(), "\n"; +} + +?> +--EXPECT-- +--- Legacy test remove index 2 at index 2 --- +bool(true) +int(0) +string(5) "item1" +bool(true) +int(1) +string(5) "item2" +bool(true) +int(2) +string(5) "item3" + + + +--- Legacy test remove index 1 at index 2 --- +bool(true) +int(0) +string(5) "item1" +bool(true) +int(1) +string(5) "item2" +bool(true) +int(2) +string(5) "item3" +bool(false) +int(3) +string(5) "item4" +bool(false) +int(4) +string(5) "item5" + + + +--- Legacy test remove all --- +bool(true) +int(0) +string(5) "item1" + + + +--- Modern test remove index 2 at index 2 --- +bool(true) +int(0) +string(5) "item1" +bool(true) +int(1) +string(5) "item2" +bool(true) +int(2) +string(5) "item3" +bool(true) +int(3) +string(5) "item5" + + +--- Modern test remove index 1 at index 2 --- +bool(true) +int(0) +string(5) "item1" +bool(true) +int(1) +string(5) "item2" +bool(true) +int(2) +string(5) "item3" +bool(true) +int(3) +string(5) "item5" + + +--- Modern test remove all --- +bool(true) +int(0) +string(5) "item1" +bool(true) +int(1) +string(5) "item3" +bool(true) +int(2) +string(5) "item5" + +