Skip to content

Commit d4a4d2e

Browse files
nielsdosnielsdos
committed
Fix bugs GH-16150 and GH-16152: intern document mismanagement
The reference counts of the internal document pointer are mismanaged. In the case of fragments the refcount may be increased too much, while for other cases the document reference may not be applied to all children. This bug existed for a long time and this doesn't reproduce (easily) on 8.2 due to other bugs. Furthermore 8.2 will enter security mode soon, and this change may be too risky. Fixes GH-16150. Fixed GH-16152. Closes GH-16178.
1 parent 1aeb70f commit d4a4d2e

File tree

4 files changed

+119
-30
lines changed

4 files changed

+119
-30
lines changed

‎NEWS

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,9 @@ PHP NEWS
1919
DOMElement->getAttributeNames()). (nielsdos)
2020
. Fixed bug GH-16151 (Assertion failure in ext/dom/parentnode/tree.c).
2121
(nielsdos)
22+
. Fixed bug GH-16150 (Use after free in php_dom.c). (nielsdos)
23+
. Fixed bug GH-16152 (Memory leak in DOMProcessingInstruction/DOMDocument).
24+
(nielsdos)
2225

2326
- JSON:
2427
. Fixed bug GH-15168 (stack overflow in json_encode()). (nielsdos)

‎ext/dom/node.c

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

821821
/* }}} */
822822

823-
static xmlNodePtr _php_dom_insert_fragment(xmlNodePtr nodep, xmlNodePtr prevsib, xmlNodePtr nextsib, xmlNodePtr fragment, dom_object *intern, dom_object *childobj) /* {{{ */
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)
824825
{
825-
xmlNodePtr newchild, node;
826+
dom_object *childobj = php_dom_object_get_data(node);
827+
if (childobj && !childobj->document) {
828+
childobj->document = document;
829+
document->refcount++;
830+
return true;
831+
}
832+
return false;
833+
}
834+
835+
/* 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)
837+
{
838+
/* Applies the document to the entire subtree. */
839+
xmlSetTreeDoc(node, doc);
840+
841+
if (!dom_set_document_ref_obj_single(node, doc, document)) {
842+
return;
843+
}
844+
845+
xmlNodePtr base = node;
846+
node = node->children;
847+
while (node != NULL) {
848+
ZEND_ASSERT(node != base);
849+
850+
if (!dom_set_document_ref_obj_single(node, doc, document)) {
851+
break;
852+
}
853+
854+
if (node->type == XML_ELEMENT_NODE) {
855+
if (node->children) {
856+
node = node->children;
857+
continue;
858+
}
859+
}
860+
861+
if (node->next) {
862+
node = node->next;
863+
} else {
864+
/* Go upwards, until we find a parent node with a next sibling, or until we hit the base. */
865+
do {
866+
node = node->parent;
867+
if (node == base) {
868+
return;
869+
}
870+
} while (node->next == NULL);
871+
node = node->next;
872+
}
873+
}
874+
}
826875

