Skip to content

Commit 93951cf

Browse files
nielsdosnielsdos
committed
Fix GH-13012: DOMNode::isEqualNode() is incorrect when attribute order is different
Attributes (and namespace declarations) have to be compared in an unordered way. Closes GH-13017.
1 parent fd58f61 commit 93951cf

File tree

4 files changed

+132
-31
lines changed

4 files changed

+132
-31
lines changed

‎NEWS

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,8 @@ PHP NEWS
2222
(nielsdos)
2323
. Fix crash when toggleAttribute() is used without a document. (nielsdos)
2424
. Fix crash in adoptNode with attribute references. (nielsdos)
25+
. Fixed bug GH-13012 (DOMNode::isEqualNode() is incorrect when attribute
26+
order is different). (nielsdos)
2527

2628
- FFI:
2729
. Fixed bug GH-9698 (stream_wrapper_register crashes with FFI\CData).

‎ext/dom/node.c

Lines changed: 56 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -1490,33 +1490,58 @@ static bool php_dom_node_is_equal_node(const xmlNode *this, const xmlNode *other
14901490

14911491
#define PHP_DOM_FUNC_CAT(prefix, suffix) prefix##_##suffix
14921492
/* xmlNode and xmlNs have incompatible struct layouts, i.e. the next field is in a different offset */
1493-
#define PHP_DOM_DEFINE_LIST_EQUALITY_HELPER(type) \
1494-
static size_t PHP_DOM_FUNC_CAT(php_dom_node_count_list_size, type)(const type *node) \
1495-
{ \
1496-
size_t counter = 0; \
1497-
while (node) { \
1498-
counter++; \
1499-
node = node->next; \
1500-
} \
1501-
return counter; \
1502-
} \
1503-
static bool PHP_DOM_FUNC_CAT(php_dom_node_list_equality_check, type)(const type *list1, const type *list2) \
1504-
{ \
1505-
size_t count = PHP_DOM_FUNC_CAT(php_dom_node_count_list_size, type)(list1); \
1506-
if (count != PHP_DOM_FUNC_CAT(php_dom_node_count_list_size, type)(list2)) { \
1507-
return false; \
1508-
} \
1509-
for (size_t i = 0; i < count; i++) { \
1510-
if (!php_dom_node_is_equal_node((const xmlNode *) list1, (const xmlNode *) list2)) { \
1511-
return false; \
1512-
} \
1513-
list1 = list1->next; \
1514-
list2 = list2->next; \
1515-
} \
1516-
return true; \
1517-
}
1518-
PHP_DOM_DEFINE_LIST_EQUALITY_HELPER(xmlNode)
1519-
PHP_DOM_DEFINE_LIST_EQUALITY_HELPER(xmlNs)
1493+
#define PHP_DOM_DEFINE_LIST_COUNTER_HELPER(type) \
1494+
static size_t PHP_DOM_FUNC_CAT(php_dom_node_count_list_size, type)(const type *node) \
1495+
{ \
1496+
size_t counter = 0; \
1497+
while (node) { \
1498+
counter++; \
1499+
node = node->next; \
1500+
} \
1501+
return counter; \
1502+
}
1503+
#define PHP_DOM_DEFINE_LIST_EQUALITY_ORDERED_HELPER(type) \
1504+
static bool PHP_DOM_FUNC_CAT(php_dom_node_list_equality_check_ordered, type)(const type *list1, const type *list2) \
1505+
{ \
1506+
size_t count = PHP_DOM_FUNC_CAT(php_dom_node_count_list_size, type)(list1); \
1507+
if (count != PHP_DOM_FUNC_CAT(php_dom_node_count_list_size, type)(list2)) { \
1508+
return false; \
1509+
} \
1510+
for (size_t i = 0; i < count; i++) { \
1511+
if (!php_dom_node_is_equal_node((const xmlNode *) list1, (const xmlNode *) list2)) { \
1512+
return false; \
1513+
} \
1514+
list1 = list1->next; \
1515+
list2 = list2->next; \
1516+
} \
1517+
return true; \
1518+
}
1519+
#define PHP_DOM_DEFINE_LIST_EQUALITY_UNORDERED_HELPER(type) \
1520+
static bool PHP_DOM_FUNC_CAT(php_dom_node_list_equality_check_unordered, type)(const type *list1, const type *list2)\
1521+
{ \
1522+
size_t count = PHP_DOM_FUNC_CAT(php_dom_node_count_list_size, type)(list1); \
1523+
if (count != PHP_DOM_FUNC_CAT(php_dom_node_count_list_size, type)(list2)) { \
1524+
return false; \
1525+
} \
1526+
for (const type *n1 = list1; n1 != NULL; n1 = n1->next) { \
1527+
bool found = false; \
1528+
for (const type *n2 = list2; n2 != NULL && !found; n2 = n2->next) { \
1529+
if (php_dom_node_is_equal_node((const xmlNode *) n1, (const xmlNode *) n2)) { \
1530+
found = true; \
1531+
} \
1532+
} \
1533+
if (!found) { \
1534+
return false; \
1535+
} \
1536+
} \
1537+
return true; \
1538+
}
1539+
1540+
PHP_DOM_DEFINE_LIST_COUNTER_HELPER(xmlNode)
1541+
PHP_DOM_DEFINE_LIST_COUNTER_HELPER(xmlNs)
1542+
PHP_DOM_DEFINE_LIST_EQUALITY_ORDERED_HELPER(xmlNode)
1543+
PHP_DOM_DEFINE_LIST_EQUALITY_UNORDERED_HELPER(xmlNode)
1544+
PHP_DOM_DEFINE_LIST_EQUALITY_UNORDERED_HELPER(xmlNs)
15201545

15211546
static bool php_dom_node_is_equal_node(const xmlNode *this, const xmlNode *other)
15221547
{
@@ -1535,9 +1560,9 @@ static bool php_dom_node_is_equal_node(const xmlNode *this, const xmlNode *other
15351560
&& php_dom_node_is_ns_prefix_equal(this, other)
15361561
&& php_dom_node_is_ns_uri_equal(this, other)
15371562
/* Check attributes first, then namespace declarations, then children */
1538-
&& php_dom_node_list_equality_check_xmlNode((const xmlNode *) this->properties, (const xmlNode *) other->properties)
1539-
&& php_dom_node_list_equality_check_xmlNs(this->nsDef, other->nsDef)
1540-
&& php_dom_node_list_equality_check_xmlNode(this->children, other->children);
1563+
&& php_dom_node_list_equality_check_unordered_xmlNode((const xmlNode *) this->properties, (const xmlNode *) other->properties)
1564+
&& php_dom_node_list_equality_check_unordered_xmlNs(this->nsDef, other->nsDef)
1565+
&& php_dom_node_list_equality_check_ordered_xmlNode(this->children, other->children);
15411566
} else if (this->type == XML_DTD_NODE) {
15421567
/* Note: in the living spec entity declarations and notations are no longer compared because they're considered obsolete. */
15431568
const xmlDtd *this_dtd = (const xmlDtd *) this;
@@ -1570,7 +1595,7 @@ static bool php_dom_node_is_equal_node(const xmlNode *this, const xmlNode *other
15701595
const xmlNs *other_ns = (const xmlNs *) other;
15711596
return xmlStrEqual(this_ns->prefix, other_ns->prefix) && xmlStrEqual(this_ns->href, other_ns->href);
15721597
} else if (this->type == XML_DOCUMENT_FRAG_NODE || this->type == XML_HTML_DOCUMENT_NODE || this->type == XML_DOCUMENT_NODE) {
1573-
return php_dom_node_list_equality_check_xmlNode(this->children, other->children);
1598+
return php_dom_node_list_equality_check_ordered_xmlNode(this->children, other->children);
15741599
}
15751600

15761601
return false;

‎ext/dom/tests/gh13012.phpt

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,37 @@
1+
--TEST--
2+
GH-13012 (DOMNode::isEqualNode() is incorrect when attribute order is different)
3+
--EXTENSIONS--
4+
dom
5+
--FILE--
6+
<?php
7+
8+
$dom = new DOMDocument;
9+
$dom->loadXML("<root><x a='a' b='b'/><x b='b' a='a'/><x b='b' a='a' c='c'/></root>");
10+
11+
foreach ($dom->getElementsByTagName('x') as $x1) {
12+
foreach ($dom->getElementsByTagName('x') as $x2) {
13+
echo "Comparing ", $dom->saveXML($x1), " with ", $dom->saveXML($x2), "\n";
14+
var_dump($x1->isEqualNode($x2));
15+
}
16+
}
17+
18+
?>
19+
--EXPECT--
20+
Comparing <x a="a" b="b"/> with <x a="a" b="b"/>
21+
bool(true)
22+
Comparing <x a="a" b="b"/> with <x b="b" a="a"/>
23+
bool(true)
24+
Comparing <x a="a" b="b"/> with <x b="b" a="a" c="c"/>
25+
bool(false)
26+
Comparing <x b="b" a="a"/> with <x a="a" b="b"/>
27+
bool(true)
28+
Comparing <x b="b" a="a"/> with <x b="b" a="a"/>
29+
bool(true)
30+
Comparing <x b="b" a="a"/> with <x b="b" a="a" c="c"/>
31+
bool(false)
32+
Comparing <x b="b" a="a" c="c"/> with <x a="a" b="b"/>
33+
bool(false)
34+
Comparing <x b="b" a="a" c="c"/> with <x b="b" a="a"/>
35+
bool(false)
36+
Comparing <x b="b" a="a" c="c"/> with <x b="b" a="a" c="c"/>
37+
bool(true)

‎ext/dom/tests/gh13012_ns.phpt

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,37 @@
1+
--TEST--
2+
GH-13012 (DOMNode::isEqualNode() is incorrect when attribute order is different - ns variation)
3+
--EXTENSIONS--
4+
dom
5+
--FILE--
6+
<?php
7+
8+
$dom = new DOMDocument;
9+
$dom->loadXML("<root><x/><x xmlns:a=\"urn:a\" xmlns:b=\"urn:b\"/><x xmlns:b=\"urn:b\" xmlns:a=\"urn:a\"/></root>");
10+
11+
foreach ($dom->getElementsByTagName('x') as $x1) {
12+
foreach ($dom->getElementsByTagName('x') as $x2) {
13+
echo "Comparing ", $dom->saveXML($x1), " with ", $dom->saveXML($x2), "\n";
14+
var_dump($x1->isEqualNode($x2));
15+
}
16+
}
17+
18+
?>
19+
--EXPECT--
20+
Comparing <x/> with <x/>
21+
bool(true)
22+
Comparing <x/> with <x xmlns:a="urn:a" xmlns:b="urn:b"/>
23+
bool(false)
24+
Comparing <x/> with <x xmlns:b="urn:b" xmlns:a="urn:a"/>
25+
bool(false)
26+
Comparing <x xmlns:a="urn:a" xmlns:b="urn:b"/> with <x/>
27+
bool(false)
28+
Comparing <x xmlns:a="urn:a" xmlns:b="urn:b"/> with <x xmlns:a="urn:a" xmlns:b="urn:b"/>
29+
bool(true)
30+
Comparing <x xmlns:a="urn:a" xmlns:b="urn:b"/> with <x xmlns:b="urn:b" xmlns:a="urn:a"/>
31+
bool(true)
32+
Comparing <x xmlns:b="urn:b" xmlns:a="urn:a"/> with <x/>
33+
bool(false)
34+
Comparing <x xmlns:b="urn:b" xmlns:a="urn:a"/> with <x xmlns:a="urn:a" xmlns:b="urn:b"/>
35+
bool(true)
36+
Comparing <x xmlns:b="urn:b" xmlns:a="urn:a"/> with <x xmlns:b="urn:b" xmlns:a="urn:a"/>
37+
bool(true)

0 commit comments

Comments
 (0)