Skip to content

Commit 5cb38e9

Browse files
ndosschendossche
committed
Fix various document ref pointer mismanagements
- Properly handle attributes - Fix potential NULL dereference if the intern document pointer is NULL Fixes phpGH-16336. Fixes phpGH-16338. Closes phpGH-16345.
1 parent 0932b76 commit 5cb38e9

File tree

7 files changed

+113
-14
lines changed

7 files changed

+113
-14
lines changed

‎NEWS‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,8 @@ PHP NEWS
2222
. Fixed bug GH-16316 (DOMXPath breaks when not initialized properly).
2323
(nielsdos)
2424
. Add missing hierarchy checks to replaceChild. (nielsdos)
25+
. Fixed bug GH-16336 (Attribute intern document mismanagement). (nielsdos)
26+
. Fixed bug GH-16338 (Null-dereference in ext/dom/node.c). (nielsdos)
2527

2628
- EXIF:
2729
. Fixed bug GH-16409 (Segfault in exif_thumbnail when not dealing with a

‎ext/dom/element.c‎

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -642,9 +642,8 @@ PHP_METHOD(DOMElement, setAttributeNode)
642642
xmlUnlinkNode((xmlNodePtr) attrp);
643643
}
644644

645-
if (attrp->doc == NULL && nodep->doc != NULL) {
646-
attrobj->document = intern->document;
647-
php_libxml_increment_doc_ref((php_libxml_node_object *)attrobj, NULL);
645+
if (attrp->doc == NULL && nodep->doc != NULL && intern->document != NULL) {
646+
dom_set_document_ref_pointers_attr(attrp, intern->document);
648647
}
649648

650649
xmlAddChild(nodep, (xmlNodePtr) attrp);

‎ext/dom/node.c‎

Lines changed: 42 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -820,25 +820,56 @@ int dom_node_text_content_write(dom_object *obj, zval *newval)
820820

821821
/* }}} */
822822

823-
/* Returns true if the node was changed, false otherwise. */
824-
static bool dom_set_document_ref_obj_single(xmlNodePtr node, xmlDocPtr doc, php_libxml_ref_obj *document)
823+
/* Returns true if the node had the same document reference, false otherwise. */
824+
static bool dom_set_document_ref_obj_single(xmlNodePtr node, php_libxml_ref_obj *document)
825825
{
826826
dom_object *childobj = php_dom_object_get_data(node);
827-
if (childobj && !childobj->document) {
827+
if (!childobj) {
828+
return true;
829+
}
830+
if (!childobj->document) {
828831
childobj->document = document;
829832
document->refcount++;
830833
return true;
831834
}
832835
return false;
833836
}
834837

838+
void dom_set_document_ref_pointers_attr(xmlAttrPtr attr, php_libxml_ref_obj *document)
839+
{
840+
ZEND_ASSERT(document != NULL);
841+
842+
dom_set_document_ref_obj_single((xmlNodePtr) attr, document);
843+
for (xmlNodePtr attr_child = attr->children; attr_child; attr_child = attr_child->next) {
844+
dom_set_document_ref_obj_single(attr_child, document);
845+
}
846+
}
847+
848+
static bool dom_set_document_ref_pointers_node(xmlNodePtr node, php_libxml_ref_obj *document)
849+
{
850+
ZEND_ASSERT(document != NULL);
851+
852+
if (!dom_set_document_ref_obj_single(node, document)) {
853+
return false;
854+
}
855+
856+
if (node->type == XML_ELEMENT_NODE) {
857+
for (xmlAttrPtr attr = node->properties; attr; attr = attr->next) {
858+
dom_set_document_ref_pointers_attr(attr, document);
859+
}
860+
}
861+
862+
return true;
863+
}
864+
835865
/* TODO: on 8.4 replace the loop with the tree walk helper function. */
836-
static void dom_set_document_pointers(xmlNodePtr node, xmlDocPtr doc, php_libxml_ref_obj *document)
866+
void dom_set_document_ref_pointers(xmlNodePtr node, php_libxml_ref_obj *document)
837867
{
838-
/* Applies the document to the entire subtree. */
839-
xmlSetTreeDoc(node, doc);
868+
if (!document) {
869+
return;
870+
}
840871

841-
if (!dom_set_document_ref_obj_single(node, doc, document)) {
872+
if (!dom_set_document_ref_pointers_node(node, document)) {
842873
return;
843874
}
844875

@@ -847,7 +878,7 @@ static void dom_set_document_pointers(xmlNodePtr node, xmlDocPtr doc, php_libxml
847878
while (node != NULL) {
848879
ZEND_ASSERT(node != base);
849880

850-
if (!dom_set_document_ref_obj_single(node, doc, document)) {
881+
if (!dom_set_document_ref_pointers_node(node, document)) {
851882
break;
852883
}
853884

@@ -974,7 +1005,7 @@ PHP_METHOD(DOMNode, insertBefore)
9741005
}
9751006

9761007
if (child->doc == NULL && parentp->doc != NULL) {
977-
dom_set_document_pointers(child, parentp->doc, intern->document);
1008+
dom_set_document_ref_pointers(child, intern->document);
9781009
}
9791010

9801011
php_libxml_invalidate_node_list_cache(intern->document);
@@ -1137,7 +1168,7 @@ PHP_METHOD(DOMNode, replaceChild)
11371168
}
11381169

11391170
if (newchild->doc == NULL && nodep->doc != NULL) {
1140-
dom_set_document_pointers(newchild, nodep->doc, intern->document);
1171+
dom_set_document_ref_pointers(newchild, intern->document);
11411172
}
11421173

11431174
if (newchild->type == XML_DOCUMENT_FRAG_NODE) {
@@ -1240,7 +1271,7 @@ PHP_METHOD(DOMNode, appendChild)
12401271
}
12411272

12421273
if (child->doc == NULL && nodep->doc != NULL) {
1243-
dom_set_document_pointers(child, nodep->doc, intern->document);
1274+
dom_set_document_ref_pointers(child, intern->document);
12441275
}
12451276

12461277
if (child->parent != NULL){

‎ext/dom/php_dom.h‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -154,6 +154,8 @@ bool php_dom_is_node_connected(const xmlNode *node);
154154
bool php_dom_adopt_node(xmlNodePtr nodep, dom_object *dom_object_new_document, xmlDocPtr new_document);
155155
xmlNsPtr dom_get_ns_resolve_prefix_conflict(xmlNodePtr tree, const char *uri);
156156
xmlEntityPtr dom_entity_reference_fetch_and_sync_declaration(xmlNodePtr reference);
157+
void dom_set_document_ref_pointers(xmlNodePtr node, php_libxml_ref_obj *document);
158+
void dom_set_document_ref_pointers_attr(xmlAttrPtr attr, php_libxml_ref_obj *document);
157159

158160
/* parentnode */
159161
void dom_parent_node_prepend(dom_object *context, zval *nodes, uint32_t nodesc);

‎ext/dom/tests/gh16336_1.phpt‎

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,19 @@
1+
--TEST--
2+
GH-16336 (Attribute intern document mismanagement)
3+
--EXTENSIONS--
4+
dom
5+
--FILE--
6+
<?php
7+
8+
$doc = new DOMDocument();
9+
$elem = new DOMElement("g");
10+
$attr = new DOMAttr("iF", "j");
11+
12+
// First append, then attribute
13+
$doc->appendChild($elem);
14+
$elem->setAttributeNode($attr);
15+
echo $attr->firstChild->textContent;
16+
17+
?>
18+
--EXPECT--
19+
j

‎ext/dom/tests/gh16336_2.phpt‎

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,19 @@
1+
--TEST--
2+
GH-16336 (Attribute intern document mismanagement)
3+
--EXTENSIONS--
4+
dom
5+
--FILE--
6+
<?php
7+
8+
$doc = new DOMDocument();
9+
$elem = new DOMElement("g");
10+
$attr = new DOMAttr("iF", "j");
11+
12+
// First attribute, then append
13+
$elem->setAttributeNode($attr);
14+
$doc->appendChild($elem);
15+
echo $attr->firstChild->textContent;
16+
17+
?>
18+
--EXPECT--
19+
j

‎ext/dom/tests/gh16338.phpt‎

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,27 @@
1+
--TEST--
2+
GH-16338 (Null-dereference in ext/dom/node.c)
3+
--EXTENSIONS--
4+
dom
5+
--CREDITS--
6+
chibinz
7+
--FILE--
8+
<?php
9+
$ref = new DOMEntityReference("G");
10+
$com = new DOMComment();
11+
$doc = new DOMDocument();
12+
$elem = new DOMElement("Rj", "o");
13+
$com2 = new DOMComment();
14+
$elem2 = new DOMElement("kx", null, "r");
15+
16+
$elem2->prepend($com);
17+
$com->before("Z");
18+
$com->before($com2);
19+
$com2->after($elem);
20+
$doc->insertBefore($elem2);
21+
$elem->insertBefore($ref);
22+
23+
echo $doc->saveXML();
24+
?>
25+
--EXPECT--
26+
<?xml version="1.0"?>
27+
<kx xmlns="r">Z<Rj>o&G;</Rj></kx>

0 commit comments

Comments
 (0)