From 163e16d00386614fdedc2f926633c477b5cdb5ed Mon Sep 17 00:00:00 2001 From: Niels Dossche <7771979+nielsdos@users.noreply.github.com> Date: Mon, 7 Aug 2023 20:46:39 +0200 Subject: [PATCH 1/7] Align DOMChildNode parent checks with spec --- UPGRADING | 5 +++ ext/dom/parentnode.c | 6 --- .../DOMChildNode_methods_without_parent.phpt | 44 +++++++++++++++++++ 3 files changed, 49 insertions(+), 6 deletions(-) create mode 100644 ext/dom/tests/DOMChildNode_methods_without_parent.phpt diff --git a/UPGRADING b/UPGRADING index f18c7118e921..2d9d92459ddc 100644 --- a/UPGRADING +++ b/UPGRADING @@ -46,6 +46,11 @@ PHP 8.3 UPGRADE NOTES iterating over the WeakMap (reachability via iteration is considered weak). Previously, such entries would never be automatically removed. +- DOM: + . DOMChildNode::after(), DOMChildNode::before(), DOMChildNode::replaceWith() + on a node that has no parent is now a no-op instead of a hierarchy exception, + which is the behaviour spec demands. + - FFI: . C functions that have a return type of void now return null instead of returning the following object object(FFI\CData:void) { } diff --git a/ext/dom/parentnode.c b/ext/dom/parentnode.c index 667bb684e000..a0d84ff2c5aa 100644 --- a/ext/dom/parentnode.c +++ b/ext/dom/parentnode.c @@ -396,8 +396,6 @@ void dom_parent_node_after(dom_object *context, zval *nodes, uint32_t nodesc) parentNode = prevsib->parent; /* Spec step 2 */ if (!parentNode) { - int stricterror = dom_get_strict_error(context->document); - php_dom_throw_error(HIERARCHY_REQUEST_ERR, stricterror); return; } @@ -453,8 +451,6 @@ void dom_parent_node_before(dom_object *context, zval *nodes, uint32_t nodesc) parentNode = nextsib->parent; /* Spec step 2 */ if (!parentNode) { - int stricterror = dom_get_strict_error(context->document); - php_dom_throw_error(HIERARCHY_REQUEST_ERR, stricterror); return; } @@ -555,8 +551,6 @@ void dom_child_replace_with(dom_object *context, zval *nodes, uint32_t nodesc) xmlNodePtr parentNode = child->parent; /* Spec step 2 */ if (!parentNode) { - int stricterror = dom_get_strict_error(context->document); - php_dom_throw_error(HIERARCHY_REQUEST_ERR, stricterror); return; } diff --git a/ext/dom/tests/DOMChildNode_methods_without_parent.phpt b/ext/dom/tests/DOMChildNode_methods_without_parent.phpt new file mode 100644 index 000000000000..2591105cc37f --- /dev/null +++ b/ext/dom/tests/DOMChildNode_methods_without_parent.phpt @@ -0,0 +1,44 @@ +--TEST-- +DOMChildNode methods without a parent +--EXTENSIONS-- +dom +--FILE-- +loadXML(<< + + + +XML); + +$container = $doc->documentElement; +$child = $container->firstElementChild; + +$test = $doc->createElement('foo'); +foreach (['before', 'after', 'replaceWith'] as $method) { + echo "--- $method ---\n"; + $test->$method($child); + echo $doc->saveXML(); + echo $doc->saveXML($test), "\n"; +} +?> +--EXPECT-- +--- before --- + + + + + +--- after --- + + + + + +--- replaceWith --- + + + + + From 99908d5e063b81ed2349dc78aba26caba893aad7 Mon Sep 17 00:00:00 2001 From: Niels Dossche <7771979+nielsdos@users.noreply.github.com> Date: Mon, 7 Aug 2023 21:37:11 +0200 Subject: [PATCH 2/7] Fix mistake where it did not output a hierarchy error because the parent check came too early --- ext/dom/parentnode.c | 36 ++++++++++++++++-------------------- 1 file changed, 16 insertions(+), 20 deletions(-) diff --git a/ext/dom/parentnode.c b/ext/dom/parentnode.c index a0d84ff2c5aa..4d2648f35e1f 100644 --- a/ext/dom/parentnode.c +++ b/ext/dom/parentnode.c @@ -240,8 +240,13 @@ static void dom_fragment_assign_parent_node(xmlNodePtr parentNode, xmlNodePtr fr static zend_result dom_sanity_check_node_list_for_insertion(php_libxml_ref_obj *document, xmlNodePtr parentNode, zval *nodes, int nodesc) { - if (document == NULL) { - php_dom_throw_error(HIERARCHY_REQUEST_ERR, 1); + if (UNEXPECTED(document == NULL)) { + php_dom_throw_error(HIERARCHY_REQUEST_ERR, 1 /* no document, so be strict */); + return FAILURE; + } + + if (UNEXPECTED(parentNode == NULL)) { + /* No error required, this must be a no-op per spec */ return FAILURE; } @@ -394,8 +399,9 @@ void dom_parent_node_after(dom_object *context, zval *nodes, uint32_t nodesc) /* Spec step 1 */ parentNode = prevsib->parent; - /* Spec step 2 */ - if (!parentNode) { + + /* Sanity check for fragment, includes spec step 2 */ + if (UNEXPECTED(dom_sanity_check_node_list_for_insertion(context->document, parentNode, nodes, nodesc) != SUCCESS)) { return; } @@ -410,10 +416,6 @@ void dom_parent_node_after(dom_object *context, zval *nodes, uint32_t nodesc) doc = prevsib->doc; - if (UNEXPECTED(dom_sanity_check_node_list_for_insertion(context->document, parentNode, nodes, nodesc) != SUCCESS)) { - return; - } - php_libxml_invalidate_node_list_cache_from_doc(doc); /* Spec step 4: convert nodes into fragment */ @@ -449,8 +451,9 @@ void dom_parent_node_before(dom_object *context, zval *nodes, uint32_t nodesc) /* Spec step 1 */ parentNode = nextsib->parent; - /* Spec step 2 */ - if (!parentNode) { + + /* Sanity check for fragment, includes spec step 2 */ + if (UNEXPECTED(dom_sanity_check_node_list_for_insertion(context->document, parentNode, nodes, nodesc) != SUCCESS)) { return; } @@ -465,10 +468,6 @@ void dom_parent_node_before(dom_object *context, zval *nodes, uint32_t nodesc) doc = nextsib->doc; - if (UNEXPECTED(dom_sanity_check_node_list_for_insertion(context->document, parentNode, nodes, nodesc) != SUCCESS)) { - return; - } - php_libxml_invalidate_node_list_cache_from_doc(doc); /* Spec step 4: convert nodes into fragment */ @@ -549,8 +548,9 @@ void dom_child_replace_with(dom_object *context, zval *nodes, uint32_t nodesc) /* Spec step 1 */ xmlNodePtr parentNode = child->parent; - /* Spec step 2 */ - if (!parentNode) { + + /* Sanity check for fragment, includes spec step 2 */ + if (UNEXPECTED(dom_sanity_check_node_list_for_insertion(context->document, parentNode, nodes, nodesc) != SUCCESS)) { return; } @@ -568,10 +568,6 @@ void dom_child_replace_with(dom_object *context, zval *nodes, uint32_t nodesc) viable_next_sibling = viable_next_sibling->next; } - if (UNEXPECTED(dom_sanity_check_node_list_for_insertion(context->document, parentNode, nodes, nodesc) != SUCCESS)) { - return; - } - php_libxml_invalidate_node_list_cache_from_doc(context->document->ptr); /* Spec step 4: convert nodes into fragment */ From 68fa8d4c0a65442a4ec925b97c3a9e066a49780d Mon Sep 17 00:00:00 2001 From: Niels Dossche <7771979+nielsdos@users.noreply.github.com> Date: Mon, 7 Aug 2023 21:37:17 +0200 Subject: [PATCH 3/7] Extend regression test --- ext/dom/tests/bug79968.phpt | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/ext/dom/tests/bug79968.phpt b/ext/dom/tests/bug79968.phpt index bfd69e69c971..150be3d75671 100644 --- a/ext/dom/tests/bug79968.phpt +++ b/ext/dom/tests/bug79968.phpt @@ -10,8 +10,23 @@ $cdata = new DOMText; try { $cdata->before("string"); } catch (DOMException $e) { - echo $e->getMessage(); + echo $e->getMessage(), "\n"; } + +try { + $cdata->after("string"); +} catch (DOMException $e) { + echo $e->getMessage(), "\n"; +} + +try { + $cdata->replaceWith("string"); +} catch (DOMException $e) { + echo $e->getMessage(), "\n"; +} + ?> --EXPECT-- Hierarchy Request Error +Hierarchy Request Error +Hierarchy Request Error From be1867ec681f87041d507137fab7104796d78a25 Mon Sep 17 00:00:00 2001 From: Niels Dossche <7771979+nielsdos@users.noreply.github.com> Date: Tue, 8 Aug 2023 20:45:47 +0200 Subject: [PATCH 4/7] Remove document check --- ext/dom/parentnode.c | 5 ----- ext/dom/tests/bug79968.phpt | 26 +++++++------------------- 2 files changed, 7 insertions(+), 24 deletions(-) diff --git a/ext/dom/parentnode.c b/ext/dom/parentnode.c index 4d2648f35e1f..8eebfaabe4cd 100644 --- a/ext/dom/parentnode.c +++ b/ext/dom/parentnode.c @@ -240,11 +240,6 @@ static void dom_fragment_assign_parent_node(xmlNodePtr parentNode, xmlNodePtr fr static zend_result dom_sanity_check_node_list_for_insertion(php_libxml_ref_obj *document, xmlNodePtr parentNode, zval *nodes, int nodesc) { - if (UNEXPECTED(document == NULL)) { - php_dom_throw_error(HIERARCHY_REQUEST_ERR, 1 /* no document, so be strict */); - return FAILURE; - } - if (UNEXPECTED(parentNode == NULL)) { /* No error required, this must be a no-op per spec */ return FAILURE; diff --git a/ext/dom/tests/bug79968.phpt b/ext/dom/tests/bug79968.phpt index 150be3d75671..5ce1bcb7c6e9 100644 --- a/ext/dom/tests/bug79968.phpt +++ b/ext/dom/tests/bug79968.phpt @@ -7,26 +7,14 @@ dom $cdata = new DOMText; -try { - $cdata->before("string"); -} catch (DOMException $e) { - echo $e->getMessage(), "\n"; -} +$cdata->before("string"); +$cdata->after("string"); +$cdata->replaceWith("string"); -try { - $cdata->after("string"); -} catch (DOMException $e) { - echo $e->getMessage(), "\n"; -} - -try { - $cdata->replaceWith("string"); -} catch (DOMException $e) { - echo $e->getMessage(), "\n"; -} +$dom = new DOMDocument(); +$dom->adoptNode($cdata); +var_dump($dom->saveXML($cdata)); ?> --EXPECT-- -Hierarchy Request Error -Hierarchy Request Error -Hierarchy Request Error +string(0) "" From a521284774fcdddbf98ccdcbf36a1108f9faa7db Mon Sep 17 00:00:00 2001 From: Niels Dossche <7771979+nielsdos@users.noreply.github.com> Date: Tue, 8 Aug 2023 20:52:10 +0200 Subject: [PATCH 5/7] Add additional test --- ...hild_and_parent_node_without_document.phpt | 21 +++++++++++++++++++ 1 file changed, 21 insertions(+) create mode 100644 ext/dom/tests/element_child_and_parent_node_without_document.phpt diff --git a/ext/dom/tests/element_child_and_parent_node_without_document.phpt b/ext/dom/tests/element_child_and_parent_node_without_document.phpt new file mode 100644 index 000000000000..a849135f6699 --- /dev/null +++ b/ext/dom/tests/element_child_and_parent_node_without_document.phpt @@ -0,0 +1,21 @@ +--TEST-- +DOMElement: DOMChildNode, DOMParentNode modifications without a document +--EXTENSIONS-- +dom +--FILE-- +append("APPENDED"); +$element->prepend("PREPENDED"); +$element->after("AFTER"); +$element->before("BEFORE"); +$element->replaceWith("REPLACE"); + +$doc = new DOMDocument(); +$doc->adoptNode($element); +echo $doc->saveXML($element), "\n"; + +?> +--EXPECT-- +

PREPENDED Hello World! APPENDED

From 02a3dbb7583d8aada71dbac382c705101896c598 Mon Sep 17 00:00:00 2001 From: Niels Dossche <7771979+nielsdos@users.noreply.github.com> Date: Tue, 8 Aug 2023 20:52:16 +0200 Subject: [PATCH 6/7] [ci skip] UPGRADING --- UPGRADING | 3 +++ 1 file changed, 3 insertions(+) diff --git a/UPGRADING b/UPGRADING index 2d9d92459ddc..9cb34f3a7165 100644 --- a/UPGRADING +++ b/UPGRADING @@ -50,6 +50,9 @@ PHP 8.3 UPGRADE NOTES . DOMChildNode::after(), DOMChildNode::before(), DOMChildNode::replaceWith() on a node that has no parent is now a no-op instead of a hierarchy exception, which is the behaviour spec demands. + . Using the DOMParentNode and DOMChildNode methods without a document now works + instead of throwing a HIERARCHY_REQUEST_ERR DOMException. This is in line with + the behaviour spec demands. - FFI: . C functions that have a return type of void now return null instead of From 1cb6dbd751f2c226722633e8ba1800b48e86d1ac Mon Sep 17 00:00:00 2001 From: Niels Dossche <7771979+nielsdos@users.noreply.github.com> Date: Tue, 8 Aug 2023 21:17:26 +0200 Subject: [PATCH 7/7] Fix null pointer dereference by always using the node instead of the context --- ext/dom/parentnode.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/ext/dom/parentnode.c b/ext/dom/parentnode.c index 8eebfaabe4cd..865dba9b50b0 100644 --- a/ext/dom/parentnode.c +++ b/ext/dom/parentnode.c @@ -530,7 +530,7 @@ void dom_child_node_remove(dom_object *context) return; } - php_libxml_invalidate_node_list_cache_from_doc(context->document->ptr); + php_libxml_invalidate_node_list_cache_from_doc(child->doc); xmlUnlinkNode(child); } @@ -563,7 +563,8 @@ void dom_child_replace_with(dom_object *context, zval *nodes, uint32_t nodesc) viable_next_sibling = viable_next_sibling->next; } - php_libxml_invalidate_node_list_cache_from_doc(context->document->ptr); + xmlDocPtr doc = parentNode->doc; + php_libxml_invalidate_node_list_cache_from_doc(doc); /* Spec step 4: convert nodes into fragment */ xmlNodePtr fragment = dom_zvals_to_fragment(context->document, parentNode, nodes, nodesc); @@ -574,7 +575,6 @@ void dom_child_replace_with(dom_object *context, zval *nodes, uint32_t nodesc) /* Spec step 5: perform the replacement */ xmlNodePtr newchild = fragment->children; - xmlDocPtr doc = parentNode->doc; /* Unlink it unless it became a part of the fragment. * Freeing will be taken care of by the lifetime of the returned dom object. */ @@ -609,7 +609,7 @@ void dom_parent_node_replace_children(dom_object *context, zval *nodes, uint32_t return; } - php_libxml_invalidate_node_list_cache_from_doc(context->document->ptr); + php_libxml_invalidate_node_list_cache_from_doc(thisp->doc); dom_remove_all_children(thisp);