From 8e8d5ce2404f95acf5201b1c84f58af6b50e287e Mon Sep 17 00:00:00 2001 From: Niels Dossche <7771979+nielsdos@users.noreply.github.com> Date: Fri, 22 Dec 2023 22:03:36 +0100 Subject: [PATCH 1/2] Fix crash in adoptNode with attribute references I forgot to also update the document reference of attributes, so when there is no document reference anymore from a variable, but still an attribute, this can crash. Fix it by also updating the document references for attributes. Closes GH-13002. --- NEWS | 1 + ext/dom/document.c | 34 +++++++++++++------ ...cument_adoptNode_attribute_references.phpt | 27 +++++++++++++++ 3 files changed, 51 insertions(+), 11 deletions(-) create mode 100644 ext/dom/tests/DOMDocument_adoptNode_attribute_references.phpt diff --git a/NEWS b/NEWS index 1161e854d2b..7f08765f436 100644 --- a/NEWS +++ b/NEWS @@ -21,6 +21,7 @@ PHP NEWS . Fixed bug GH-12870 (Creating an xmlns attribute results in a DOMException). (nielsdos) . Fix crash when toggleAttribute() is used without a document. (nielsdos) + . Fix crash in adoptNode with attribute references. (nielsdos) - FFI: . Fixed bug GH-9698 (stream_wrapper_register crashes with FFI\CData). diff --git a/ext/dom/document.c b/ext/dom/document.c index 417cb70be9d..831f01b87b2 100644 --- a/ext/dom/document.c +++ b/ext/dom/document.c @@ -1041,21 +1041,33 @@ PHP_METHOD(DOMDocument, getElementById) } /* }}} end dom_document_get_element_by_id */ -static void php_dom_transfer_document_ref(xmlNodePtr node, dom_object *dom_object_document, xmlDocPtr document) +static zend_always_inline void php_dom_transfer_document_ref_single_node(xmlNodePtr node, php_libxml_ref_obj *new_document) +{ + php_libxml_node_ptr *iteration_object_ptr = node->_private; + if (iteration_object_ptr) { + php_libxml_node_object *iteration_object = iteration_object_ptr->_private; + ZEND_ASSERT(iteration_object != NULL); + /* Must increase refcount first because we could be the last reference holder, and the document may be equal. */ + new_document->refcount++; + php_libxml_decrement_doc_ref(iteration_object); + iteration_object->document = new_document; + } +} + +static void php_dom_transfer_document_ref(xmlNodePtr node, php_libxml_ref_obj *new_document) { if (node->children) { - php_dom_transfer_document_ref(node->children, dom_object_document, document); + php_dom_transfer_document_ref(node->children, new_document); } + while (node) { - php_libxml_node_ptr *iteration_object_ptr = node->_private; - if (iteration_object_ptr) { - php_libxml_node_object *iteration_object = iteration_object_ptr->_private; - ZEND_ASSERT(iteration_object != NULL); - /* Must increase refcount first because we could be the last reference holder, and the document may be equal. */ - dom_object_document->document->refcount++; - php_libxml_decrement_doc_ref(iteration_object); - iteration_object->document = dom_object_document->document; + if (node->type == XML_ELEMENT_NODE) { + for (xmlAttrPtr attr = node->properties; attr != NULL; attr = attr->next) { + php_dom_transfer_document_ref_single_node((xmlNodePtr) attr, new_document); + } } + + php_dom_transfer_document_ref_single_node(node, new_document); node = node->next; } } @@ -1073,7 +1085,7 @@ bool php_dom_adopt_node(xmlNodePtr nodep, dom_object *dom_object_new_document, x return false; } - php_dom_transfer_document_ref(nodep, dom_object_new_document, new_document); + php_dom_transfer_document_ref(nodep, dom_object_new_document->document); } else { xmlUnlinkNode(nodep); } diff --git a/ext/dom/tests/DOMDocument_adoptNode_attribute_references.phpt b/ext/dom/tests/DOMDocument_adoptNode_attribute_references.phpt new file mode 100644 index 00000000000..fcbf032bbfd --- /dev/null +++ b/ext/dom/tests/DOMDocument_adoptNode_attribute_references.phpt @@ -0,0 +1,27 @@ +--TEST-- +DOMDocument::adoptNode() with attribute references +--EXTENSIONS-- +dom +--FILE-- +appendChild($dom->createElement('root')); +$root->setAttributeNS("urn:a", "a:root1", "bar"); +$root1 = $root->getAttributeNodeNS("urn:a", "root1"); +echo $dom->saveXML(); + +$dom = new DOMDocument; +$dom->appendChild($dom->adoptNode($root)); +foreach ($dom->documentElement->attributes as $attr) { + var_dump($attr->namespaceURI, $attr->prefix, $attr->localName, $attr->nodeValue); +} + +?> +--EXPECT-- + + +string(5) "urn:a" +string(1) "a" +string(5) "root1" +string(3) "bar" From 3fa5af8496bdbc74e24828b790e4acfd6a26f0d4 Mon Sep 17 00:00:00 2001 From: Niels Dossche <7771979+nielsdos@users.noreply.github.com> Date: Fri, 22 Dec 2023 23:24:34 +0100 Subject: [PATCH 2/2] Fix crashes with entity references and predefined entities There's two issues here: - freeing of predefined entity declaration crashes (unique to 8.3 & master) - using multiple entity references for a single entity declaration crashes (since forever) The fix for the last issue is fairly easy to do on 8.3, but may require a slightly different approach on 8.2. Therefore, for now this is 8.3-only. Closes GH-13004. --- NEWS | 1 + .../DOMEntityReference_predefined_free.phpt | 46 +++++++++++++++++++ .../delayed_freeing/entity_declaration.phpt | 22 +++++++-- ext/libxml/libxml.c | 17 +++++-- 4 files changed, 78 insertions(+), 8 deletions(-) create mode 100644 ext/dom/tests/DOMEntityReference_predefined_free.phpt diff --git a/NEWS b/NEWS index 7f08765f436..4cda13adfe4 100644 --- a/NEWS +++ b/NEWS @@ -22,6 +22,7 @@ PHP NEWS (nielsdos) . Fix crash when toggleAttribute() is used without a document. (nielsdos) . Fix crash in adoptNode with attribute references. (nielsdos) + . Fix crashes with entity references and predefined entities. (nielsdos) - FFI: . Fixed bug GH-9698 (stream_wrapper_register crashes with FFI\CData). diff --git a/ext/dom/tests/DOMEntityReference_predefined_free.phpt b/ext/dom/tests/DOMEntityReference_predefined_free.phpt new file mode 100644 index 00000000000..4b971d83703 --- /dev/null +++ b/ext/dom/tests/DOMEntityReference_predefined_free.phpt @@ -0,0 +1,46 @@ +--TEST-- +Freeing of a predefined DOMEntityReference +--EXTENSIONS-- +dom +--FILE-- + +--EXPECT-- +object(DOMEntityReference)#1 (17) { + ["nodeName"]=> + string(3) "amp" + ["nodeValue"]=> + NULL + ["nodeType"]=> + int(5) + ["parentNode"]=> + NULL + ["parentElement"]=> + NULL + ["childNodes"]=> + string(22) "(object value omitted)" + ["firstChild"]=> + string(22) "(object value omitted)" + ["lastChild"]=> + string(22) "(object value omitted)" + ["previousSibling"]=> + NULL + ["nextSibling"]=> + NULL + ["attributes"]=> + NULL + ["isConnected"]=> + bool(false) + ["namespaceURI"]=> + NULL + ["prefix"]=> + string(0) "" + ["localName"]=> + NULL + ["baseURI"]=> + NULL + ["textContent"]=> + string(0) "" +} diff --git a/ext/dom/tests/delayed_freeing/entity_declaration.phpt b/ext/dom/tests/delayed_freeing/entity_declaration.phpt index 3e082611c35..5caf29eedad 100644 --- a/ext/dom/tests/delayed_freeing/entity_declaration.phpt +++ b/ext/dom/tests/delayed_freeing/entity_declaration.phpt @@ -9,16 +9,32 @@ $doc->loadXML(<<<'XML' + ]> XML); -$entity = $doc->doctype->entities[0]; -var_dump($entity->nodeName, $entity->parentNode->nodeName); +$ref1 = $doc->createEntityReference("test"); +$ref2 = $doc->createEntityReference("myimage"); +$entity1 = $doc->doctype->entities[0]; +$entity2 = $doc->doctype->entities[1]; + +// Entity order depends on addresses +if ($entity1->nodeName !== "test") { + [$entity1, $entity2] = [$entity2, $entity1]; +} + +var_dump($entity1->nodeName, $entity1->parentNode->nodeName); +var_dump($entity2->nodeName, $entity2->parentNode->nodeName); $doc->removeChild($doc->doctype); -var_dump($entity->nodeName, $entity->parentNode); +var_dump($entity1->nodeName, $entity1->parentNode); +var_dump($entity2->nodeName, $entity2->parentNode); ?> --EXPECT-- string(4) "test" string(5) "books" +string(7) "myimage" +string(5) "books" string(4) "test" NULL +string(7) "myimage" +NULL diff --git a/ext/libxml/libxml.c b/ext/libxml/libxml.c index 526aa296aad..2eef24d2fff 100644 --- a/ext/libxml/libxml.c +++ b/ext/libxml/libxml.c @@ -206,12 +206,10 @@ static void php_libxml_node_free(xmlNodePtr node) * dtd is attached to the document. This works around the issue by inspecting the parent directly. */ case XML_ENTITY_DECL: { xmlEntityPtr entity = (xmlEntityPtr) node; - php_libxml_unlink_entity_decl(entity); - if (entity->orig != NULL) { - xmlFree((char *) entity->orig); - entity->orig = NULL; + if (entity->etype != XML_INTERNAL_PREDEFINED_ENTITY) { + php_libxml_unlink_entity_decl(entity); + xmlFreeEntity(entity); } - xmlFreeNode(node); break; } case XML_NOTATION_NODE: { @@ -1385,6 +1383,15 @@ PHP_LIBXML_API void php_libxml_node_free_resource(xmlNodePtr node) case XML_DOCUMENT_NODE: case XML_HTML_DOCUMENT_NODE: break; + case XML_ENTITY_REF_NODE: + /* Entity reference nodes are special: their children point to entity declarations, + * but they don't own the declarations and therefore shouldn't free the children. + * Moreover, there can be N>1 reference nodes for a single entity declarations. */ + php_libxml_unregister_node(node); + if (node->parent == NULL) { + php_libxml_node_free(node); + } + break; default: if (node->parent == NULL || node->type == XML_NAMESPACE_DECL) { php_libxml_node_free_list((xmlNodePtr) node->children);