Skip to content

Commit 8e614b3

Browse files
nielsdosnielsdos
committed
Fix tree traversal and add some tests
1 parent 57215ac commit 8e614b3

7 files changed

+165
-39
lines changed

‎UPGRADING.INTERNALS

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -130,6 +130,8 @@ PHP 8.3 INTERNALS UPGRADE NOTES
130130
. php_dom_is_cache_tag_stale_from_doc_ptr()
131131
. php_dom_is_cache_tag_stale_from_node()
132132
. php_dom_mark_cache_tag_up_to_date_from_node()
133+
- The function dom_get_elements_by_tag_name_ns_raw() has an additional parameter to indicate
134+
the base node of the node list.
133135

134136
g. ext/libxml
135137
- Two new functions: php_libxml_invalidate_node_list_cache_from_doc() and

‎ext/dom/dom_iterators.c

Lines changed: 17 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -213,23 +213,24 @@ static void php_dom_iterator_move_forward(zend_object_iterator *iter) /* {{{ */
213213
/* The collection is live, we nav the tree from the base object if we cannot
214214
* use the cache to restart from the last point. */
215215
basenode = dom_object_get_node(objmap->baseobj);
216+
if (UNEXPECTED(!basenode)) {
217+
goto err;
218+
}
216219
if (php_dom_is_cache_tag_stale_from_node(&iterator->cache_tag, basenode)) {
220+
php_dom_mark_cache_tag_up_to_date_from_node(&iterator->cache_tag, basenode);
221+
previndex = 0;
217222
if (basenode && (basenode->type == XML_DOCUMENT_NODE ||
218-
basenode->type == XML_HTML_DOCUMENT_NODE)) {
219-
basenode = xmlDocGetRootElement((xmlDoc *) basenode);
220-
} else if (basenode) {
221-
basenode = basenode->children;
223+
basenode->type == XML_HTML_DOCUMENT_NODE)) {
224+
curnode = xmlDocGetRootElement((xmlDoc *) basenode);
222225
} else {
223-
goto err;
226+
curnode = basenode->children;
224227
}
225-
php_dom_mark_cache_tag_up_to_date_from_node(&iterator->cache_tag, basenode);
226-
previndex = 0;
227228
} else {
228229
previndex = iter->index - 1;
229-
basenode = (xmlNodePtr)((php_libxml_node_ptr *)intern->ptr)->node;
230+
curnode = (xmlNodePtr)((php_libxml_node_ptr *)intern->ptr)->node;
230231
}
231232
curnode = dom_get_elements_by_tag_name_ns_raw(
232-
basenode, (char *) objmap->ns, (char *) objmap->local, &previndex, iter->index);
233+
basenode, curnode, (char *) objmap->ns, (char *) objmap->local, &previndex, iter->index);
233234
}
234235
}
235236
} else {
@@ -266,7 +267,7 @@ zend_object_iterator *php_dom_get_iterator(zend_class_entry *ce, zval *object, i
266267
{
267268
dom_object *intern;
268269
dom_nnodemap_object *objmap;
269-
xmlNodePtr nodep, curnode=NULL;
270+
xmlNodePtr curnode=NULL;
270271
int curindex = 0;
271272
HashTable *nodeht;
272273
zval *entry;
@@ -297,24 +298,25 @@ zend_object_iterator *php_dom_get_iterator(zend_class_entry *ce, zval *object, i
297298
ZVAL_COPY(&iterator->curobj, entry);
298299
}
299300
} else {
300-
nodep = (xmlNode *)dom_object_get_node(objmap->baseobj);
301-
if (!nodep) {
301+
xmlNodePtr basep = (xmlNode *)dom_object_get_node(objmap->baseobj);
302+
if (!basep) {
302303
goto err;
303304
}
304305
if (objmap->nodetype == XML_ATTRIBUTE_NODE || objmap->nodetype == XML_ELEMENT_NODE) {
305306
if (objmap->nodetype == XML_ATTRIBUTE_NODE) {
306-
curnode = (xmlNodePtr) nodep->properties;
307+
curnode = (xmlNodePtr) basep->properties;
307308
} else {
308-
curnode = (xmlNodePtr) nodep->children;
309+
curnode = (xmlNodePtr) basep->children;
309310
}
310311
} else {
312+
xmlNodePtr nodep = basep;
311313
if (nodep->type == XML_DOCUMENT_NODE || nodep->type == XML_HTML_DOCUMENT_NODE) {
312314
nodep = xmlDocGetRootElement((xmlDoc *) nodep);
313315
} else {
314316
nodep = nodep->children;
315317
}
316318
curnode = dom_get_elements_by_tag_name_ns_raw(
317-
nodep, (char *) objmap->ns, (char *) objmap->local, &curindex, 0);
319+
basep, nodep, (char *) objmap->ns, (char *) objmap->local, &curindex, 0);
318320
}
319321
}
320322
} else {

‎ext/dom/nodelist.c

Lines changed: 12 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -92,13 +92,14 @@ static int get_nodelist_length(dom_object *obj)
9292
}
9393
}
9494
} else {
95+
xmlNodePtr basep = nodep;
9596
if (nodep->type == XML_DOCUMENT_NODE || nodep->type == XML_HTML_DOCUMENT_NODE) {
9697
nodep = xmlDocGetRootElement((xmlDoc *) nodep);
9798
} else {
9899
nodep = nodep->children;
99100
}
100101
dom_get_elements_by_tag_name_ns_raw(
101-
nodep, (char *) objmap->ns, (char *) objmap->local, &count, -1);
102+
basep, nodep, (char *) objmap->ns, (char *) objmap->local, &count, -1);
102103
}
103104

