Skip to content

Commit cba335d

Browse files
nielsdosnielsdos
committed
Fix GH-11288 and GH-11289 and GH-11290 and GH-9142: DOMExceptions and segfaults with replaceWith
This replaces the implementation of before and after with one following the spec very strictly, instead of trying to figure out the state we're in by looking at the pointers. Also relaxes the condition on text node copying to prevent working on a stale node pointer. Closes GH-11299.
1 parent 8946b7b commit cba335d

10 files changed

+490
-173
lines changed

‎NEWS

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,10 @@ PHP NEWS
22
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
33
?? ??? ????, PHP 8.1.21
44

5+
- DOM:
6+
. Fixed bugs GH-11288 and GH-11289 and GH-11290 and GH-9142 (DOMExceptions
7+
and segfaults with replaceWith). (nielsdos)
8+
59
- Opcache:
610
. Fix allocation loop in zend_shared_alloc_startup(). (nielsdos)
711
. Access violation on smm_shared_globals with ALLOC_FALLBACK. (KoudelkaB)

‎ext/dom/parentnode.c

Lines changed: 103 additions & 85 deletions
Original file line numberDiff line numberDiff line change
@@ -124,6 +124,23 @@ int dom_parent_node_child_element_count(dom_object *obj, zval *retval)
124124
}
125125
/* }}} */
126126