827-
newchild = fragment->children;
876+
static xmlNodePtr _php_dom_insert_fragment(xmlNodePtr nodep, xmlNodePtr prevsib, xmlNodePtr nextsib, xmlNodePtr fragment, dom_object *intern, dom_object *childobj) /* {{{ */
877+
{
878+
xmlNodePtr newchild = fragment->children;
828879

829880
if (newchild) {
830881
if (prevsib == NULL) {
@@ -840,17 +891,10 @@ static xmlNodePtr _php_dom_insert_fragment(xmlNodePtr nodep, xmlNodePtr prevsib,
840891
nextsib->prev = fragment->last;
841892
}
842893

843-
node = newchild;
894+
/* Assign parent node pointer */
895+
xmlNodePtr node = newchild;
844896
while (node != NULL) {
845897
node->parent = nodep;
846-
if (node->doc != nodep->doc) {
847-
xmlSetTreeDoc(node, nodep->doc);
848-
if (node->_private != NULL) {
849-
childobj = node->_private;
850-
childobj->document = intern->document;
851-
php_libxml_increment_doc_ref((php_libxml_node_object *)childobj, NULL);
852-
}
853-
}
854898
if (node == fragment->last) {
855899
break;
856900
}
@@ -930,8 +974,7 @@ PHP_METHOD(DOMNode, insertBefore)
930974
}
931975

932976
if (child->doc == NULL && parentp->doc != NULL) {
933-
childobj->document = intern->document;
934-
php_libxml_increment_doc_ref((php_libxml_node_object *)childobj, NULL);
977+
dom_set_document_pointers(child, parentp->doc, intern->document);
935978
}
936979

937980
php_libxml_invalidate_node_list_cache(intern->document);
@@ -949,9 +992,6 @@ PHP_METHOD(DOMNode, insertBefore)
949992

950993
if (child->type == XML_TEXT_NODE && (refp->type == XML_TEXT_NODE ||
951994
(refp->prev != NULL && refp->prev->type == XML_TEXT_NODE))) {
952-
if (child->doc == NULL) {
953-
xmlSetTreeDoc(child, parentp->doc);
954-
}
955995
new_child = child;
956996
new_child->parent = refp->parent;
957997
new_child->next = refp;
@@ -1003,9 +1043,6 @@ PHP_METHOD(DOMNode, insertBefore)
10031043
}
10041044
if (child->type == XML_TEXT_NODE && parentp->last != NULL && parentp->last->type == XML_TEXT_NODE) {
10051045
child->parent = parentp;
1006-
if (child->doc == NULL) {
1007-
xmlSetTreeDoc(child, parentp->doc);
1008-
}
10091046
new_child = child;
10101047
if (parentp->children == NULL) {
10111048
parentp->children = child;
@@ -1111,6 +1148,10 @@ PHP_METHOD(DOMNode, replaceChild)
11111148
RETURN_FALSE;
11121149
}
11131150

1151+
if (newchild->doc == NULL && nodep->doc != NULL) {
1152+
dom_set_document_pointers(newchild, nodep->doc, intern->document);
1153+
}
1154+
11141155
if (newchild->type == XML_DOCUMENT_FRAG_NODE) {
11151156
xmlNodePtr prevsib, nextsib;
11161157
prevsib = oldchild->prev;
@@ -1127,11 +1168,6 @@ PHP_METHOD(DOMNode, replaceChild)
11271168
xmlDtdPtr intSubset = xmlGetIntSubset(nodep->doc);
11281169
replacedoctype = (intSubset == (xmlDtd *) oldchild);
11291170

1130-
if (newchild->doc == NULL && nodep->doc != NULL) {
1131-
xmlSetTreeDoc(newchild, nodep->doc);
1132-
newchildobj->document = intern->document;
1133-
php_libxml_increment_doc_ref((php_libxml_node_object *)newchildobj, NULL);
1134-
}
11351171
xmlReplaceNode(oldchild, newchild);
11361172
dom_reconcile_ns(nodep->doc, newchild);
11371173

@@ -1216,8 +1252,7 @@ PHP_METHOD(DOMNode, appendChild)
12161252
}
12171253

12181254
if (child->doc == NULL && nodep->doc != NULL) {
1219-
childobj->document = intern->document;
1220-
php_libxml_increment_doc_ref((php_libxml_node_object *)childobj, NULL);
1255+
dom_set_document_pointers(child, nodep->doc, intern->document);
12211256
}
12221257

12231258
if (child->parent != NULL){
@@ -1226,9 +1261,6 @@ PHP_METHOD(DOMNode, appendChild)
12261261

12271262
if (child->type == XML_TEXT_NODE && nodep->last != NULL && nodep->last->type == XML_TEXT_NODE) {
12281263
child->parent = nodep;
1229-
if (child->doc == NULL) {
1230-
xmlSetTreeDoc(child, nodep->doc);
1231-
}
12321264
new_child = child;
12331265
if (nodep->children == NULL) {
12341266
nodep->children = child;

‎ext/dom/tests/gh16150.phpt

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,27 @@
1+
--TEST--
2+
GH-16150 (Use after free in php_dom.c)
3+
--EXTENSIONS--
4+
dom
5+
--FILE--
6+
<?php
7+
8+
function test($fname) {
9+
$e1 = new DOMElement("E1");
10+
$e2 = new DOMElement("E2");
11+
$e3 = new DOMElement("E3");
12+
$doc = new DOMDocument(); // Must be placed here so it is destroyed first
13+
$doc->{$fname}($e3);
14+
$e2->append($e1);
15+
$e3->{$fname}($e2);
16+
echo $doc->saveXML();
17+
}
18+
19+
test('appendChild');
20+
test('insertBefore');
21+
22+
?>
23+
--EXPECT--
24+
<?xml version="1.0"?>
25+
<E3><E2><E1/></E2></E3>
26+
<?xml version="1.0"?>
27+
<E3><E2><E1/></E2></E3>

‎ext/dom/tests/gh16152.phpt

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,27 @@
1+
--TEST--
2+
GH-16152 (Memory leak in DOMProcessingInstruction/DOMDocument)
3+
--EXTENSIONS--
4+
dom
5+
--FILE--
6+
<?php
7+
8+
function test($fname) {
9+
$doc = new DOMDocument();
10+
$instr = new DOMProcessingInstruction("tr", "r");
11+
$frag = new DOMDocumentFragment();
12+
$frag2 = new DOMDocumentFragment();
13+
$frag2->append($instr);
14+
$frag->append($frag2);
15+
$doc->{$fname}($frag);
16+
echo $doc->saveXML();
17+
}
18+
19+
test('insertBefore');
20+
test('appendChild');
21+
22+
?>
23+
--EXPECT--
24+
<?xml version="1.0"?>
25+
<?tr r?>
26+
<?xml version="1.0"?>
27+
<?tr r?>

0 commit comments

Comments
 (0)