104105
objmap->cached_length = count;
@@ -149,7 +150,7 @@ PHP_METHOD(DOMNodeList, item)
149150
xmlNodePtr itemnode = NULL;
150151

151152
dom_nnodemap_object *objmap;
152-
xmlNodePtr nodep;
153+
xmlNodePtr basep;
153154
int count = 0;
154155

155156
id = ZEND_THIS;
@@ -177,8 +178,9 @@ PHP_METHOD(DOMNodeList, item)
177178
return;
178179
}
179180
} else if (objmap->baseobj) {
180-
nodep = dom_object_get_node(objmap->baseobj);
181-
if (nodep) {
181+
basep = dom_object_get_node(objmap->baseobj);
182+
if (basep) {
183+
xmlNodePtr nodep = basep;
182184
/* For now we're only able to use cache for forward search.
183185
* TODO: in the future we could extend the logic of the node list such that backwards searches
184186
* are also possible. */
@@ -212,13 +214,13 @@ PHP_METHOD(DOMNodeList, item)
212214
itemnode = nodep;
213215
} else {
214216
if (restart) {
215-
if (nodep->type == XML_DOCUMENT_NODE || nodep->type == XML_HTML_DOCUMENT_NODE) {
216-
nodep = xmlDocGetRootElement((xmlDoc*) nodep);
217+
if (basep->type == XML_DOCUMENT_NODE || basep->type == XML_HTML_DOCUMENT_NODE) {
218+
nodep = xmlDocGetRootElement((xmlDoc*) basep);
217219
} else {
218-
nodep = nodep->children;
220+
nodep = basep->children;
219221
}
220222
}
221-
itemnode = dom_get_elements_by_tag_name_ns_raw(nodep, (char *) objmap->ns, (char *) objmap->local, &count, relative_index);
223+
itemnode = dom_get_elements_by_tag_name_ns_raw(basep, nodep, (char *) objmap->ns, (char *) objmap->local, &count, relative_index);
222224
}
223225
cache_itemnode = true;
224226
}
@@ -238,8 +240,8 @@ PHP_METHOD(DOMNodeList, item)
238240
dom_object *cached_obj = Z_DOMOBJ_P(return_value);
239241
GC_ADDREF(&cached_obj->std);
240242
/* If the tag is stale, all cached data is useless. Otherwise only the cached object is useless. */
241-
if (php_dom_is_cache_tag_stale_from_node(&objmap->cache_tag, nodep)) {
242-
php_dom_mark_cache_tag_up_to_date_from_node(&objmap->cache_tag, nodep);
243+
if (php_dom_is_cache_tag_stale_from_node(&objmap->cache_tag, itemnode)) {
244+
php_dom_mark_cache_tag_up_to_date_from_node(&objmap->cache_tag, itemnode);
243245
reset_objmap_cache(objmap);
244246
} else {
245247
objmap_cache_release_cached_obj(objmap);

‎ext/dom/php_dom.c

Lines changed: 19 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1227,7 +1227,7 @@ bool dom_has_feature(zend_string *feature, zend_string *version)
12271227
}
12281228
/* }}} end dom_has_feature */
12291229

1230-
xmlNode *dom_get_elements_by_tag_name_ns_raw(xmlNodePtr nodep, char *ns, char *local, int *cur, int index) /* {{{ */
1230+
xmlNode *dom_get_elements_by_tag_name_ns_raw(xmlNodePtr basep, xmlNodePtr nodep, char *ns, char *local, int *cur, int index) /* {{{ */
12311231
{
12321232
xmlNodePtr ret = NULL;
12331233
bool local_match_any = local[0] == '*' && local[1] == '\0';
@@ -1248,12 +1248,26 @@ xmlNode *dom_get_elements_by_tag_name_ns_raw(xmlNodePtr nodep, char *ns, char *l
12481248
(*cur)++;
12491249
}
12501250
}
1251-
ret = dom_get_elements_by_tag_name_ns_raw(nodep->children, ns, local, cur, index);
1252-
if (ret != NULL) {
1253-
break;
1251+
1252+
if (nodep->children) {
1253+
nodep = nodep->children;
1254+
continue;
12541255
}
12551256
}
1256-
nodep = nodep->next;
1257+
1258+
if (nodep->next) {
1259+
nodep = nodep->next;
1260+
} else {
1261+
/* Go upwards, until we find a parent node with a next sibling, or until we hit the base. */
1262+
do {
1263+
nodep = nodep->parent;
1264+
ZEND_ASSERT(nodep != NULL);
1265+
if (nodep == basep) {
1266+
return NULL;
1267+
}
1268+
} while (nodep->next == NULL);
1269+
nodep = nodep->next;
1270+
}
12571271
}
12581272
return ret;
12591273
}

‎ext/dom/php_dom.h

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -118,7 +118,7 @@ void dom_set_old_ns(xmlDoc *doc, xmlNs *ns);
118118
void dom_reconcile_ns(xmlDocPtr doc, xmlNodePtr nodep);
119119
xmlNsPtr dom_get_nsdecl(xmlNode *node, xmlChar *localName);
120120
void dom_normalize (xmlNodePtr nodep);
121-
xmlNode *dom_get_elements_by_tag_name_ns_raw(xmlNodePtr nodep, char *ns, char *local, int *cur, int index);
121+
xmlNode *dom_get_elements_by_tag_name_ns_raw(xmlNodePtr basep, xmlNodePtr nodep, char *ns, char *local, int *cur, int index);
122122
void php_dom_create_implementation(zval *retval);
123123
int dom_hierarchy(xmlNodePtr parent, xmlNodePtr child);
124124
bool dom_has_feature(zend_string *feature, zend_string *version);

‎ext/dom/tests/DOMDocument_getElementsByTagName_liveness_tree_walk.phpt

Lines changed: 89 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,89 @@
1+
--TEST--
2+
DOMDocument::getElementsByTagName() liveness tree walk
3+
--EXTENSIONS--
4+
dom
5+
--FILE--
6+
<?php
7+
8+
$doc = new DOMDocument;
9+
$doc->loadXML('<root><container><a><b i="1"/><b i="2"/></a><b i="3"/></container><b i="4"/></root>');
10+
11+
echo "-- On first child, for --\n";
12+
$list = $doc->documentElement->firstChild->getElementsByTagName('b');
13+
var_dump($list->length);
14+
for ($i = 0; $i < $list->length; $i++) {
15+
echo $i, " ", $list->item($i)->getAttribute('i'), "\n";
16+
}
17+
// Try to access one beyond to check if we don't get excess elements
18+
var_dump($list->item($i));
19+
20+
echo "-- On first child, foreach --\n";
21+
foreach ($list as $item) {
22+
echo $item->getAttribute('i'), "\n";
23+
}
24+
25+
echo "-- On document, for --\n";
26+
$list = $doc->getElementsByTagName('b');
27+
var_dump($list->length);
28+
for ($i = 0; $i < $list->length; $i++) {
29+
echo $i, " ", $list->item($i)->getAttribute('i'), "\n";
30+
}
31+
// Try to access one beyond to check if we don't get excess elements
32+
var_dump($list->item($i));
33+
34+
echo "-- On document, foreach --\n";
35+
foreach ($list as $item) {
36+
echo $item->getAttribute('i'), "\n";
37+
}
38+
39+
echo "-- On document, after caching followed by removing --\n";
40+
41+
$list = $doc->documentElement->firstChild->getElementsByTagName('b');
42+
$list->item(0); // Activate item cache
43+
$list->item(0)->remove();
44+
$list->item(0)->remove();
45+
$list->item(0)->remove();
46+
var_dump($list->length);
47+
var_dump($list->item(0));
48+
foreach ($list as $item) {
49+
echo "Should not execute\n";
50+
}
51+
52+
echo "-- On document, clean list after removal --\n";
53+
$list = $doc->documentElement->firstChild->getElementsByTagName('b');
54+
var_dump($list->length);
55+
var_dump($list->item(0));
56+
foreach ($list as $item) {
57+
echo "Should not execute\n";
58+
}
59+
60+
?>
61+
--EXPECT--
62+
-- On first child, for --
63+
int(3)
64+
0 1
65+
1 2
66+
2 3
67+
NULL
68+
-- On first child, foreach --
69+
1
70+
2
71+
3
72+
-- On document, for --
73+
int(4)
74+
0 1
75+
1 2
76+
2 3
77+
3 4
78+
NULL
79+
-- On document, foreach --
80+
1
81+
2
82+
3
83+
4
84+
-- On document, after caching followed by removing --
85+
int(0)
86+
NULL
87+
-- On document, clean list after removal --
88+
int(0)
89+
NULL

‎ext/dom/tests/DOMDocument_liveness_caching_invalidation.phpt

Lines changed: 25 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -5,22 +5,39 @@ dom
55
--FILE--
66
<?php
77
$doc = new DOMDocument;
8-
$root = $doc->createElement( 'root' );
9-
$doc->appendChild( $root );
10-
for ( $i = 0; $i < 5; $i++ ) {
11-
$root->appendChild( $doc->createElement( 'e' ) );
12-
}
8+
$doc->loadXML('<root><e id="1"/><e id="2"/><e id="3"/><e id="4"/><e id="5"/></root>');
9+
$root = $doc->documentElement;
1310

1411
$i = 0;
1512

16-
foreach ( $doc->getElementsByTagName( 'e' ) as $node ) {
13+
echo "-- Overwrite during iteration --\n";
14+
15+
foreach ($doc->getElementsByTagName('e') as $node) {
1716
if ($i++ == 2) {
1817
$root->textContent = 'overwrite';
1918
}
20-
var_dump($node->tagName);
19+
var_dump($node->tagName, $node->getAttribute('id'));
20+
}
21+
22+
echo "-- Empty iteration --\n";
23+
foreach ($doc->getElementsByTagName('e') as $node) {
24+
echo "Should not execute\n";
25+
}
26+
27+
echo "-- After adding an element again --\n";
28+
$root->appendChild(new DOMElement('e'));
29+
foreach ($doc->getElementsByTagName('e') as $node) {
30+
echo "Should execute once\n";
2131
}
2232
?>
23-
--EXPECTF--
33+
--EXPECT--
34+
-- Overwrite during iteration --
2435
string(1) "e"
36+
string(1) "1"
2537
string(1) "e"
38+
string(1) "2"
2639
string(1) "e"
40+
string(1) "3"
41+
-- Empty iteration --
42+
-- After adding an element again --
43+
Should execute once

0 commit comments

Comments
 (0)