Skip to content

Commit dddd309

Browse files
nielsdosnielsdos
committed
Fix GH-11830: ParentNode methods should perform their checks upfront
Closes GH-11887.
1 parent 08c4db7 commit dddd309

File tree

6 files changed

+330
-70
lines changed

6 files changed

+330
-70
lines changed

‎NEWS

+2
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,8 @@ PHP NEWS
1818
(nielsdos)
1919
. Fix json_encode result on DOMDocument. (nielsdos)
2020
. Fix manually calling __construct() on DOM classes. (nielsdos)
21+
. Fixed bug GH-11830 (ParentNode methods should perform their checks
22+
upfront). (nielsdos)
2123

2224
- FFI:
2325
. Fix leaking definitions when using FFI::cdef()->new(...). (ilutov)

‎ext/dom/parentnode.c

+79-70
Original file line numberDiff line numberDiff line change
@@ -141,107 +141,84 @@ static bool dom_is_node_in_list(const zval *nodes, int nodesc, const xmlNodePtr
141141
return false;
142142
}
143143

144+
static xmlDocPtr dom_doc_from_context_node(xmlNodePtr contextNode)
145+
{
146+
if (contextNode->type == XML_DOCUMENT_NODE || contextNode->type == XML_HTML_DOCUMENT_NODE) {
147+
return (xmlDocPtr) contextNode;
148+
} else {
149+
return contextNode->doc;
150+
}
151+
}
152+
144153
xmlNode* dom_zvals_to_fragment(php_libxml_ref_obj *document, xmlNode *contextNode, zval *nodes, int nodesc)
145154
{
146155
int i;
147156
xmlDoc *documentNode;
148157
xmlNode *fragment;
149158
xmlNode *newNode;
150-
zend_class_entry *ce;
151159
dom_object *newNodeObj;
152-
int stricterror;
153-
154-
if (document == NULL) {
155-
php_dom_throw_error(HIERARCHY_REQUEST_ERR, 1);
156-
return NULL;
157-
}
158160

159-
if (contextNode->type == XML_DOCUMENT_NODE || contextNode->type == XML_HTML_DOCUMENT_NODE) {
160-
documentNode = (xmlDoc *) contextNode;
161-
} else {
162-
documentNode = contextNode->doc;
163-
}
161+
documentNode = dom_doc_from_context_node(contextNode);
164162

165163
fragment = xmlNewDocFragment(documentNode);
166164

167165
if (!fragment) {
168166
return NULL;
169167
}
170168

171-
stricterror = dom_get_strict_error(document);
172-
173169
for (i = 0; i < nodesc; i++) {
174170
if (Z_TYPE(nodes[i]) == IS_OBJECT) {
175-
ce = Z_OBJCE(nodes[i]);
171+
newNodeObj = Z_DOMOBJ_P(&nodes[i]);
172+
newNode = dom_object_get_node(newNodeObj);
176173

177-
if (instanceof_function(ce, dom_node_class_entry)) {
178-
newNodeObj = Z_DOMOBJ_P(&nodes[i]);
179-
newNode = dom_object_get_node(newNodeObj);
180-
181-
if (newNode->doc != documentNode) {
182-
php_dom_throw_error(WRONG_DOCUMENT_ERR, stricterror);
183-
goto err;
184-
}
174+
if (newNode->parent != NULL) {
175+
xmlUnlinkNode(newNode);
176+
}
185177

186-
if (newNode->parent != NULL) {
187-
xmlUnlinkNode(newNode);
188-
}
178+
newNodeObj->document = document;
179+
xmlSetTreeDoc(newNode, documentNode);
189180

190-
newNodeObj->document = document;
191-
xmlSetTreeDoc(newNode, documentNode);
181+
/* Citing from the docs (https://gnome.pages.gitlab.gnome.org/libxml2/devhelp/libxml2-tree.html#xmlAddChild):
182+
* "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)".
183+
* So we must take a copy if this situation arises to prevent a use-after-free. */
184+
bool will_free = newNode->type == XML_TEXT_NODE && fragment->last && fragment->last->type == XML_TEXT_NODE;
185+
if (will_free) {
186+
newNode = xmlCopyNode(newNode, 1);
187+
}
192188

193-
if (newNode->type == XML_ATTRIBUTE_NODE) {
194-
goto hierarchy_request_err;
189+
if (newNode->type == XML_DOCUMENT_FRAG_NODE) {
190+
/* Unpack document fragment nodes, the behaviour differs for different libxml2 versions. */
191+
newNode = newNode->children;
192+
while (newNode) {
193+
xmlNodePtr next = newNode->next;
194+
xmlUnlinkNode(newNode);
195+
if (!xmlAddChild(fragment, newNode)) {
196+
goto err;
197+
}
198+
newNode = next;
195199
}
196-
197-
/* Citing from the docs (https://gnome.pages.gitlab.gnome.org/libxml2/devhelp/libxml2-tree.html#xmlAddChild):
198-
* "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)".
199-
* So we must take a copy if this situation arises to prevent a use-after-free. */
200-
bool will_free = newNode->type == XML_TEXT_NODE && fragment->last && fragment->last->type == XML_TEXT_NODE;
200+
} else if (!xmlAddChild(fragment, newNode)) {
201201
if (will_free) {
202-
newNode = xmlCopyNode(newNode, 1);
203-
}
204-
205-
if (newNode->type == XML_DOCUMENT_FRAG_NODE) {
206-
/* Unpack document fragment nodes, the behaviour differs for different libxml2 versions. */
207-
newNode = newNode->children;
208-
while (newNode) {
209-
xmlNodePtr next = newNode->next;
210-
xmlUnlinkNode(newNode);
211-
if (!xmlAddChild(fragment, newNode)) {
212-
goto hierarchy_request_err;
213-
}
214-
newNode = next;
215-
}
216-
} else if (!xmlAddChild(fragment, newNode)) {
217-
if (will_free) {
218-
xmlFreeNode(newNode);
219-
}
220-
goto hierarchy_request_err;
202+
xmlFreeNode(newNode);
221203
}
222-
} else {
223-
zend_argument_type_error(i + 1, "must be of type DOMNode|string, %s given", zend_zval_type_name(&nodes[i]));
224204
goto err;
225205
}
226-
} else if (Z_TYPE(nodes[i]) == IS_STRING) {
206+
} else {
207+
ZEND_ASSERT(Z_TYPE(nodes[i]) == IS_STRING);
208+
227209
newNode = xmlNewDocText(documentNode, (xmlChar *) Z_STRVAL(nodes[i]));
228210

229211
xmlSetTreeDoc(newNode, documentNode);
230212

231213
if (!xmlAddChild(fragment, newNode)) {
232214
xmlFreeNode(newNode);
233-
goto hierarchy_request_err;
215+
goto err;
234216
}
235-
} else {
236-
zend_argument_type_error(i + 1, "must be of type DOMNode|string, %s given", zend_zval_type_name(&nodes[i]));
237-
goto err;
238217
}
239218
}
240219

241220
return fragment;
242221

243-
hierarchy_request_err:
244-
php_dom_throw_error(HIERARCHY_REQUEST_ERR, stricterror);
245222
err:
246223
xmlFreeNode(fragment);
247224
return NULL;
@@ -264,17 +241,39 @@ static void dom_fragment_assign_parent_node(xmlNodePtr parentNode, xmlNodePtr fr
264241
fragment->last = NULL;
265242
}
266243

267-
static zend_result dom_hierarchy_node_list(xmlNodePtr parentNode, zval *nodes, int nodesc)
244+
static zend_result dom_sanity_check_node_list_for_insertion(php_libxml_ref_obj *document, xmlNodePtr parentNode, zval *nodes, int nodesc)
268245
{
246+
if (document == NULL) {
247+
php_dom_throw_error(HIERARCHY_REQUEST_ERR, 1);
248+
return FAILURE;
249+
}
250+
251+
xmlDocPtr documentNode = dom_doc_from_context_node(parentNode);
252+
269253
for (int i = 0; i < nodesc; i++) {
270-
if (Z_TYPE(nodes[i]) == IS_OBJECT) {
254+
zend_uchar type = Z_TYPE(nodes[i]);
255+
if (type == IS_OBJECT) {
271256
const zend_class_entry *ce = Z_OBJCE(nodes[i]);
272257

273258
if (instanceof_function(ce, dom_node_class_entry)) {
274-
if (dom_hierarchy(parentNode, dom_object_get_node(Z_DOMOBJ_P(nodes + i))) != SUCCESS) {
259+
xmlNodePtr node = dom_object_get_node(Z_DOMOBJ_P(nodes + i));
260+
261+
if (node->doc != documentNode) {
262+
php_dom_throw_error(WRONG_DOCUMENT_ERR, dom_get_strict_error(document));
275263
return FAILURE;
276264
}
265+
266+
if (node->type == XML_ATTRIBUTE_NODE || dom_hierarchy(parentNode, node) != SUCCESS) {
267+
php_dom_throw_error(HIERARCHY_REQUEST_ERR, dom_get_strict_error(document));
268+
return FAILURE;
269+
}
270+
} else {
271+
zend_argument_type_error(i + 1, "must be of type DOMNode|string, %s given", zend_zval_type_name(&nodes[i]));
272+
return FAILURE;
277273
}
274+
} else if (type != IS_STRING) {
275+
zend_argument_type_error(i + 1, "must be of type DOMNode|string, %s given", zend_zval_type_name(&nodes[i]));
276+
return FAILURE;
278277
}
279278
}
280279

@@ -286,8 +285,7 @@ void dom_parent_node_append(dom_object *context, zval *nodes, int nodesc)
286285
xmlNode *parentNode = dom_object_get_node(context);
287286
xmlNodePtr newchild, prevsib;
288287

289-
if (UNEXPECTED(dom_hierarchy_node_list(parentNode, nodes, nodesc) != SUCCESS)) {
290-
php_dom_throw_error(HIERARCHY_REQUEST_ERR, dom_get_strict_error(context->document));
288+
if (UNEXPECTED(dom_sanity_check_node_list_for_insertion(context->document, parentNode, nodes, nodesc) != SUCCESS)) {
291289
return;
292290
}
293291

@@ -329,8 +327,7 @@ void dom_parent_node_prepend(dom_object *context, zval *nodes, int nodesc)
329327
return;
330328
}
331329

332-
if (UNEXPECTED(dom_hierarchy_node_list(parentNode, nodes, nodesc) != SUCCESS)) {
333-
php_dom_throw_error(HIERARCHY_REQUEST_ERR, dom_get_strict_error(context->document));
330+
if (UNEXPECTED(dom_sanity_check_node_list_for_insertion(context->document, parentNode, nodes, nodesc) != SUCCESS)) {
334331
return;
335332
}
336333

@@ -414,6 +411,10 @@ void dom_parent_node_after(dom_object *context, zval *nodes, int nodesc)
414411

415412
doc = prevsib->doc;
416413

414+
if (UNEXPECTED(dom_sanity_check_node_list_for_insertion(context->document, parentNode, nodes, nodesc) != SUCCESS)) {
415+
return;
416+
}
417+
417418
/* Spec step 4: convert nodes into fragment */
418419
fragment = dom_zvals_to_fragment(context->document, parentNode, nodes, nodesc);
419420

@@ -465,6 +466,10 @@ void dom_parent_node_before(dom_object *context, zval *nodes, int nodesc)
465466

466467
doc = nextsib->doc;
467468

469+
if (UNEXPECTED(dom_sanity_check_node_list_for_insertion(context->document, parentNode, nodes, nodesc) != SUCCESS)) {
470+
return;
471+
}
472+
468473
/* Spec step 4: convert nodes into fragment */
469474
fragment = dom_zvals_to_fragment(context->document, parentNode, nodes, nodesc);
470475

@@ -555,6 +560,10 @@ void dom_child_replace_with(dom_object *context, zval *nodes, int nodesc)
555560

556561
xmlNodePtr insertion_point = child->next;
557562

563+
if (UNEXPECTED(dom_sanity_check_node_list_for_insertion(context->document, parentNode, nodes, nodesc) != SUCCESS)) {
564+
return;
565+
}
566+
558567
xmlNodePtr fragment = dom_zvals_to_fragment(context->document, parentNode, nodes, nodesc);
559568
if (UNEXPECTED(fragment == NULL)) {
560569
return;

‎ext/dom/tests/gh11830/attribute_variation.phpt

+56
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,56 @@
1+
--TEST--
2+
GH-11830 (ParentNode methods should perform their checks upfront) - attribute variation
3+
--EXTENSIONS--
4+
dom
5+
--FILE--
6+
<?php
7+
$doc = new DOMDocument;
8+
$doc->loadXML(<<<XML
9+
<?xml version="1.0"?>
10+
<container x="foo">
11+
<test/>
12+
</container>
13+
XML);
14+
15+
try {
16+
$doc->documentElement->firstElementChild->prepend($doc->documentElement->attributes[0]);
17+
} catch (\DOMException $e) {
18+
echo $e->getMessage(), "\n";
19+
}
20+
21+
try {
22+
$doc->documentElement->firstElementChild->append($doc->documentElement->attributes[0]);
23+
} catch (\DOMException $e) {
24+
echo $e->getMessage(), "\n";
25+
}
26+
27+
try {
28+
$doc->documentElement->firstElementChild->before($doc->documentElement->attributes[0]);
29+
} catch (\DOMException $e) {
30+
echo $e->getMessage(), "\n";
31+
}
32+
33+
try {
34+
$doc->documentElement->firstElementChild->after($doc->documentElement->attributes[0]);
35+
} catch (\DOMException $e) {
36+
echo $e->getMessage(), "\n";
37+
}
38+
39+
try {
40+
$doc->documentElement->firstElementChild->replaceWith($doc->documentElement->attributes[0]);
41+
} catch (\DOMException $e) {
42+
echo $e->getMessage(), "\n";
43+
}
44+
45+
echo $doc->saveXML();
46+
?>
47+
--EXPECT--
48+
Hierarchy Request Error
49+
Hierarchy Request Error
50+
Hierarchy Request Error
51+
Hierarchy Request Error
52+
Hierarchy Request Error
53+
<?xml version="1.0"?>
54+
<container x="foo">
55+
<test/>
56+
</container>

‎ext/dom/tests/gh11830/document_variation.phpt

+71
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,71 @@
1+
--TEST--
2+
GH-11830 (ParentNode methods should perform their checks upfront) - document variation
3+
--EXTENSIONS--
4+
dom
5+
--FILE--
6+
<?php
7+
$otherDoc = new DOMDocument;
8+
$otherDoc->loadXML(<<<XML
9+
<?xml version="1.0"?>
10+
<other/>
11+
XML);
12+
13+
$otherElement = $otherDoc->documentElement;
14+
15+
$doc = new DOMDocument;
16+
$doc->loadXML(<<<XML
17+
<?xml version="1.0"?>
18+
<container>
19+
<alone/>
20+
<child><testelement/></child>
21+
</container>
22+
XML);
23+
24+
$testElement = $doc->documentElement->firstElementChild->nextElementSibling->firstElementChild;
25+
26+
try {
27+
$doc->documentElement->firstElementChild->prepend($testElement, $otherElement);
28+
} catch (\DOMException $e) {
29+
echo $e->getMessage(), "\n";
30+
}
31+
32+
try {
33+
$doc->documentElement->firstElementChild->append($testElement, $otherElement);
34+
} catch (\DOMException $e) {
35+
echo $e->getMessage(), "\n";
36+
}
37+
38+
try {
39+
$doc->documentElement->firstElementChild->before($testElement, $otherElement);
40+
} catch (\DOMException $e) {
41+
echo $e->getMessage(), "\n";
42+
}
43+
44+
try {
45+
$doc->documentElement->firstElementChild->after($testElement, $otherElement);
46+
} catch (\DOMException $e) {
47+
echo $e->getMessage(), "\n";
48+
}
49+
50+
try {
51+
$doc->documentElement->firstElementChild->replaceWith($testElement, $otherElement);
52+
} catch (\DOMException $e) {
53+
echo $e->getMessage(), "\n";
54+
}
55+
56+
echo $otherDoc->saveXML();
57+
echo $doc->saveXML();
58+
?>
59+
--EXPECT--
60+
Wrong Document Error
61+
Wrong Document Error
62+
Wrong Document Error
63+
Wrong Document Error
64+
Wrong Document Error
65+
<?xml version="1.0"?>
66+
<other/>
67+
<?xml version="1.0"?>
68+
<container>
69+
<alone/>
70+
<child><testelement/></child>
71+
</container>

0 commit comments

Comments
 (0)