From fe554cf2b423dbffafa36e5d095ca27cc130bbb3 Mon Sep 17 00:00:00 2001 From: Jordi Kroon Date: Fri, 24 Apr 2026 20:37:54 +0200 Subject: [PATCH 1/5] fix Dom\Notation nodes missing tree connection --- ext/dom/dom_iterators.c | 6 +- ext/dom/obj_map.c | 6 +- ext/dom/php_dom.h | 2 +- ext/dom/tests/modern/xml/DTDNamedNodeMap.phpt | 13 +++-- .../xml/XMLDocument_node_notation_wiring.phpt | 57 +++++++++++++++++++ 5 files changed, 74 insertions(+), 10 deletions(-) create mode 100644 ext/dom/tests/modern/xml/XMLDocument_node_notation_wiring.phpt diff --git a/ext/dom/dom_iterators.c b/ext/dom/dom_iterators.c index b71d188dceee..d1e3368d2d3c 100644 --- a/ext/dom/dom_iterators.c +++ b/ext/dom/dom_iterators.c @@ -54,7 +54,7 @@ static dom_nnodemap_object *php_dom_iterator_get_nnmap(const php_dom_iterator *i return nnmap->ptr; } -xmlNodePtr create_notation(const xmlChar *name, const xmlChar *ExternalID, const xmlChar *SystemID) /* {{{ */ +xmlNodePtr create_notation(xmlDtdPtr parent_dtd, const xmlChar *name, const xmlChar *ExternalID, const xmlChar *SystemID) /* {{{ */ { xmlEntityPtr ret = xmlMalloc(sizeof(xmlEntity)); memset(ret, 0, sizeof(xmlEntity)); @@ -62,6 +62,10 @@ xmlNodePtr create_notation(const xmlChar *name, const xmlChar *ExternalID, const ret->name = xmlStrdup(name); ret->ExternalID = xmlStrdup(ExternalID); ret->SystemID = xmlStrdup(SystemID); + if (parent_dtd != NULL) { + ret->parent = parent_dtd; + ret->doc = parent_dtd->doc; + } return (xmlNodePtr) ret; } /* }}} */ diff --git a/ext/dom/obj_map.c b/ext/dom/obj_map.c index 60f8b28e6cad..eeef8345fc63 100644 --- a/ext/dom/obj_map.c +++ b/ext/dom/obj_map.c @@ -176,7 +176,8 @@ static void dom_map_get_notation_item(dom_nnodemap_object *map, zend_long index, xmlNodePtr node = map->ht ? php_dom_libxml_hash_iter(map->ht, index) : NULL; if (node) { xmlNotation *notation = (xmlNotation *) node; - node = create_notation(notation->name, notation->PublicID, notation->SystemID); + xmlDtdPtr dtd = (xmlDtdPtr) dom_object_get_node(map->baseobj); + node = create_notation(dtd, notation->name, notation->PublicID, notation->SystemID); } dom_ret_node_to_zobj(map, node, return_value); } @@ -504,7 +505,8 @@ static xmlNodePtr dom_map_get_ns_named_item_notation(dom_nnodemap_object *map, c { xmlNotationPtr notation = xmlHashLookup(map->ht, BAD_CAST ZSTR_VAL(named)); if (notation) { - return create_notation(notation->name, notation->PublicID, notation->SystemID); + xmlDtdPtr dtd = (xmlDtdPtr) dom_object_get_node(map->baseobj); + return create_notation(dtd, notation->name, notation->PublicID, notation->SystemID); } return NULL; } diff --git a/ext/dom/php_dom.h b/ext/dom/php_dom.h index bc414adaa2f9..e74cd24d2d06 100644 --- a/ext/dom/php_dom.h +++ b/ext/dom/php_dom.h @@ -125,7 +125,7 @@ int dom_hierarchy(xmlNodePtr parent, xmlNodePtr child); bool dom_has_feature(zend_string *feature, zend_string *version); bool dom_node_is_read_only(const xmlNode *node); bool dom_node_children_valid(const xmlNode *node); -xmlNodePtr create_notation(const xmlChar *name, const xmlChar *ExternalID, const xmlChar *SystemID); +xmlNodePtr create_notation(xmlDtdPtr parent_dtd, const xmlChar *name, const xmlChar *ExternalID, const xmlChar *SystemID); xmlNode *php_dom_libxml_hash_iter(xmlHashTable *ht, int index); zend_object_iterator *php_dom_get_iterator(zend_class_entry *ce, zval *object, int by_ref); void dom_set_doc_classmap(php_libxml_ref_obj *document, zend_class_entry *basece, zend_class_entry *ce); diff --git a/ext/dom/tests/modern/xml/DTDNamedNodeMap.phpt b/ext/dom/tests/modern/xml/DTDNamedNodeMap.phpt index f9bb1f7a996e..4ac15a029e37 100644 --- a/ext/dom/tests/modern/xml/DTDNamedNodeMap.phpt +++ b/ext/dom/tests/modern/xml/DTDNamedNodeMap.phpt @@ -21,7 +21,6 @@ var_dump($doctype); var_dump($doctype->entities["test"]); var_dump($doctype->entities["myimage"]); -// TODO: isConnected returning false is a bug var_dump($doctype->notations["GIF"]); ?> @@ -142,17 +141,19 @@ object(Dom\Entity)#3 (17) { ["textContent"]=> NULL } -object(Dom\Notation)#4 (13) { +object(Dom\Notation)#4 (14) { ["nodeType"]=> int(12) ["nodeName"]=> string(3) "GIF" ["baseURI"]=> - string(11) "about:blank" + string(%d) "%s" ["isConnected"]=> - bool(false) + bool(true) + ["ownerDocument"]=> + string(22) "(object value omitted)" ["parentNode"]=> - NULL + string(22) "(object value omitted)" ["parentElement"]=> NULL ["childNodes"]=> @@ -168,5 +169,5 @@ object(Dom\Notation)#4 (13) { ["nodeValue"]=> NULL ["textContent"]=> - string(0) "" + NULL } diff --git a/ext/dom/tests/modern/xml/XMLDocument_node_notation_wiring.phpt b/ext/dom/tests/modern/xml/XMLDocument_node_notation_wiring.phpt new file mode 100644 index 000000000000..623e169a7792 --- /dev/null +++ b/ext/dom/tests/modern/xml/XMLDocument_node_notation_wiring.phpt @@ -0,0 +1,57 @@ +--TEST-- +Dom\XMLDocument: Dom\Notation nodes are connected to their document and doctype +--EXTENSIONS-- +dom +--FILE-- + + + +]> + +XML; + +$dom = Dom\XMLDocument::createFromString($xml); +$doctype = $dom->doctype; +$notations = $doctype->notations; + +// Make notations deterministic by name, as the order of notations in the map is not guaranteed +foreach (['GIF', 'JPEG', 'HTML'] as $name) { + echo "=== $name ===\n"; + $notation = $notations->getNamedItem($name); + var_dump($notation->nodeName); + var_dump($notation->textContent); + var_dump($notation->nodeValue); + var_dump($notation->isConnected); + var_dump($notation->ownerDocument === $dom); + var_dump($notation->parentNode === $doctype); + var_dump($notation->parentElement); +} +?> +--EXPECT-- +=== GIF === +string(3) "GIF" +NULL +NULL +bool(true) +bool(true) +bool(true) +NULL +=== JPEG === +string(4) "JPEG" +NULL +NULL +bool(true) +bool(true) +bool(true) +NULL +=== HTML === +string(4) "HTML" +NULL +NULL +bool(true) +bool(true) +bool(true) +NULL From 11b3bdd2881960c5a82939b0b8b6e442b9b79a71 Mon Sep 17 00:00:00 2001 From: Jordi Kroon Date: Sat, 25 Apr 2026 00:11:48 +0200 Subject: [PATCH 2/5] free notation node memory during object destruction --- ext/dom/dom_iterators.c | 9 +++++++++ ext/dom/php_dom.c | 8 +++++++- ext/dom/php_dom.h | 1 + 3 files changed, 17 insertions(+), 1 deletion(-) diff --git a/ext/dom/dom_iterators.c b/ext/dom/dom_iterators.c index d1e3368d2d3c..f97a9ec825d5 100644 --- a/ext/dom/dom_iterators.c +++ b/ext/dom/dom_iterators.c @@ -70,6 +70,15 @@ xmlNodePtr create_notation(xmlDtdPtr parent_dtd, const xmlChar *name, const xmlC } /* }}} */ +void dom_free_notation(xmlEntityPtr entity) /* {{{ */ +{ + xmlFree((xmlChar *) entity->name); + xmlFree((xmlChar *) entity->ExternalID); + xmlFree((xmlChar *) entity->SystemID); + xmlFree(entity); +} +/* }}} */ + xmlNodePtr php_dom_libxml_hash_iter(xmlHashTable *ht, int index) { int htsize; diff --git a/ext/dom/php_dom.c b/ext/dom/php_dom.c index b188dea6eb49..afe895617bd9 100644 --- a/ext/dom/php_dom.c +++ b/ext/dom/php_dom.c @@ -1486,7 +1486,13 @@ void dom_objects_free_storage(zend_object *object) if (ptr != NULL && ptr->node != NULL) { xmlNodePtr node = ptr->node; - if (node->type != XML_DOCUMENT_NODE && node->type != XML_HTML_DOCUMENT_NODE) { + if (node->type == XML_NOTATION_NODE) { + int refcount = php_libxml_decrement_node_ptr((php_libxml_node_object *) intern); + php_libxml_decrement_doc_ref((php_libxml_node_object *) intern); + if (refcount == 0) { + dom_free_notation((xmlEntityPtr) node); + } + } else if (node->type != XML_DOCUMENT_NODE && node->type != XML_HTML_DOCUMENT_NODE) { php_libxml_node_decrement_resource((php_libxml_node_object *) intern); } else { php_libxml_decrement_node_ptr((php_libxml_node_object *) intern); diff --git a/ext/dom/php_dom.h b/ext/dom/php_dom.h index e74cd24d2d06..d424b26cc694 100644 --- a/ext/dom/php_dom.h +++ b/ext/dom/php_dom.h @@ -126,6 +126,7 @@ bool dom_has_feature(zend_string *feature, zend_string *version); bool dom_node_is_read_only(const xmlNode *node); bool dom_node_children_valid(const xmlNode *node); xmlNodePtr create_notation(xmlDtdPtr parent_dtd, const xmlChar *name, const xmlChar *ExternalID, const xmlChar *SystemID); +void dom_free_notation(xmlEntityPtr entity); xmlNode *php_dom_libxml_hash_iter(xmlHashTable *ht, int index); zend_object_iterator *php_dom_get_iterator(zend_class_entry *ce, zval *object, int by_ref); void dom_set_doc_classmap(php_libxml_ref_obj *document, zend_class_entry *basece, zend_class_entry *ce); From aab5579d42b5f412f7c1660693bbe387fabd38e2 Mon Sep 17 00:00:00 2001 From: Jordi Kroon Date: Sat, 25 Apr 2026 11:52:13 +0200 Subject: [PATCH 3/5] refcount should be unsigned, and update tests --- ext/dom/php_dom.c | 2 +- .../xml/XMLDocument_node_notation_wiring.phpt | 77 +++++++++++++++---- 2 files changed, 61 insertions(+), 18 deletions(-) diff --git a/ext/dom/php_dom.c b/ext/dom/php_dom.c index afe895617bd9..ec1a2ea1c6cf 100644 --- a/ext/dom/php_dom.c +++ b/ext/dom/php_dom.c @@ -1487,7 +1487,7 @@ void dom_objects_free_storage(zend_object *object) xmlNodePtr node = ptr->node; if (node->type == XML_NOTATION_NODE) { - int refcount = php_libxml_decrement_node_ptr((php_libxml_node_object *) intern); + unsigned int refcount = php_libxml_decrement_node_ptr((php_libxml_node_object *) intern); php_libxml_decrement_doc_ref((php_libxml_node_object *) intern); if (refcount == 0) { dom_free_notation((xmlEntityPtr) node); diff --git a/ext/dom/tests/modern/xml/XMLDocument_node_notation_wiring.phpt b/ext/dom/tests/modern/xml/XMLDocument_node_notation_wiring.phpt index 623e169a7792..a3502bcddf53 100644 --- a/ext/dom/tests/modern/xml/XMLDocument_node_notation_wiring.phpt +++ b/ext/dom/tests/modern/xml/XMLDocument_node_notation_wiring.phpt @@ -4,30 +4,49 @@ Dom\XMLDocument: Dom\Notation nodes are connected to their document and doctype dom --FILE-- '', + 'JPEG' => '', + 'HTML' => '', +]; + +foreach ($cases as $name => $declaration) { + $xml = << - - + $declaration ]> XML; -$dom = Dom\XMLDocument::createFromString($xml); -$doctype = $dom->doctype; -$notations = $doctype->notations; + $dom = Dom\XMLDocument::createFromString($xml); + $doctype = $dom->doctype; + $notations = $doctype->notations; -// Make notations deterministic by name, as the order of notations in the map is not guaranteed -foreach (['GIF', 'JPEG', 'HTML'] as $name) { echo "=== $name ===\n"; - $notation = $notations->getNamedItem($name); - var_dump($notation->nodeName); - var_dump($notation->textContent); - var_dump($notation->nodeValue); - var_dump($notation->isConnected); - var_dump($notation->ownerDocument === $dom); - var_dump($notation->parentNode === $doctype); - var_dump($notation->parentElement); + + $namedNotation = $notations->getNamedItem($name); + foreach ($notations as $iteratedNotation) { + // getNamedItem + var_dump($namedNotation->nodeName); + var_dump($namedNotation->textContent); + var_dump($namedNotation->nodeValue); + var_dump($namedNotation->isConnected); + var_dump($namedNotation->ownerDocument === $dom); + var_dump($namedNotation->parentNode === $doctype); + var_dump($namedNotation->parentElement); + + // iteration + var_dump($iteratedNotation->nodeName); + var_dump($iteratedNotation->textContent); + var_dump($iteratedNotation->nodeValue); + var_dump($iteratedNotation->isConnected); + var_dump($iteratedNotation->ownerDocument === $dom); + var_dump($iteratedNotation->parentNode === $doctype); + var_dump($iteratedNotation->parentElement); + + // wiring + var_dump($namedNotation->nodeName === $iteratedNotation->nodeName); + } } ?> --EXPECT-- @@ -39,6 +58,14 @@ bool(true) bool(true) bool(true) NULL +string(3) "GIF" +NULL +NULL +bool(true) +bool(true) +bool(true) +NULL +bool(true) === JPEG === string(4) "JPEG" NULL @@ -47,6 +74,14 @@ bool(true) bool(true) bool(true) NULL +string(4) "JPEG" +NULL +NULL +bool(true) +bool(true) +bool(true) +NULL +bool(true) === HTML === string(4) "HTML" NULL @@ -55,3 +90,11 @@ bool(true) bool(true) bool(true) NULL +string(4) "HTML" +NULL +NULL +bool(true) +bool(true) +bool(true) +NULL +bool(true) From 370a4464b249562a1a4683f8cbe48744044ade3b Mon Sep 17 00:00:00 2001 From: Jordi Kroon Date: Sat, 25 Apr 2026 14:43:16 +0200 Subject: [PATCH 4/5] apply suggested review Co-authored-by: David CARLIER --- ext/dom/tests/modern/xml/XMLDocument_node_notation_wiring.phpt | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/ext/dom/tests/modern/xml/XMLDocument_node_notation_wiring.phpt b/ext/dom/tests/modern/xml/XMLDocument_node_notation_wiring.phpt index a3502bcddf53..15dfb537a291 100644 --- a/ext/dom/tests/modern/xml/XMLDocument_node_notation_wiring.phpt +++ b/ext/dom/tests/modern/xml/XMLDocument_node_notation_wiring.phpt @@ -45,7 +45,8 @@ XML; var_dump($iteratedNotation->parentElement); // wiring - var_dump($namedNotation->nodeName === $iteratedNotation->nodeName); +// getNamedItem and iteration each allocate a fresh Notation instance + var_dump($namedNotation !== $iteratedNotation); } } ?> From 9e945f9f18a8b3e4f65946e0712a76dff2a375f0 Mon Sep 17 00:00:00 2001 From: Jordi Kroon Date: Sat, 25 Apr 2026 14:46:26 +0200 Subject: [PATCH 5/5] Fix comment alignment in XMLDocument test --- .../tests/modern/xml/XMLDocument_node_notation_wiring.phpt | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/ext/dom/tests/modern/xml/XMLDocument_node_notation_wiring.phpt b/ext/dom/tests/modern/xml/XMLDocument_node_notation_wiring.phpt index 15dfb537a291..dd74a2737594 100644 --- a/ext/dom/tests/modern/xml/XMLDocument_node_notation_wiring.phpt +++ b/ext/dom/tests/modern/xml/XMLDocument_node_notation_wiring.phpt @@ -45,8 +45,8 @@ XML; var_dump($iteratedNotation->parentElement); // wiring -// getNamedItem and iteration each allocate a fresh Notation instance - var_dump($namedNotation !== $iteratedNotation); + // getNamedItem and iteration each allocate a fresh Notation instance + var_dump($namedNotation !== $iteratedNotation); } } ?>