Fix bug #80602 #81642 GH-9142

[NathanFreeman]

https://bugs.php.net/bug.php?id=80602

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

If the number of parameters is greater than or equal to three, xmlNewDocText function always returns a pointer that point to $doc->documentElement->firstChild.

https://bugs.php.net/bug.php?id=81642

#9142

[devnexen]

your fix looks correct but maybe @smalyshev would like to have a look first.

[devnexen]

I think it is a fix worth having, the UAF issue is real if @cmb69 or another dom sensei can look at it, it would be appreciated :-)

[cmb69]

Indeed, that UAF is ugly, but I'm not sure whether this patch wouldn't leak memory. I'll have a closer look ASAP.

[cmb69]

Not really sure what to do here; apparently, the implementation is broken. E.g.

$doc = new \DOMDocument();
$doc->loadXML('<a>foo<last/></a>');
$target = $doc->documentElement->firstChild;
$target->before($target, 'baz', 'cat');
echo $doc->saveXML($doc->documentElement).PHP_EOL;

segfaults with and without the patch. Or the following:

$doc = new \DOMDocument();
$doc->loadXML('<a>foo<last/></a>');
$target = $doc->documentElement->lastChild;
$target->before('baz', 'cat', $target);
echo $doc->saveXML($doc->documentElement).PHP_EOL;

hangs without the patch, and still has a double free with it. But besides that, it outputs <a>foobaz<last/>cat<last/></a>, although that should be <a>foobaz<last/>cat</a>. So besides the memory mismanagement, there is also a logic error, namely that we don't properly detect the `viablePreviousSibling which is supposed to initially be "this’s first preceding sibling not in nodes".

And while it seems to me that doing the xmlCopyNode() is an improvement, I wonder whether we would need to xmlFreeNode(fragment) instead of only xmlFree(fragment).
Disregard that; but still we may properly need to free some nodes.*

[NathanFreeman]

Confusingly, let me see what caused the segment fault.
There are still too few unit tests about libxml extension.
I will try my best to fix such bug.
Thanks.

[NathanFreeman]

Hi, I have a question. @cmb69
What is the output if the code runs successfully? <a><last/>foobazcatfoo</a>?

$doc = new \DOMDocument();
$doc->loadXML('<a>foo<last/></a>');
$target = $doc->documentElement->firstChild;
$target->before($target, 'baz', 'cat');
echo $doc->saveXML($doc->documentElement).PHP_EOL;

[cmb69]

I think that code should output: <a>foobazcat<last/></a>.

[NathanFreeman]

I think that code should output: <a>foobazcat<last/></a>.

Thanks.

[cmb69]

Thank you! On a quick glance, this looks much better. Maybe @beberlei wants to review.

[NathanFreeman]

Thanks.

[devnexen]

I gave a shot at your branch locally, no leaks neither.

[NathanFreeman]

@cmb69 @devnexen @beberlei
Could you please review it?😀😀

[devnexen]

Since it is a bugfix there is still a bit time :-) however, at worse, @cmb69 will have the final word.

[NathanFreeman]

@devnexen @cmb69 @beberlei @smalyshev
Hi!
I have pushed some commit about bug #81642.
It would be greatly appreciated if you could review it.
https://bugs.php.net/bug.php?id=81642

[NathanFreeman]

@beberlei
Hi!
Do you have time to review it now?
You are the only one who can review it😭

[nielsdos]

Thanks for your interest in ext/dom.
One of the fixes here was merged a while ago via a separate PR. And the remaining issues here were fixed by me by modifying the relevant code to follow the DOM spec.
So I'll close this now.