From cece2056bfa1523a0227b66b24b34a0eab0ac983 Mon Sep 17 00:00:00 2001 From: David Carlier Date: Sat, 7 Nov 2026 10:09:24 +0000 Subject: [PATCH 1/3] ext/dom: clone of Dom\Notation NULL dereference. Fix GH-25268 dom_clone_node() could not copy notation nodes, so cloning a notation attached to a document left the clone without a document proxy and dereferenced it. Copy the notation like create_notation() does. --- ext/dom/php_dom.c | 7 +++++++ ext/dom/tests/modern/xml/gh25268.phpt | 29 +++++++++++++++++++++++++++ 2 files changed, 36 insertions(+) create mode 100644 ext/dom/tests/modern/xml/gh25268.phpt diff --git a/ext/dom/php_dom.c b/ext/dom/php_dom.c index 445eacdfa198..d28ca0c0b6e5 100644 --- a/ext/dom/php_dom.c +++ b/ext/dom/php_dom.c @@ -2683,6 +2683,13 @@ static xmlNodePtr dom_clone_helper(php_dom_libxml_ns_mapper *ns_mapper, xmlNodeP xmlNodePtr dom_clone_node(php_dom_libxml_ns_mapper *ns_mapper, xmlNodePtr node, xmlDocPtr doc, bool recursive) { + if (node->type == XML_NOTATION_NODE) { + const xmlEntity *notation = (const xmlEntity *) node; + xmlNodePtr clone = create_notation(NULL, notation->name, notation->ExternalID, notation->SystemID); + clone->doc = doc; + return clone; + } + if (node->type == XML_DTD_NODE) { /* The behaviour w.r.t. the internal subset is implementation-defined according to DOM 3. * This follows what e.g. Java and C# do: copy it regardless of the recursiveness. diff --git a/ext/dom/tests/modern/xml/gh25268.phpt b/ext/dom/tests/modern/xml/gh25268.phpt new file mode 100644 index 000000000000..419aaa7e0f8e --- /dev/null +++ b/ext/dom/tests/modern/xml/gh25268.phpt @@ -0,0 +1,29 @@ +--TEST-- +GH-25268 (NULL php_libxml_ref_obj dereference in dom_objects_store_clone_obj when cloning Dom\Notation) +--EXTENSIONS-- +dom +--CREDITS-- +YuanchengJiang +--FILE-- +]>'); +$notation = $dom->doctype->notations->getNamedItem("GIF"); + +$clone = clone $notation; +var_dump($clone->nodeName, $clone->ownerDocument === $dom, $clone->parentNode, $clone->isConnected); + +$clone2 = $notation->cloneNode(); +var_dump($clone2->nodeName, $clone2 === $clone); + +unset($dom, $notation); +var_dump($clone->nodeName, $clone2->nodeName); +?> +--EXPECT-- +string(3) "GIF" +bool(true) +NULL +bool(false) +string(3) "GIF" +bool(false) +string(3) "GIF" +string(3) "GIF" From ac6d7f0968c264d4d46848bb99e860e10b17abd5 Mon Sep 17 00:00:00 2001 From: David Carlier Date: Sat, 7 Nov 2026 15:45:27 +0000 Subject: [PATCH 2/3] apply suggestions --- ext/dom/php_dom.c | 15 ++++------ ext/dom/tests/modern/xml/gh25268.phpt | 29 ------------------- .../modern/xml/gh25268_declarations.phpt | 20 +++++++++++++ 3 files changed, 25 insertions(+), 39 deletions(-) delete mode 100644 ext/dom/tests/modern/xml/gh25268.phpt create mode 100644 ext/dom/tests/modern/xml/gh25268_declarations.phpt diff --git a/ext/dom/php_dom.c b/ext/dom/php_dom.c index d28ca0c0b6e5..ed300185eec1 100644 --- a/ext/dom/php_dom.c +++ b/ext/dom/php_dom.c @@ -663,9 +663,11 @@ static zend_object *dom_objects_store_clone_obj(zend_object *zobject) /* {{{ */ xmlNodePtr cloned_node = dom_clone_node(php_dom_ns_mapper_from_private(private_data), node, node->doc, true); if (cloned_node != NULL) { dom_update_refcount_after_clone(intern, node, clone, cloned_node); - } - if (private_data != NULL) { - clone->document->private_data = php_dom_libxml_private_data_header(private_data); + if (private_data != NULL) { + clone->document->private_data = php_dom_libxml_private_data_header(private_data); + } + } else if (private_data != NULL && (node->type == XML_DOCUMENT_NODE || node->type == XML_HTML_DOCUMENT_NODE)) { + php_dom_private_data_destroy(private_data); } } } @@ -2683,13 +2685,6 @@ static xmlNodePtr dom_clone_helper(php_dom_libxml_ns_mapper *ns_mapper, xmlNodeP xmlNodePtr dom_clone_node(php_dom_libxml_ns_mapper *ns_mapper, xmlNodePtr node, xmlDocPtr doc, bool recursive) { - if (node->type == XML_NOTATION_NODE) { - const xmlEntity *notation = (const xmlEntity *) node; - xmlNodePtr clone = create_notation(NULL, notation->name, notation->ExternalID, notation->SystemID); - clone->doc = doc; - return clone; - } - if (node->type == XML_DTD_NODE) { /* The behaviour w.r.t. the internal subset is implementation-defined according to DOM 3. * This follows what e.g. Java and C# do: copy it regardless of the recursiveness. diff --git a/ext/dom/tests/modern/xml/gh25268.phpt b/ext/dom/tests/modern/xml/gh25268.phpt deleted file mode 100644 index 419aaa7e0f8e..000000000000 --- a/ext/dom/tests/modern/xml/gh25268.phpt +++ /dev/null @@ -1,29 +0,0 @@ ---TEST-- -GH-25268 (NULL php_libxml_ref_obj dereference in dom_objects_store_clone_obj when cloning Dom\Notation) ---EXTENSIONS-- -dom ---CREDITS-- -YuanchengJiang ---FILE-- -]>'); -$notation = $dom->doctype->notations->getNamedItem("GIF"); - -$clone = clone $notation; -var_dump($clone->nodeName, $clone->ownerDocument === $dom, $clone->parentNode, $clone->isConnected); - -$clone2 = $notation->cloneNode(); -var_dump($clone2->nodeName, $clone2 === $clone); - -unset($dom, $notation); -var_dump($clone->nodeName, $clone2->nodeName); -?> ---EXPECT-- -string(3) "GIF" -bool(true) -NULL -bool(false) -string(3) "GIF" -bool(false) -string(3) "GIF" -string(3) "GIF" diff --git a/ext/dom/tests/modern/xml/gh25268_declarations.phpt b/ext/dom/tests/modern/xml/gh25268_declarations.phpt new file mode 100644 index 000000000000..e61d6eef3cb1 --- /dev/null +++ b/ext/dom/tests/modern/xml/gh25268_declarations.phpt @@ -0,0 +1,20 @@ +--TEST-- +GH-25268 (NULL php_libxml_ref_obj dereference in dom_objects_store_clone_obj when cloning DTD declarations) +--EXTENSIONS-- +dom +--FILE-- +]>'); + +foreach ([$dom->doctype->entities->getNamedItem("e"), $dom->doctype->childNodes[0]] as $decl) { + $clone = clone $decl; + try { + var_dump($clone->nodeName); + } catch (DOMException $e) { + echo $e::class, ': ', $e->getMessage(), PHP_EOL; + } +} +?> +--EXPECT-- +DOMException: Invalid State Error +DOMException: Invalid State Error From ead08c738176190b93c31d887f6e56806c869393 Mon Sep 17 00:00:00 2001 From: David Carlier Date: Sat, 7 Nov 2026 17:48:26 +0000 Subject: [PATCH 3/3] feedback --- ext/dom/node.c | 4 ++++ ext/dom/tests/modern/xml/gh25268_declarations.phpt | 7 +++++++ 2 files changed, 11 insertions(+) diff --git a/ext/dom/node.c b/ext/dom/node.c index 49e400ef5302..867d166edb47 100644 --- a/ext/dom/node.c +++ b/ext/dom/node.c @@ -1513,6 +1513,10 @@ PHP_METHOD(DOMNode, cloneNode) if (clone_document && private_data != NULL) { php_dom_private_data_destroy(private_data); } + if (instanceof_function(intern->std.ce, dom_modern_node_class_entry)) { + php_dom_throw_error(INVALID_STATE_ERR, /* strict */ true); + RETURN_THROWS(); + } RETURN_FALSE; } diff --git a/ext/dom/tests/modern/xml/gh25268_declarations.phpt b/ext/dom/tests/modern/xml/gh25268_declarations.phpt index e61d6eef3cb1..ce814f2f7c4b 100644 --- a/ext/dom/tests/modern/xml/gh25268_declarations.phpt +++ b/ext/dom/tests/modern/xml/gh25268_declarations.phpt @@ -13,8 +13,15 @@ foreach ([$dom->doctype->entities->getNamedItem("e"), $dom->doctype->childNodes[ } catch (DOMException $e) { echo $e::class, ': ', $e->getMessage(), PHP_EOL; } + try { + var_dump($decl->cloneNode()); + } catch (DOMException $e) { + echo $e::class, ': ', $e->getMessage(), PHP_EOL; + } } ?> --EXPECT-- DOMException: Invalid State Error DOMException: Invalid State Error +DOMException: Invalid State Error +DOMException: Invalid State Error