diff --git a/ext/dom/parentnode.c b/ext/dom/parentnode.c index 8467fbf0603a4..df14f194bcb07 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->doc != documentNode) { - php_dom_throw_error(WRONG_DOCUMENT_ERR, stricterror); - goto err; - } + if (newNode->parent != NULL) { + xmlUnlinkNode(newNode); + } - if (newNode->parent != NULL) { - xmlUnlinkNode(newNode); - } + newNodeObj->document = document; + xmlSetTreeDoc(newNode, documentNode); - newNodeObj->document = document; - xmlSetTreeDoc(newNode, documentNode); + /* 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_ATTRIBUTE_NODE) { - goto hierarchy_request_err; + 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; } - - /* 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); - } - - 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; + xmlFreeNode(newNode); } - } 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 0000000000000..34a6f094f58f2 --- /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 0000000000000..89eed3dff22c0 --- /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 0000000000000..bd6534ee71b12 --- /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 0000000000000..76732775e6dbf --- /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 + + + + +