127+
static bool dom_is_node_in_list(const zval *nodes, int nodesc, const xmlNodePtr node_to_find)
128+
{
129+
for (int i = 0; i < nodesc; i++) {
130+
if (Z_TYPE(nodes[i]) == IS_OBJECT) {
131+
const zend_class_entry *ce = Z_OBJCE(nodes[i]);
132+
133+
if (instanceof_function(ce, dom_node_class_entry)) {
134+
if (dom_object_get_node(Z_DOMOBJ_P(nodes + i)) == node_to_find) {
135+
return true;
136+
}
137+
}
138+
}
139+
}
140+
141+
return false;
142+
}
143+
127144
xmlNode* dom_zvals_to_fragment(php_libxml_ref_obj *document, xmlNode *contextNode, zval *nodes, int nodesc)
128145
{
129146
int i;
@@ -177,17 +194,16 @@ xmlNode* dom_zvals_to_fragment(php_libxml_ref_obj *document, xmlNode *contextNod
177194
goto hierarchy_request_err;
178195
}
179196

180-
/*
181-
* xmlNewDocText function will always returns same address to the second parameter if the parameters are greater than or equal to three.
182-
* If it's text, that's fine, but if it's an object, it can cause invalid pointer because many new nodes point to the same memory address.
183-
* So we must copy the new node to avoid this situation.
184-
*/
185-
if (nodesc > 1) {
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;
201+
if (will_free) {
186202
newNode = xmlCopyNode(newNode, 1);
187203
}
188204

189205
if (!xmlAddChild(fragment, newNode)) {
190-
if (nodesc > 1) {
206+
if (will_free) {
191207
xmlFreeNode(newNode);
192208
}
193209
goto hierarchy_request_err;
@@ -303,25 +319,64 @@ void dom_parent_node_prepend(dom_object *context, zval *nodes, int nodesc)
303319
xmlFree(fragment);
304320
}
305321

322+
static void dom_pre_insert(xmlNodePtr insertion_point, xmlNodePtr parentNode, xmlNodePtr newchild, xmlNodePtr fragment)
323+
{
324+
if (!insertion_point) {
325+
/* Place it as last node */
326+
if (parentNode->children) {
327+
/* There are children */
328+
fragment->last->prev = parentNode->last;
329+
newchild->prev = parentNode->last->prev;
330+
parentNode->last->next = newchild;
331+
} else {
332+
/* No children, because they moved out when they became a fragment */
333+
parentNode->children = newchild;
334+
parentNode->last = newchild;
335+
}
336+
} else {
337+
/* Insert fragment before insertion_point */
338+
fragment->last->next = insertion_point;
339+
if (insertion_point->prev) {
340+
insertion_point->prev->next = newchild;
341+
newchild->prev = insertion_point->prev;
342+
}
343+
insertion_point->prev = newchild;
344+
if (parentNode->children == insertion_point) {
345+
parentNode->children = newchild;
346+
}
347+
}
348+
}
349+
306350
void dom_parent_node_after(dom_object *context, zval *nodes, int nodesc)
307351
{
352+
/* Spec link: https://dom.spec.whatwg.org/#dom-childnode-after */
353+
308354
xmlNode *prevsib = dom_object_get_node(context);
309355
xmlNodePtr newchild, parentNode;
310-
xmlNode *fragment, *nextsib;
356+
xmlNode *fragment;
311357
xmlDoc *doc;
312-
bool afterlastchild;
313-
314-
int stricterror = dom_get_strict_error(context->document);
315358

316-
if (!prevsib->parent) {
317-
php_dom_throw_error(NO_MODIFICATION_ALLOWED_ERR, stricterror);
359+
/* Spec step 1 */
360+
parentNode = prevsib->parent;
361+
/* Spec step 2 */
362+
if (!parentNode) {
363+
int stricterror = dom_get_strict_error(context->document);
364+
php_dom_throw_error(HIERARCHY_REQUEST_ERR, stricterror);
318365
return;
319366
}
320367

368+
/* Spec step 3: find first following child not in nodes; otherwise null */
369+
xmlNodePtr viable_next_sibling = prevsib->next;
370+
while (viable_next_sibling) {
371+
if (!dom_is_node_in_list(nodes, nodesc, viable_next_sibling)) {
372+
break;
373+
}
374+
viable_next_sibling = viable_next_sibling->next;
375+
}
376+
321377
doc = prevsib->doc;
322-
parentNode = prevsib->parent;
323-
nextsib = prevsib->next;
324-
afterlastchild = (nextsib == NULL);
378+
379+
/* Spec step 4: convert nodes into fragment */
325380
fragment = dom_zvals_to_fragment(context->document, parentNode, nodes, nodesc);
326381

327382
if (fragment == NULL) {
@@ -331,40 +386,9 @@ void dom_parent_node_after(dom_object *context, zval *nodes, int nodesc)
331386
newchild = fragment->children;
332387

333388
if (newchild) {
334-
/* first node and last node are both both parameters to DOMElement::after() method so nextsib and prevsib are null. */
335-
if (!parentNode->children) {
336-
prevsib = nextsib = NULL;
337-
} else if (afterlastchild) {
338-
/*
339-
* The new node will be inserted after last node, prevsib is last node.
340-
* The first node is the parameter to DOMElement::after() if parentNode->children == prevsib is true
341-
* and prevsib does not change, otherwise prevsib is parentNode->last (first node).
342-
*/
343-
prevsib = parentNode->children == prevsib ? prevsib : parentNode->last;
344-
} else {
345-
/*
346-
* The new node will be inserted after first node, prevsib is first node.
347-
* The first node is not the parameter to DOMElement::after() if parentNode->children == prevsib is true
348-
* and prevsib does not change otherwise prevsib is null to mean that parentNode->children is the new node.
349-
*/
350-
prevsib = parentNode->children == prevsib ? prevsib : NULL;
351-
}
352-
353-
if (prevsib) {
354-
fragment->last->next = prevsib->next;
355-
if (prevsib->next) {
356-
prevsib->next->prev = fragment->last;
357-
}
358-
prevsib->next = newchild;
359-
} else {
360-
parentNode->children = newchild;
361-
if (nextsib) {
362-
fragment->last->next = nextsib;
363-
nextsib->prev = fragment->last;
364-
}
365-
}
389+
/* Step 5: place fragment into the parent before viable_next_sibling */
390+
dom_pre_insert(viable_next_sibling, parentNode, newchild, fragment);
366391

367-
newchild->prev = prevsib;
368392
dom_fragment_assign_parent_node(parentNode, fragment);
369393
dom_reconcile_ns(doc, newchild);
370394
}
@@ -374,17 +398,34 @@ void dom_parent_node_after(dom_object *context, zval *nodes, int nodesc)
374398

375399
void dom_parent_node_before(dom_object *context, zval *nodes, int nodesc)
376400
{
401+
/* Spec link: https://dom.spec.whatwg.org/#dom-childnode-before */
402+
377403
xmlNode *nextsib = dom_object_get_node(context);
378-
xmlNodePtr newchild, prevsib, parentNode;
379-
xmlNode *fragment, *afternextsib;
404+
xmlNodePtr newchild, parentNode;
405+
xmlNode *fragment;
380406
xmlDoc *doc;
381-
bool beforefirstchild;
382407

383-
doc = nextsib->doc;
384-
prevsib = nextsib->prev;
385-
afternextsib = nextsib->next;
408+
/* Spec step 1 */
386409
parentNode = nextsib->parent;
387-
beforefirstchild = !prevsib;
410+
/* Spec step 2 */
411+
if (!parentNode) {
412+
int stricterror = dom_get_strict_error(context->document);
413+
php_dom_throw_error(HIERARCHY_REQUEST_ERR, stricterror);
414+
return;
415+
}
416+
417+
/* Spec step 3: find first following child not in nodes; otherwise null */
418+
xmlNodePtr viable_previous_sibling = nextsib->prev;
419+
while (viable_previous_sibling) {
420+
if (!dom_is_node_in_list(nodes, nodesc, viable_previous_sibling)) {
421+
break;
422+
}
423+
viable_previous_sibling = viable_previous_sibling->prev;
424+
}
425+
426+
doc = nextsib->doc;
427+
428+
/* Spec step 4: convert nodes into fragment */
388429
fragment = dom_zvals_to_fragment(context->document, parentNode, nodes, nodesc);
389430

390431
if (fragment == NULL) {
@@ -394,37 +435,14 @@ void dom_parent_node_before(dom_object *context, zval *nodes, int nodesc)
394435
newchild = fragment->children;
395436

396437
if (newchild) {
397-
/* first node and last node are both both parameters to DOMElement::before() method so nextsib is null. */
398-
if (!parentNode->children) {
399-
nextsib = NULL;
400-
} else if (beforefirstchild) {
401-
/*
402-
* The new node will be inserted before first node, nextsib is first node and afternextsib is last node.
403-
* The first node is not the parameter to DOMElement::before() if parentNode->children == nextsib is true
404-
* and nextsib does not change, otherwise nextsib is the last node.
405-
*/
406-
nextsib = parentNode->children == nextsib ? nextsib : afternextsib;
407-
} else {
408-
/*
409-
* The new node will be inserted before last node, prevsib is first node and nestsib is last node.
410-
* The first node is not the parameter to DOMElement::before() if parentNode->children == prevsib is true
411-
* but last node may be, so use prevsib->next to determine the value of nextsib, otherwise nextsib does not change.
412-
*/
413-
nextsib = parentNode->children == prevsib ? prevsib->next : nextsib;
414-
}
415-
416-
if (parentNode->children == nextsib) {
417-
parentNode->children = newchild;
438+
/* Step 5: if viable_previous_sibling is null, set it to the parent's first child, otherwise viable_previous_sibling's next sibling */
439+
if (!viable_previous_sibling) {
440+
viable_previous_sibling = parentNode->children;
418441
} else {
419-
prevsib->next = newchild;
420-
}
421-
422-
fragment->last->next = nextsib;
423-
if (nextsib) {
424-
nextsib->prev = fragment->last;
442+
viable_previous_sibling = viable_previous_sibling->next;
425443
}
426-
427-
newchild->prev = prevsib;
444+
/* Step 6: place fragment into the parent after viable_previous_sibling */
445+
dom_pre_insert(viable_previous_sibling, parentNode, newchild, fragment);
428446

429447
dom_fragment_assign_parent_node(parentNode, fragment);
430448
dom_reconcile_ns(doc, newchild);

0 commit comments

Comments
 (0)