Fix bug #80602 Segfault when using DOMChildNode::before()

[NathanFreeman]

https://bugs.php.net/bug.php?id=80602
#8729
I make a new pr base on PHP-8.1 branch because PHP-8.0 that is supported for critical security issues only.

[NathanFreeman]

cc @beberlei

[nielsdos]

Took me a while to understand all the possible paths in ::before & ::after, but I think your changes are generally correct. I'd just like some comments, especially why you need to do the copy with a reference to the docs. Otherwise, it's quite obscure.

I'm fairly limited in knowledge about this extension in PHP, so I might not be completely correct ofc...

[nielsdos]

We might need a comment here to explain why the copy is necessary, with a reference to the libxml docs talking about the merging of TEXT nodes (in which case our node is freed). I also wonder if we can't check the type of the node to avoid some unnecessary copies.

[NathanFreeman]

For example:

<?php 
declare(strict_types=1);

$doc = new \DOMDocument();
$doc->loadXML('<a>foo<last/></a>');
$target = $doc->documentElement->lastChild;
$target->before('bar', $doc->documentElement->firstChild, 'baz');

After debugging, I found that the xmlNewDocText function will always returns same address to the second parameter if the parameters are greater than or equal to three.
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.
So we must copy the new node to avoid this situation.

[nielsdos]

Thanks for explaining. Could you please add a short comment in the code above the if condition (may be even one line only referencing this PR comment) ?

[NathanFreeman]

I have added comment in the code.

[nielsdos]

Thanks for the changes, I was just busy reviewing and extensively testing this right now. :)

[nielsdos]

Question: as far as I can see, prevsib->next will always equal nextsib. So that means this check and assignment is redundant, no?

[NathanFreeman]

// ext/dom/parentnode.c:170
if (newNode->parent != NULL) {
	xmlUnlinkNode(newNode);
}

xmlUnlinkNode will detach the relation between with parent node and child node.

if (!parentNode->children) {
    nextsib = NULL;
}

It mean first node and child node are both parameters to DOMElement::before() method.
So nextslib is null.

if (beforefirstchild) {
	nextsib = parentNode->children == nextsib ? nextsib : afternextsib;
}

It mean that the new node will insert before $doc->documentElement->firstChild.
Let's discuss two possible scenarios.

1. nextsib does not change means that $doc->documentElement->firstChild is not the parameter to DOMElement::before() method.
2. nextsib = afternextsib means that $doc->documentElement->firstChild is the parameter to DOMElement::before() method and $doc->documentElement->firstChild will removed from node tree.

It means that the new node will insert before $doc->documentElement->lastChild if neither of the above conditions is satisfied.
Let's discuss two possible scenarios too.

1. If parentNode->children == prevsib is true, it means that $doc->documentElement->firstChild is not the parameter to DOMElement::before() method so nextsib = prevsib->next.
   The reason we use prevsib->next is that $doc->documentElement->lastChild may be a parameter to DOMElement::before() and $doc->documentElement->lastChild will removed from node tree.
   In this case, prevsib->next is NULL.

2. If parentNode->children == prevsib is false, it means that $doc->documentElement->firstChild is the parameter to DOMElement::before() method so nextsib does not change.

[nielsdos]

Okay that seems to make sense, thanks for explaining.

[nielsdos]

I'm going to recheck this patch later this week, and run some Valgrind tests on it to check for leaks etc

[nielsdos]

Okay that seems to make sense, thanks for explaining.

[nielsdos]

I've tested the changes with Valgrind, and I've checked the semantics of the checks and it looks correct. Given the fact that the semantics were already (quickly) glanced over in the previous PR and deemed correct also gives me more confidence that this is correct. It's also in line with how other non-special cases behave w.r.t. node detaching.
The code reasoning also seems correct.
Thanks! I'll wait a bit longer to see if other feedback comes in and for the CI, and if it's all good I'll see if I can merge it in the coming week.

[NathanFreeman]

Thank you @nielsdos

[nielsdos]

Merged it! Thanks :)