From 08c4db7f36d30578c803d8950df3deda4f46516d Mon Sep 17 00:00:00 2001 From: Niels Dossche <7771979+nielsdos@users.noreply.github.com> Date: Sun, 6 Aug 2023 16:49:02 +0200 Subject: [PATCH 1/2] Fix manually calling __construct() on DOM classes Closes GH-11894. --- NEWS | 1 + ext/dom/attr.c | 2 +- ext/dom/cdatasection.c | 2 +- ext/dom/comment.c | 10 +++--- ext/dom/document.c | 26 +++++++------- ext/dom/documentfragment.c | 3 +- ext/dom/element.c | 2 +- ext/dom/entityreference.c | 10 +++--- ext/dom/processinginstruction.c | 2 +- ...MDocumentFragment_construct_basic_002.phpt | 16 --------- .../manually_call_constructor/attribute.phpt | 34 +++++++++++++++++++ .../cdatasection.phpt | 32 +++++++++++++++++ .../manually_call_constructor/comment.phpt | 34 +++++++++++++++++++ .../manually_call_constructor/document.phpt | 15 ++++++++ .../documentfragment.phpt | 32 +++++++++++++++++ .../manually_call_constructor/element.phpt | 34 +++++++++++++++++++ .../entityreference.phpt | 34 +++++++++++++++++++ .../processinginstruction.phpt | 34 +++++++++++++++++++ .../tests/manually_call_constructor/text.phpt | 33 ++++++++++++++++++ ext/dom/text.c | 10 +++--- 20 files changed, 312 insertions(+), 54 deletions(-) delete mode 100644 ext/dom/tests/DOMDocumentFragment_construct_basic_002.phpt create mode 100644 ext/dom/tests/manually_call_constructor/attribute.phpt create mode 100644 ext/dom/tests/manually_call_constructor/cdatasection.phpt create mode 100644 ext/dom/tests/manually_call_constructor/comment.phpt create mode 100644 ext/dom/tests/manually_call_constructor/document.phpt create mode 100644 ext/dom/tests/manually_call_constructor/documentfragment.phpt create mode 100644 ext/dom/tests/manually_call_constructor/element.phpt create mode 100644 ext/dom/tests/manually_call_constructor/entityreference.phpt create mode 100644 ext/dom/tests/manually_call_constructor/processinginstruction.phpt create mode 100644 ext/dom/tests/manually_call_constructor/text.phpt diff --git a/NEWS b/NEWS index c245c0bdc85..4b74b53d913 100644 --- a/NEWS +++ b/NEWS @@ -17,6 +17,7 @@ PHP NEWS . Fixed bug GH-11791 (Wrong default value of DOMDocument::xmlStandalone). (nielsdos) . Fix json_encode result on DOMDocument. (nielsdos) + . Fix manually calling __construct() on DOM classes. (nielsdos) - FFI: . Fix leaking definitions when using FFI::cdef()->new(...). (ilutov) diff --git a/ext/dom/attr.c b/ext/dom/attr.c index a262aea8213..399de6bfb90 100644 --- a/ext/dom/attr.c +++ b/ext/dom/attr.c @@ -62,7 +62,7 @@ PHP_METHOD(DOMAttr, __construct) oldnode = dom_object_get_node(intern); if (oldnode != NULL) { - php_libxml_node_free_resource(oldnode ); + php_libxml_node_decrement_resource((php_libxml_node_object *)intern); } php_libxml_increment_node_ptr((php_libxml_node_object *)intern, (xmlNodePtr)nodep, (void *)intern); } diff --git a/ext/dom/cdatasection.c b/ext/dom/cdatasection.c index e95e1a9030f..08d4937341d 100644 --- a/ext/dom/cdatasection.c +++ b/ext/dom/cdatasection.c @@ -52,7 +52,7 @@ PHP_METHOD(DOMCdataSection, __construct) intern = Z_DOMOBJ_P(ZEND_THIS); oldnode = dom_object_get_node(intern); if (oldnode != NULL) { - php_libxml_node_free_resource(oldnode ); + php_libxml_node_decrement_resource((php_libxml_node_object *)intern); } php_libxml_increment_node_ptr((php_libxml_node_object *)intern, nodep, (void *)intern); } diff --git a/ext/dom/comment.c b/ext/dom/comment.c index 76fe3200d46..16a52a5a4e2 100644 --- a/ext/dom/comment.c +++ b/ext/dom/comment.c @@ -50,13 +50,11 @@ PHP_METHOD(DOMComment, __construct) } intern = Z_DOMOBJ_P(ZEND_THIS); - if (intern != NULL) { - oldnode = dom_object_get_node(intern); - if (oldnode != NULL) { - php_libxml_node_free_resource(oldnode ); - } - php_libxml_increment_node_ptr((php_libxml_node_object *)intern, (xmlNodePtr)nodep, (void *)intern); + oldnode = dom_object_get_node(intern); + if (oldnode != NULL) { + php_libxml_node_decrement_resource((php_libxml_node_object *)intern); } + php_libxml_increment_node_ptr((php_libxml_node_object *)intern, (xmlNodePtr)nodep, (void *)intern); } /* }}} end DOMComment::__construct */ diff --git a/ext/dom/document.c b/ext/dom/document.c index ad98cf9a850..ea6daafeb30 100644 --- a/ext/dom/document.c +++ b/ext/dom/document.c @@ -1118,22 +1118,20 @@ PHP_METHOD(DOMDocument, __construct) } intern = Z_DOMOBJ_P(ZEND_THIS); - if (intern != NULL) { - olddoc = (xmlDocPtr) dom_object_get_node(intern); - if (olddoc != NULL) { - php_libxml_decrement_node_ptr((php_libxml_node_object *) intern); - refcount = php_libxml_decrement_doc_ref((php_libxml_node_object *)intern); - if (refcount != 0) { - olddoc->_private = NULL; - } + olddoc = (xmlDocPtr) dom_object_get_node(intern); + if (olddoc != NULL) { + php_libxml_decrement_node_ptr((php_libxml_node_object *) intern); + refcount = php_libxml_decrement_doc_ref((php_libxml_node_object *)intern); + if (refcount != 0) { + olddoc->_private = NULL; } - intern->document = NULL; - if (php_libxml_increment_doc_ref((php_libxml_node_object *)intern, docp) == -1) { - /* docp is always non-null so php_libxml_increment_doc_ref() never returns -1 */ - ZEND_UNREACHABLE(); - } - php_libxml_increment_node_ptr((php_libxml_node_object *)intern, (xmlNodePtr)docp, (void *)intern); } + intern->document = NULL; + if (php_libxml_increment_doc_ref((php_libxml_node_object *)intern, docp) == -1) { + /* docp is always non-null so php_libxml_increment_doc_ref() never returns -1 */ + ZEND_UNREACHABLE(); + } + php_libxml_increment_node_ptr((php_libxml_node_object *)intern, (xmlNodePtr)docp, (void *)intern); } /* }}} end DOMDocument::__construct */ diff --git a/ext/dom/documentfragment.c b/ext/dom/documentfragment.c index 274d6954353..033ee08aee1 100644 --- a/ext/dom/documentfragment.c +++ b/ext/dom/documentfragment.c @@ -50,9 +50,8 @@ PHP_METHOD(DOMDocumentFragment, __construct) intern = Z_DOMOBJ_P(ZEND_THIS); oldnode = dom_object_get_node(intern); if (oldnode != NULL) { - php_libxml_node_free_resource(oldnode ); + php_libxml_node_decrement_resource((php_libxml_node_object *)intern); } - /* php_dom_set_object(intern, nodep); */ php_libxml_increment_node_ptr((php_libxml_node_object *)intern, nodep, (void *)intern); } /* }}} end DOMDocumentFragment::__construct */ diff --git a/ext/dom/element.c b/ext/dom/element.c index 2379b81e1dd..c630bec2b50 100644 --- a/ext/dom/element.c +++ b/ext/dom/element.c @@ -97,7 +97,7 @@ PHP_METHOD(DOMElement, __construct) intern = Z_DOMOBJ_P(ZEND_THIS); oldnode = dom_object_get_node(intern); if (oldnode != NULL) { - php_libxml_node_free_resource(oldnode ); + php_libxml_node_decrement_resource((php_libxml_node_object *)intern); } php_libxml_increment_node_ptr((php_libxml_node_object *)intern, nodep, (void *)intern); } diff --git a/ext/dom/entityreference.c b/ext/dom/entityreference.c index 16aadabfb59..61f0b92eedc 100644 --- a/ext/dom/entityreference.c +++ b/ext/dom/entityreference.c @@ -57,13 +57,11 @@ PHP_METHOD(DOMEntityReference, __construct) } intern = Z_DOMOBJ_P(ZEND_THIS); - if (intern != NULL) { - oldnode = dom_object_get_node(intern); - if (oldnode != NULL) { - php_libxml_node_free_resource(oldnode ); - } - php_libxml_increment_node_ptr((php_libxml_node_object *)intern, node, (void *)intern); + oldnode = dom_object_get_node(intern); + if (oldnode != NULL) { + php_libxml_node_decrement_resource((php_libxml_node_object *)intern); } + php_libxml_increment_node_ptr((php_libxml_node_object *)intern, node, (void *)intern); } /* }}} end DOMEntityReference::__construct */ diff --git a/ext/dom/processinginstruction.c b/ext/dom/processinginstruction.c index 465ecb431e7..a1c3d3a0878 100644 --- a/ext/dom/processinginstruction.c +++ b/ext/dom/processinginstruction.c @@ -59,7 +59,7 @@ PHP_METHOD(DOMProcessingInstruction, __construct) intern = Z_DOMOBJ_P(ZEND_THIS); oldnode = dom_object_get_node(intern); if (oldnode != NULL) { - php_libxml_node_free_resource(oldnode ); + php_libxml_node_decrement_resource((php_libxml_node_object *)intern); } php_libxml_increment_node_ptr((php_libxml_node_object *)intern, nodep, (void *)intern); } diff --git a/ext/dom/tests/DOMDocumentFragment_construct_basic_002.phpt b/ext/dom/tests/DOMDocumentFragment_construct_basic_002.phpt deleted file mode 100644 index 36a0762c16f..00000000000 --- a/ext/dom/tests/DOMDocumentFragment_construct_basic_002.phpt +++ /dev/null @@ -1,16 +0,0 @@ ---TEST-- -DOMDocumentFragment::__construct() called twice. ---CREDITS-- -Eric Lee Stewart -# TestFest Atlanta 2009-05-24 ---EXTENSIONS-- -dom ---FILE-- -__construct(); -var_dump($fragment); -?> ---EXPECT-- -object(DOMDocumentFragment)#1 (0) { -} diff --git a/ext/dom/tests/manually_call_constructor/attribute.phpt b/ext/dom/tests/manually_call_constructor/attribute.phpt new file mode 100644 index 00000000000..5e8be0abf74 --- /dev/null +++ b/ext/dom/tests/manually_call_constructor/attribute.phpt @@ -0,0 +1,34 @@ +--TEST-- +Manually call __construct() - attribute variation +--EXTENSIONS-- +dom +--FILE-- +nodeName, $attr->nodeValue); +$attr->__construct("newattribute", "my new value"); +var_dump($attr->nodeName, $attr->nodeValue); + +$doc = new DOMDocument(); +$doc->loadXML(<< + +XML); +$doc->documentElement->setAttributeNode($attr); +echo $doc->saveXML(); + +$attr->__construct("newnewattribute", "my even newer value"); +$doc->documentElement->setAttributeNode($attr); +echo $doc->saveXML(); + +?> +--EXPECT-- +string(9) "attribute" +string(8) "my value" +string(12) "newattribute" +string(12) "my new value" + + + + diff --git a/ext/dom/tests/manually_call_constructor/cdatasection.phpt b/ext/dom/tests/manually_call_constructor/cdatasection.phpt new file mode 100644 index 00000000000..01b8ab7ebd4 --- /dev/null +++ b/ext/dom/tests/manually_call_constructor/cdatasection.phpt @@ -0,0 +1,32 @@ +--TEST-- +Manually call __construct() - CDATA section variation +--EXTENSIONS-- +dom +--FILE-- +nodeValue); +$cdata->__construct("my new value"); +var_dump($cdata->nodeValue); + +$doc = new DOMDocument(); +$doc->loadXML(<< + +XML); +$doc->documentElement->appendChild($cdata); +echo $doc->saveXML(); + +$cdata->__construct("my even newer value"); +$doc->documentElement->appendChild($cdata); +echo $doc->saveXML(); + +?> +--EXPECT-- +string(8) "my value" +string(12) "my new value" + + + + diff --git a/ext/dom/tests/manually_call_constructor/comment.phpt b/ext/dom/tests/manually_call_constructor/comment.phpt new file mode 100644 index 00000000000..0abf6bcf6c1 --- /dev/null +++ b/ext/dom/tests/manually_call_constructor/comment.phpt @@ -0,0 +1,34 @@ +--TEST-- +Manually call __construct() - comment variation +--EXTENSIONS-- +dom +--FILE-- +nodeName, $comment->nodeValue); +$comment->__construct("my new value"); +var_dump($comment->nodeName, $comment->nodeValue); + +$doc = new DOMDocument(); +$doc->loadXML(<< + +XML); +$doc->documentElement->appendChild($comment); +echo $doc->saveXML(); + +$comment->__construct("my even newer value"); +$doc->documentElement->appendChild($comment); +echo $doc->saveXML(); + +?> +--EXPECT-- +string(8) "#comment" +string(8) "my value" +string(8) "#comment" +string(12) "my new value" + + + + diff --git a/ext/dom/tests/manually_call_constructor/document.phpt b/ext/dom/tests/manually_call_constructor/document.phpt new file mode 100644 index 00000000000..ce7aacc08b6 --- /dev/null +++ b/ext/dom/tests/manually_call_constructor/document.phpt @@ -0,0 +1,15 @@ +--TEST-- +Manually call __construct() - document variation +--EXTENSIONS-- +dom +--FILE-- +loadXML(''); +$doc->__construct("1.1", "UTF-8"); +echo $doc->saveXML(); + +?> +--EXPECT-- + diff --git a/ext/dom/tests/manually_call_constructor/documentfragment.phpt b/ext/dom/tests/manually_call_constructor/documentfragment.phpt new file mode 100644 index 00000000000..8e6f40d9951 --- /dev/null +++ b/ext/dom/tests/manually_call_constructor/documentfragment.phpt @@ -0,0 +1,32 @@ +--TEST-- +Manually call __construct() - document fragment variation +--EXTENSIONS-- +dom +--FILE-- +textContent); +$fragment->__construct(); +var_dump($fragment->textContent); + +$doc = new DOMDocument(); +$doc->loadXML(<< + +XML); +@$doc->documentElement->appendChild($fragment); +echo $doc->saveXML(); + +$fragment->__construct(); +@$doc->documentElement->appendChild($fragment); +echo $doc->saveXML(); + +?> +--EXPECT-- +string(0) "" +string(0) "" + + + + diff --git a/ext/dom/tests/manually_call_constructor/element.phpt b/ext/dom/tests/manually_call_constructor/element.phpt new file mode 100644 index 00000000000..9187340a461 --- /dev/null +++ b/ext/dom/tests/manually_call_constructor/element.phpt @@ -0,0 +1,34 @@ +--TEST-- +Manually call __construct() - element variation +--EXTENSIONS-- +dom +--FILE-- +nodeName, $element->textContent); +$element->__construct('foo2', 'my new value'); +var_dump($element->nodeName, $element->textContent); + +$doc = new DOMDocument(); +$doc->loadXML(<< + +XML); +$doc->documentElement->appendChild($element); +echo $doc->saveXML(); + +$element->__construct('foo3', 'my new new value'); +$doc->documentElement->appendChild($element); +echo $doc->saveXML(); + +?> +--EXPECT-- +string(3) "foo" +string(8) "my value" +string(4) "foo2" +string(12) "my new value" + +my new value + +my new valuemy new new value diff --git a/ext/dom/tests/manually_call_constructor/entityreference.phpt b/ext/dom/tests/manually_call_constructor/entityreference.phpt new file mode 100644 index 00000000000..5bffda5fd02 --- /dev/null +++ b/ext/dom/tests/manually_call_constructor/entityreference.phpt @@ -0,0 +1,34 @@ +--TEST-- +Manually call __construct() - entity reference variation +--EXTENSIONS-- +dom +--FILE-- +nodeName, $entityRef->textContent); +$entityRef->__construct('foo2'); +var_dump($entityRef->nodeName, $entityRef->textContent); + +$doc = new DOMDocument(); +$doc->loadXML(<< + +XML); +$doc->documentElement->appendChild($entityRef); +echo $doc->saveXML(); + +$entityRef->__construct('foo3'); +$doc->documentElement->appendChild($entityRef); +echo $doc->saveXML(); + +?> +--EXPECT-- +string(3) "foo" +string(0) "" +string(4) "foo2" +string(0) "" + +&foo2; + +&foo2;&foo3; diff --git a/ext/dom/tests/manually_call_constructor/processinginstruction.phpt b/ext/dom/tests/manually_call_constructor/processinginstruction.phpt new file mode 100644 index 00000000000..11e7ab1e28d --- /dev/null +++ b/ext/dom/tests/manually_call_constructor/processinginstruction.phpt @@ -0,0 +1,34 @@ +--TEST-- +Manually call __construct() - processing instruction variation +--EXTENSIONS-- +dom +--FILE-- +target, $pi->data); +$pi->__construct('name2', 'value2'); +var_dump($pi->target, $pi->data); + +$doc = new DOMDocument(); +$doc->loadXML(<< + +XML); +$doc->documentElement->appendChild($pi); +echo $doc->saveXML(); + +$pi->__construct('name3', 'value3'); +$doc->documentElement->appendChild($pi); +echo $doc->saveXML(); + +?> +--EXPECT-- +string(5) "name1" +string(6) "value1" +string(5) "name2" +string(6) "value2" + + + + diff --git a/ext/dom/tests/manually_call_constructor/text.phpt b/ext/dom/tests/manually_call_constructor/text.phpt new file mode 100644 index 00000000000..b91b0f66e33 --- /dev/null +++ b/ext/dom/tests/manually_call_constructor/text.phpt @@ -0,0 +1,33 @@ +--TEST-- +Manually call __construct() - text variation +--EXTENSIONS-- +dom +--FILE-- +textContent); +$text->__construct('my new value'); +var_dump($text->textContent); + +$doc = new DOMDocument(); +$doc->loadXML(<< + +XML); +$doc->documentElement->appendChild($text); +echo $doc->saveXML(); + +$text->__construct("\nmy new new value"); +$doc->documentElement->appendChild($text); +echo $doc->saveXML(); + +?> +--EXPECT-- +string(8) "my value" +string(12) "my new value" + +my new value + +my new value +my new new value diff --git a/ext/dom/text.c b/ext/dom/text.c index e782ff8ff6c..2a5630fa608 100644 --- a/ext/dom/text.c +++ b/ext/dom/text.c @@ -51,13 +51,11 @@ PHP_METHOD(DOMText, __construct) } intern = Z_DOMOBJ_P(ZEND_THIS); - if (intern != NULL) { - oldnode = dom_object_get_node(intern); - if (oldnode != NULL) { - php_libxml_node_free_resource(oldnode ); - } - php_libxml_increment_node_ptr((php_libxml_node_object *)intern, nodep, (void *)intern); + oldnode = dom_object_get_node(intern); + if (oldnode != NULL) { + php_libxml_node_decrement_resource((php_libxml_node_object *)intern); } + php_libxml_increment_node_ptr((php_libxml_node_object *)intern, nodep, (void *)intern); } /* }}} end DOMText::__construct */ From dddd309da4567ae2c9e8a104f9606320d94a5ef8 Mon Sep 17 00:00:00 2001 From: Niels Dossche <7771979+nielsdos@users.noreply.github.com> Date: Sat, 5 Aug 2023 21:27:02 +0200 Subject: [PATCH 2/2] Fix GH-11830: ParentNode methods should perform their checks upfront Closes GH-11887. --- NEWS | 2 + ext/dom/parentnode.c | 149 ++++++++++-------- .../tests/gh11830/attribute_variation.phpt | 56 +++++++ ext/dom/tests/gh11830/document_variation.phpt | 71 +++++++++ .../tests/gh11830/hierarchy_variation.phpt | 62 ++++++++ ext/dom/tests/gh11830/type_variation.phpt | 60 +++++++ 6 files changed, 330 insertions(+), 70 deletions(-) create mode 100644 ext/dom/tests/gh11830/attribute_variation.phpt create mode 100644 ext/dom/tests/gh11830/document_variation.phpt create mode 100644 ext/dom/tests/gh11830/hierarchy_variation.phpt create mode 100644 ext/dom/tests/gh11830/type_variation.phpt diff --git a/NEWS b/NEWS index 4b74b53d913..d6be4635337 100644 --- a/NEWS +++ b/NEWS @@ -18,6 +18,8 @@ PHP NEWS (nielsdos) . Fix json_encode result on DOMDocument. (nielsdos) . Fix manually calling __construct() on DOM classes. (nielsdos) + . Fixed bug GH-11830 (ParentNode methods should perform their checks + upfront). (nielsdos) - FFI: . Fix leaking definitions when using FFI::cdef()->new(...). (ilutov) diff --git a/ext/dom/parentnode.c b/ext/dom/parentnode.c index 8467fbf0603..df14f194bcb 100644 --- a/ext/dom/parentnode.c +++ b/ext/dom/parentnode.c @@ -141,26 +141,24 @@ static bool dom_is_node_in_list(const zval *nodes, int nodesc, const xmlNodePtr return false; } +static xmlDocPtr dom_doc_from_context_node(xmlNodePtr contextNode) +{ + if (contextNode->type == XML_DOCUMENT_NODE || contextNode->type == XML_HTML_DOCUMENT_NODE) { + return (xmlDocPtr) contextNode; + } else { + return contextNode->doc; + } +} + xmlNode* dom_zvals_to_fragment(php_libxml_ref_obj *document, xmlNode *contextNode, zval *nodes, int nodesc) { int i; xmlDoc *documentNode; xmlNode *fragment; xmlNode *newNode; - zend_class_entry *ce; dom_object *newNodeObj; - int stricterror; - if (document == NULL) { - php_dom_throw_error(HIERARCHY_REQUEST_ERR, 1); - return NULL; - } - - if (contextNode->type == XML_DOCUMENT_NODE || contextNode->type == XML_HTML_DOCUMENT_NODE) { - documentNode = (xmlDoc *) contextNode; - } else { - documentNode = contextNode->doc; - } + documentNode = dom_doc_from_context_node(contextNode); fragment = xmlNewDocFragment(documentNode); @@ -168,80 +166,59 @@ xmlNode* dom_zvals_to_fragment(php_libxml_ref_obj *document, xmlNode *contextNod return NULL; } - stricterror = dom_get_strict_error(document); - for (i = 0; i < nodesc; i++) { if (Z_TYPE(nodes[i]) == IS_OBJECT) { - ce = Z_OBJCE(nodes[i]); + newNodeObj = Z_DOMOBJ_P(&nodes[i]); + newNode = dom_object_get_node(newNodeObj); - if (instanceof_function(ce, dom_node_class_entry)) { - newNodeObj = Z_DOMOBJ_P(&nodes[i]); - newNode = dom_object_get_node(newNodeObj); + if (newNode->parent != NULL) { + xmlUnlinkNode(newNode); + } - if (newNode->doc != documentNode) { - php_dom_throw_error(WRONG_DOCUMENT_ERR, stricterror); - goto err; - } + newNodeObj->document = document; + xmlSetTreeDoc(newNode, documentNode); - if (newNode->parent != NULL) { + /* Citing from the docs (https://gnome.pages.gitlab.gnome.org/libxml2/devhelp/libxml2-tree.html#xmlAddChild): + * "Add a new node to @parent, at the end of the child (or property) list merging adjacent TEXT nodes (in which case @cur is freed)". + * So we must take a copy if this situation arises to prevent a use-after-free. */ + bool will_free = newNode->type == XML_TEXT_NODE && fragment->last && fragment->last->type == XML_TEXT_NODE; + if (will_free) { + newNode = xmlCopyNode(newNode, 1); + } + + if (newNode->type == XML_DOCUMENT_FRAG_NODE) { + /* Unpack document fragment nodes, the behaviour differs for different libxml2 versions. */ + newNode = newNode->children; + while (newNode) { + xmlNodePtr next = newNode->next; xmlUnlinkNode(newNode); + if (!xmlAddChild(fragment, newNode)) { + goto err; + } + newNode = next; } - - newNodeObj->document = document; - xmlSetTreeDoc(newNode, documentNode); - - if (newNode->type == XML_ATTRIBUTE_NODE) { - goto hierarchy_request_err; - } - - /* Citing from the docs (https://gnome.pages.gitlab.gnome.org/libxml2/devhelp/libxml2-tree.html#xmlAddChild): - * "Add a new node to @parent, at the end of the child (or property) list merging adjacent TEXT nodes (in which case @cur is freed)". - * So we must take a copy if this situation arises to prevent a use-after-free. */ - bool will_free = newNode->type == XML_TEXT_NODE && fragment->last && fragment->last->type == XML_TEXT_NODE; + } else if (!xmlAddChild(fragment, newNode)) { if (will_free) { - newNode = xmlCopyNode(newNode, 1); + xmlFreeNode(newNode); } - - if (newNode->type == XML_DOCUMENT_FRAG_NODE) { - /* Unpack document fragment nodes, the behaviour differs for different libxml2 versions. */ - newNode = newNode->children; - while (newNode) { - xmlNodePtr next = newNode->next; - xmlUnlinkNode(newNode); - if (!xmlAddChild(fragment, newNode)) { - goto hierarchy_request_err; - } - newNode = next; - } - } else if (!xmlAddChild(fragment, newNode)) { - if (will_free) { - xmlFreeNode(newNode); - } - goto hierarchy_request_err; - } - } else { - zend_argument_type_error(i + 1, "must be of type DOMNode|string, %s given", zend_zval_type_name(&nodes[i])); goto err; } - } else if (Z_TYPE(nodes[i]) == IS_STRING) { + } else { + ZEND_ASSERT(Z_TYPE(nodes[i]) == IS_STRING); + newNode = xmlNewDocText(documentNode, (xmlChar *) Z_STRVAL(nodes[i])); xmlSetTreeDoc(newNode, documentNode); if (!xmlAddChild(fragment, newNode)) { xmlFreeNode(newNode); - goto hierarchy_request_err; + goto err; } - } else { - zend_argument_type_error(i + 1, "must be of type DOMNode|string, %s given", zend_zval_type_name(&nodes[i])); - goto err; } } return fragment; -hierarchy_request_err: - php_dom_throw_error(HIERARCHY_REQUEST_ERR, stricterror); err: xmlFreeNode(fragment); return NULL; @@ -264,17 +241,39 @@ static void dom_fragment_assign_parent_node(xmlNodePtr parentNode, xmlNodePtr fr fragment->last = NULL; } -static zend_result dom_hierarchy_node_list(xmlNodePtr parentNode, zval *nodes, int nodesc) +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); + return FAILURE; + } + + xmlDocPtr documentNode = dom_doc_from_context_node(parentNode); + for (int i = 0; i < nodesc; i++) { - if (Z_TYPE(nodes[i]) == IS_OBJECT) { + zend_uchar type = Z_TYPE(nodes[i]); + if (type == IS_OBJECT) { const zend_class_entry *ce = Z_OBJCE(nodes[i]); if (instanceof_function(ce, dom_node_class_entry)) { - if (dom_hierarchy(parentNode, dom_object_get_node(Z_DOMOBJ_P(nodes + i))) != SUCCESS) { + xmlNodePtr node = dom_object_get_node(Z_DOMOBJ_P(nodes + i)); + + if (node->doc != documentNode) { + php_dom_throw_error(WRONG_DOCUMENT_ERR, dom_get_strict_error(document)); return FAILURE; } + + if (node->type == XML_ATTRIBUTE_NODE || dom_hierarchy(parentNode, node) != SUCCESS) { + php_dom_throw_error(HIERARCHY_REQUEST_ERR, dom_get_strict_error(document)); + return FAILURE; + } + } else { + zend_argument_type_error(i + 1, "must be of type DOMNode|string, %s given", zend_zval_type_name(&nodes[i])); + return FAILURE; } + } else if (type != IS_STRING) { + zend_argument_type_error(i + 1, "must be of type DOMNode|string, %s given", zend_zval_type_name(&nodes[i])); + return FAILURE; } } @@ -286,8 +285,7 @@ void dom_parent_node_append(dom_object *context, zval *nodes, int nodesc) xmlNode *parentNode = dom_object_get_node(context); xmlNodePtr newchild, prevsib; - if (UNEXPECTED(dom_hierarchy_node_list(parentNode, nodes, nodesc) != SUCCESS)) { - php_dom_throw_error(HIERARCHY_REQUEST_ERR, dom_get_strict_error(context->document)); + if (UNEXPECTED(dom_sanity_check_node_list_for_insertion(context->document, parentNode, nodes, nodesc) != SUCCESS)) { return; } @@ -329,8 +327,7 @@ void dom_parent_node_prepend(dom_object *context, zval *nodes, int nodesc) return; } - if (UNEXPECTED(dom_hierarchy_node_list(parentNode, nodes, nodesc) != SUCCESS)) { - php_dom_throw_error(HIERARCHY_REQUEST_ERR, dom_get_strict_error(context->document)); + if (UNEXPECTED(dom_sanity_check_node_list_for_insertion(context->document, parentNode, nodes, nodesc) != SUCCESS)) { return; } @@ -414,6 +411,10 @@ void dom_parent_node_after(dom_object *context, zval *nodes, int nodesc) doc = prevsib->doc; + if (UNEXPECTED(dom_sanity_check_node_list_for_insertion(context->document, parentNode, nodes, nodesc) != SUCCESS)) { + return; + } + /* Spec step 4: convert nodes into fragment */ fragment = dom_zvals_to_fragment(context->document, parentNode, nodes, nodesc); @@ -465,6 +466,10 @@ void dom_parent_node_before(dom_object *context, zval *nodes, int nodesc) doc = nextsib->doc; + if (UNEXPECTED(dom_sanity_check_node_list_for_insertion(context->document, parentNode, nodes, nodesc) != SUCCESS)) { + return; + } + /* Spec step 4: convert nodes into fragment */ fragment = dom_zvals_to_fragment(context->document, parentNode, nodes, nodesc); @@ -555,6 +560,10 @@ void dom_child_replace_with(dom_object *context, zval *nodes, int nodesc) xmlNodePtr insertion_point = child->next; + if (UNEXPECTED(dom_sanity_check_node_list_for_insertion(context->document, parentNode, nodes, nodesc) != SUCCESS)) { + return; + } + xmlNodePtr fragment = dom_zvals_to_fragment(context->document, parentNode, nodes, nodesc); if (UNEXPECTED(fragment == NULL)) { return; diff --git a/ext/dom/tests/gh11830/attribute_variation.phpt b/ext/dom/tests/gh11830/attribute_variation.phpt new file mode 100644 index 00000000000..34a6f094f58 --- /dev/null +++ b/ext/dom/tests/gh11830/attribute_variation.phpt @@ -0,0 +1,56 @@ +--TEST-- +GH-11830 (ParentNode methods should perform their checks upfront) - attribute variation +--EXTENSIONS-- +dom +--FILE-- +loadXML(<< + + + +XML); + +try { + $doc->documentElement->firstElementChild->prepend($doc->documentElement->attributes[0]); +} catch (\DOMException $e) { + echo $e->getMessage(), "\n"; +} + +try { + $doc->documentElement->firstElementChild->append($doc->documentElement->attributes[0]); +} catch (\DOMException $e) { + echo $e->getMessage(), "\n"; +} + +try { + $doc->documentElement->firstElementChild->before($doc->documentElement->attributes[0]); +} catch (\DOMException $e) { + echo $e->getMessage(), "\n"; +} + +try { + $doc->documentElement->firstElementChild->after($doc->documentElement->attributes[0]); +} catch (\DOMException $e) { + echo $e->getMessage(), "\n"; +} + +try { + $doc->documentElement->firstElementChild->replaceWith($doc->documentElement->attributes[0]); +} catch (\DOMException $e) { + echo $e->getMessage(), "\n"; +} + +echo $doc->saveXML(); +?> +--EXPECT-- +Hierarchy Request Error +Hierarchy Request Error +Hierarchy Request Error +Hierarchy Request Error +Hierarchy Request Error + + + + diff --git a/ext/dom/tests/gh11830/document_variation.phpt b/ext/dom/tests/gh11830/document_variation.phpt new file mode 100644 index 00000000000..89eed3dff22 --- /dev/null +++ b/ext/dom/tests/gh11830/document_variation.phpt @@ -0,0 +1,71 @@ +--TEST-- +GH-11830 (ParentNode methods should perform their checks upfront) - document variation +--EXTENSIONS-- +dom +--FILE-- +loadXML(<< + +XML); + +$otherElement = $otherDoc->documentElement; + +$doc = new DOMDocument; +$doc->loadXML(<< + + + + +XML); + +$testElement = $doc->documentElement->firstElementChild->nextElementSibling->firstElementChild; + +try { + $doc->documentElement->firstElementChild->prepend($testElement, $otherElement); +} catch (\DOMException $e) { + echo $e->getMessage(), "\n"; +} + +try { + $doc->documentElement->firstElementChild->append($testElement, $otherElement); +} catch (\DOMException $e) { + echo $e->getMessage(), "\n"; +} + +try { + $doc->documentElement->firstElementChild->before($testElement, $otherElement); +} catch (\DOMException $e) { + echo $e->getMessage(), "\n"; +} + +try { + $doc->documentElement->firstElementChild->after($testElement, $otherElement); +} catch (\DOMException $e) { + echo $e->getMessage(), "\n"; +} + +try { + $doc->documentElement->firstElementChild->replaceWith($testElement, $otherElement); +} catch (\DOMException $e) { + echo $e->getMessage(), "\n"; +} + +echo $otherDoc->saveXML(); +echo $doc->saveXML(); +?> +--EXPECT-- +Wrong Document Error +Wrong Document Error +Wrong Document Error +Wrong Document Error +Wrong Document Error + + + + + + + diff --git a/ext/dom/tests/gh11830/hierarchy_variation.phpt b/ext/dom/tests/gh11830/hierarchy_variation.phpt new file mode 100644 index 00000000000..bd6534ee71b --- /dev/null +++ b/ext/dom/tests/gh11830/hierarchy_variation.phpt @@ -0,0 +1,62 @@ +--TEST-- +GH-11830 (ParentNode methods should perform their checks upfront) - hierarchy variation +--EXTENSIONS-- +dom +--FILE-- +loadXML(<< + + + + +XML); + +$container = $doc->documentElement; +$alone = $container->firstElementChild; +$testElement = $alone->nextElementSibling->firstElementChild; + +try { + $testElement->prepend($alone, $container); +} catch (\DOMException $e) { + echo $e->getMessage(), "\n"; +} + +try { + $testElement->append($alone, $container); +} catch (\DOMException $e) { + echo $e->getMessage(), "\n"; +} + +try { + $testElement->before($alone, $container); +} catch (\DOMException $e) { + echo $e->getMessage(), "\n"; +} + +try { + $testElement->after($alone, $container); +} catch (\DOMException $e) { + echo $e->getMessage(), "\n"; +} + +try { + $testElement->replaceWith($alone, $container); +} catch (\DOMException $e) { + echo $e->getMessage(), "\n"; +} + +echo $doc->saveXML(); +?> +--EXPECT-- +Hierarchy Request Error +Hierarchy Request Error +Hierarchy Request Error +Hierarchy Request Error +Hierarchy Request Error + + + + + diff --git a/ext/dom/tests/gh11830/type_variation.phpt b/ext/dom/tests/gh11830/type_variation.phpt new file mode 100644 index 00000000000..76732775e6d --- /dev/null +++ b/ext/dom/tests/gh11830/type_variation.phpt @@ -0,0 +1,60 @@ +--TEST-- +GH-11830 (ParentNode methods should perform their checks upfront) - type variation +--EXTENSIONS-- +dom +--FILE-- +loadXML(<< + + + + +XML); + +$testElement = $doc->documentElement->firstElementChild->nextElementSibling->firstElementChild; + +try { + $doc->documentElement->firstElementChild->prepend($testElement, 0); +} catch (\TypeError $e) { + echo $e->getMessage(), "\n"; +} + +try { + $doc->documentElement->firstElementChild->append($testElement, true); +} catch (\TypeError $e) { + echo $e->getMessage(), "\n"; +} + +try { + $doc->documentElement->firstElementChild->before($testElement, null); +} catch (\TypeError $e) { + echo $e->getMessage(), "\n"; +} + +try { + $doc->documentElement->firstElementChild->after($testElement, new stdClass); +} catch (\TypeError $e) { + echo $e->getMessage(), "\n"; +} + +try { + $doc->documentElement->firstElementChild->replaceWith($testElement, []); +} catch (\TypeError $e) { + echo $e->getMessage(), "\n"; +} + +echo $doc->saveXML(); +?> +--EXPECT-- +DOMElement::prepend(): Argument #2 must be of type DOMNode|string, int given +DOMElement::append(): Argument #2 must be of type DOMNode|string, bool given +DOMElement::before(): Argument #2 must be of type DOMNode|string, null given +DOMElement::after(): Argument #2 must be of type DOMNode|string, stdClass given +DOMElement::replaceWith(): Argument #2 must be of type DOMNode|string, array given + + + + +