From ecd0a68cdaba4a7df340a1aceae641c7ed789eb1 Mon Sep 17 00:00:00 2001 From: Niels Dossche <7771979+nielsdos@users.noreply.github.com> Date: Mon, 7 Aug 2023 21:16:33 +0200 Subject: [PATCH 1/3] Fix segfault when DOMParentNode::prepend() is called when the child disappears --- ext/dom/parentnode.c | 6 +++++- ext/dom/tests/gh11906.phpt | 37 +++++++++++++++++++++++++++++++++++++ 2 files changed, 42 insertions(+), 1 deletion(-) create mode 100644 ext/dom/tests/gh11906.phpt diff --git a/ext/dom/parentnode.c b/ext/dom/parentnode.c index 4f3187db96978..2405b8b6f622f 100644 --- a/ext/dom/parentnode.c +++ b/ext/dom/parentnode.c @@ -345,7 +345,11 @@ void dom_parent_node_prepend(dom_object *context, zval *nodes, int nodesc) xmlNodePtr last = fragment->last; parentNode->children = newchild; fragment->last->next = nextsib; - nextsib->prev = last; + /* Note: the first child may be moved into the fragment by dom_zvals_to_fragment(), + * hence the NULL check is needed even though we checked for the null at the start of the function. */ + if (nextsib) { + nextsib->prev = last; + } dom_fragment_assign_parent_node(parentNode, fragment); diff --git a/ext/dom/tests/gh11906.phpt b/ext/dom/tests/gh11906.phpt new file mode 100644 index 0000000000000..b971500290c36 --- /dev/null +++ b/ext/dom/tests/gh11906.phpt @@ -0,0 +1,37 @@ +--TEST-- +GH-11906 (prepend without children after creating fragment results in segfault) +--EXTENSIONS-- +dom +--FILE-- +loadXML(<< + + + +XML); + +$container = $doc->documentElement; +$child = $container->firstElementChild; + +$test = $doc->createElement('foo'); +$test->append($child); +echo $doc->saveXML(); +echo $doc->saveXML($test), "\n"; +$test->prepend($child); +echo $doc->saveXML(); +echo $doc->saveXML($test), "\n"; +$test->append($child); +?> +--EXPECT-- + + + + + + + + + + From 8f8521b8ee97ecdb25385588ba968e710c15ddf6 Mon Sep 17 00:00:00 2001 From: Niels Dossche <7771979+nielsdos@users.noreply.github.com> Date: Mon, 7 Aug 2023 23:10:24 +0200 Subject: [PATCH 2/3] Move dom_pre_insert up in the file --- ext/dom/parentnode.c | 54 ++++++++++++++++++++++---------------------- 1 file changed, 27 insertions(+), 27 deletions(-) diff --git a/ext/dom/parentnode.c b/ext/dom/parentnode.c index 2405b8b6f622f..585d6f41be61a 100644 --- a/ext/dom/parentnode.c +++ b/ext/dom/parentnode.c @@ -280,6 +280,33 @@ static zend_result dom_sanity_check_node_list_for_insertion(php_libxml_ref_obj * return SUCCESS; } +static void dom_pre_insert(xmlNodePtr insertion_point, xmlNodePtr parentNode, xmlNodePtr newchild, xmlNodePtr fragment) +{ + if (!insertion_point) { + /* Place it as last node */ + if (parentNode->children) { + /* There are children */ + newchild->prev = parentNode->last; + parentNode->last->next = newchild; + } else { + /* No children, because they moved out when they became a fragment */ + parentNode->children = newchild; + } + parentNode->last = fragment->last; + } else { + /* Insert fragment before insertion_point */ + fragment->last->next = insertion_point; + if (insertion_point->prev) { + insertion_point->prev->next = newchild; + newchild->prev = insertion_point->prev; + } + insertion_point->prev = fragment->last; + if (parentNode->children == insertion_point) { + parentNode->children = newchild; + } + } +} + void dom_parent_node_append(dom_object *context, zval *nodes, int nodesc) { xmlNode *parentNode = dom_object_get_node(context); @@ -359,33 +386,6 @@ void dom_parent_node_prepend(dom_object *context, zval *nodes, int nodesc) xmlFree(fragment); } -static void dom_pre_insert(xmlNodePtr insertion_point, xmlNodePtr parentNode, xmlNodePtr newchild, xmlNodePtr fragment) -{ - if (!insertion_point) { - /* Place it as last node */ - if (parentNode->children) { - /* There are children */ - newchild->prev = parentNode->last; - parentNode->last->next = newchild; - } else { - /* No children, because they moved out when they became a fragment */ - parentNode->children = newchild; - } - parentNode->last = fragment->last; - } else { - /* Insert fragment before insertion_point */ - fragment->last->next = insertion_point; - if (insertion_point->prev) { - insertion_point->prev->next = newchild; - newchild->prev = insertion_point->prev; - } - insertion_point->prev = fragment->last; - if (parentNode->children == insertion_point) { - parentNode->children = newchild; - } - } -} - void dom_parent_node_after(dom_object *context, zval *nodes, int nodesc) { /* Spec link: https://dom.spec.whatwg.org/#dom-childnode-after */ From a8e1e7760fc55d62876766cfd705bc6d12d4a138 Mon Sep 17 00:00:00 2001 From: Niels Dossche <7771979+nielsdos@users.noreply.github.com> Date: Mon, 7 Aug 2023 23:11:58 +0200 Subject: [PATCH 3/3] Use dom_pre_insert and be done with it --- ext/dom/parentnode.c | 13 +++---------- 1 file changed, 3 insertions(+), 10 deletions(-) diff --git a/ext/dom/parentnode.c b/ext/dom/parentnode.c index 585d6f41be61a..d6b0705545a17 100644 --- a/ext/dom/parentnode.c +++ b/ext/dom/parentnode.c @@ -358,25 +358,18 @@ void dom_parent_node_prepend(dom_object *context, zval *nodes, int nodesc) return; } - xmlNodePtr newchild, nextsib; xmlNode *fragment = dom_zvals_to_fragment(context->document, parentNode, nodes, nodesc); if (fragment == NULL) { return; } - newchild = fragment->children; - nextsib = parentNode->children; + xmlNode *newchild = fragment->children; if (newchild) { xmlNodePtr last = fragment->last; - parentNode->children = newchild; - fragment->last->next = nextsib; - /* Note: the first child may be moved into the fragment by dom_zvals_to_fragment(), - * hence the NULL check is needed even though we checked for the null at the start of the function. */ - if (nextsib) { - nextsib->prev = last; - } + + dom_pre_insert(parentNode->children, parentNode, newchild, fragment); dom_fragment_assign_parent_node(parentNode, fragment);