Skip to content

Commit 78ccea4

Browse files
nielsdosnielsdos
committed
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.
1 parent 2bf9da6 commit 78ccea4

File tree

3 files changed

+184
-2
lines changed

3 files changed

+184
-2
lines changed

‎ext/dom/dom_iterators.c

Lines changed: 13 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -188,8 +188,19 @@ static void php_dom_iterator_move_forward(zend_object_iterator *iter) /* {{{ */
188188
} else {
189189
if (objmap->nodetype == XML_ATTRIBUTE_NODE ||
190190
objmap->nodetype == XML_ELEMENT_NODE) {
191-
curnode = (xmlNodePtr)((php_libxml_node_ptr *)intern->ptr)->node;
192-
curnode = curnode->next;
191+
192+
/* Note: keep legacy behaviour for non-spec mode. */
193+
if (php_dom_follow_spec_intern(intern) && php_dom_is_cache_tag_stale_from_doc_ptr(&iterator->cache_tag, intern->document)) {
194+
php_dom_mark_cache_tag_up_to_date_from_doc_ref(&iterator->cache_tag, intern->document);
195+
curnode = dom_fetch_first_iteration_item(objmap);
196+
zend_ulong index = 0;
197+
while (curnode != NULL && index++ < iter->index) {
198+
curnode = curnode->next;
199+
}
200+
} else {
201+
curnode = (xmlNodePtr)((php_libxml_node_ptr *)intern->ptr)->node;
202+
curnode = curnode->next;
203+
}
193204
} else {
194205
/* The collection is live, we nav the tree from the base object if we cannot
195206
* use the cache to restart from the last point. */

‎ext/dom/php_dom.h

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -262,6 +262,13 @@ static zend_always_inline void php_dom_mark_cache_tag_up_to_date_from_node(php_l
262262
}
263263
}
264264

265+
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)
266+
{
267+
ZEND_ASSERT(cache_tag != NULL);
268+
ZEND_ASSERT(document != NULL);
269+
cache_tag->modification_nr = document->cache_tag.modification_nr;
270+
}
271+
265272
static zend_always_inline bool php_dom_follow_spec_node(const xmlNode *node)
266273
{
267274
ZEND_ASSERT(node != NULL);

‎ext/dom/tests/gh13863.phpt

Lines changed: 164 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,164 @@
1+
--TEST--
2+
GH-13863 (Removal during NodeList iteration breaks loop)
3+
--EXTENSIONS--
4+
dom
5+
--FILE--
6+
<?php
7+
8+
enum DomType {
9+
case Legacy;
10+
case Modern;
11+
}
12+
13+
function createTestDoc(DomType $type) {
14+
$xml = <<<EOXML
15+
<x>
16+
<item1 />
17+
<item2 />
18+
<item3 />
19+
<item4 />
20+
<item5 />
21+
</x>
22+
EOXML;
23+
24+
if ($type === DomType::Legacy) {
25+
$doc = new DOMDocument();
26+
$doc->preserveWhiteSpace = false;
27+
$doc->loadXML($xml);
28+
} else {
29+
$doc = DOM\XMLDocument::createFromString($xml, LIBXML_NOBLANKS);
30+
}
31+
32+
return $doc;
33+
}
34+
35+
foreach ([DomType::Legacy, DomType::Modern] as $case) {
36+
$name = match ($case) {
37+
DomType::Legacy => 'Legacy',
38+
DomType::Modern => 'Modern',
39+
};
40+
41+
echo "--- $name test remove index 2 at index 2 ---\n";
42+
43+
$doc = createTestDoc($case);
44+
45+
foreach ($doc->documentElement->childNodes as $i => $node) {
46+
var_dump($doc->documentElement->childNodes->item($i) === $node);
47+
var_dump($i, $node->localName);
48+
if ($i === 2) {
49+
$node->remove();
50+
}
51+
}
52+
53+
echo $doc->saveXML(), "\n";
54+
55+
echo "--- $name test remove index 1 at index 2 ---\n";
56+
57+
$doc = createTestDoc($case);
58+
59+
foreach ($doc->documentElement->childNodes as $i => $node) {
60+
var_dump($doc->documentElement->childNodes->item($i) === $node);
61+
var_dump($i, $node->localName);
62+
if ($i === 2) {
63+
$doc->documentElement->childNodes[1]->remove();
64+
}
65+
}
66+
67+
echo $doc->saveXML(), "\n";
68+
69+
echo "--- $name test remove all ---\n";
70+
71+
$doc = createTestDoc($case);
72+
73+
foreach ($doc->documentElement->childNodes as $i => $node) {
74+
var_dump($doc->documentElement->childNodes->item($i) === $node);
75+
var_dump($i, $node->localName);
76+
$node->remove();
77+
}
78+
79+
echo $doc->saveXML(), "\n";
80+
}
81+
82+
?>
83+
--EXPECT--
84+
--- Legacy test remove index 2 at index 2 ---
85+
bool(true)
86+
int(0)
87+
string(5) "item1"
88+
bool(true)
89+
int(1)
90+
string(5) "item2"
91+
bool(true)
92+
int(2)
93+
string(5) "item3"
94+
<?xml version="1.0"?>
95+
<x><item1/><item2/><item4/><item5/></x>
96+
97+
--- Legacy test remove index 1 at index 2 ---
98+
bool(true)
99+
int(0)
100+
string(5) "item1"
101+
bool(true)
102+
int(1)
103+
string(5) "item2"
104+
bool(true)
105+
int(2)
106+
string(5) "item3"
107+
bool(false)
108+
int(3)
109+
string(5) "item4"
110+
bool(false)
111+
int(4)
112+
string(5) "item5"
113+
<?xml version="1.0"?>
114+
<x><item1/><item3/><item4/><item5/></x>
115+
116+
--- Legacy test remove all ---
117+
bool(true)
118+
int(0)
119+
string(5) "item1"
120+
<?xml version="1.0"?>
121+
<x><item2/><item3/><item4/><item5/></x>
122+
123+
--- Modern test remove index 2 at index 2 ---
124+
bool(true)
125+
int(0)
126+
string(5) "item1"
127+
bool(true)
128+
int(1)
129+
string(5) "item2"
130+
bool(true)
131+
int(2)
132+
string(5) "item3"
133+
bool(true)
134+
int(3)
135+
string(5) "item5"
136+
<?xml version="1.0" encoding="UTF-8"?>
137+
<x><item1/><item2/><item4/><item5/></x>
138+
--- Modern test remove index 1 at index 2 ---
139+
bool(true)
140+
int(0)
141+
string(5) "item1"
142+
bool(true)
143+
int(1)
144+
string(5) "item2"
145+
bool(true)
146+
int(2)
147+
string(5) "item3"
148+
bool(true)
149+
int(3)
150+
string(5) "item5"
151+
<?xml version="1.0" encoding="UTF-8"?>
152+
<x><item1/><item3/><item4/><item5/></x>
153+
--- Modern test remove all ---
154+
bool(true)
155+
int(0)
156+
string(5) "item1"
157+
bool(true)
158+
int(1)
159+
string(5) "item3"
160+
bool(true)
161+
int(2)
162+
string(5) "item5"
163+
<?xml version="1.0" encoding="UTF-8"?>
164+
<x><item2/><item4/></x>

0 commit comments

Comments
 (0)