Skip to content

ext/dom: Fix Dom\Notation nodes missing tree connection #21868

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
jordikroon wants to merge 5 commits into from
Closed

ext/dom: Fix Dom\Notation nodes missing tree connection #21868

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
fe554cf
fix Dom\Notation nodes missing tree connection
jordikroon Apr 24, 2026
11b3bdd
free notation node memory during object destruction
jordikroon Apr 24, 2026
aab5579
refcount should be unsigned, and update tests
jordikroon Apr 25, 2026
370a446
apply suggested review
jordikroon Apr 25, 2026
9e945f9
Fix comment alignment in XMLDocument test
jordikroon Apr 25, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 14 additions & 1 deletion ext/dom/dom_iterators.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,18 +54,31 @@ 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));
ret->type = XML_NOTATION_NODE;
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;
}
/* }}} */

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;
Expand Down
6 changes: 4 additions & 2 deletions ext/dom/obj_map.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand Down Expand Up @@ -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;
}
Expand Down
8 changes: 7 additions & 1 deletion ext/dom/php_dom.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
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);
}
} 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);
Expand Down
3 changes: 2 additions & 1 deletion ext/dom/php_dom.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,7 +125,8 @@ 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);
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);
Expand Down
13 changes: 7 additions & 6 deletions ext/dom/tests/modern/xml/DTDNamedNodeMap.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"]);

?>
Expand Down Expand Up @@ -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"]=>
Expand All @@ -168,5 +169,5 @@ object(Dom\Notation)#4 (13) {
["nodeValue"]=>
NULL
["textContent"]=>
string(0) ""
NULL
}
101 changes: 101 additions & 0 deletions ext/dom/tests/modern/xml/XMLDocument_node_notation_wiring.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,101 @@
--TEST--
Dom\XMLDocument: Dom\Notation nodes are connected to their document and doctype
--EXTENSIONS--
dom
--FILE--
<?php
$cases = [
'GIF' => '<!NOTATION GIF SYSTEM "image/gif">',
'JPEG' => '<!NOTATION JPEG PUBLIC "-//W3C//NOTATION JPEG//EN" "image/jpeg">',
'HTML' => '<!NOTATION HTML PUBLIC "-//W3C//NOTATION HTML//EN">',
];

foreach ($cases as $name => $declaration) {
$xml = <<<XML
<!DOCTYPE root [
$declaration
]>
<root/>
XML;

$dom = Dom\XMLDocument::createFromString($xml);
$doctype = $dom->doctype;
$notations = $doctype->notations;

echo "=== $name ===\n";

$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
// getNamedItem and iteration each allocate a fresh Notation instance
var_dump($namedNotation !== $iteratedNotation);
}
}
?>
--EXPECT--
=== GIF ===
string(3) "GIF"
NULL
NULL
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
NULL
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
NULL
bool(true)
bool(true)
bool(true)
NULL
string(4) "HTML"
NULL
NULL
bool(true)
bool(true)
bool(true)
NULL
bool(true)
Loading