dom: clear the xpath callback registrations before freeing them

[iliaal]

php_dom_xpath_callbacks_dtor() freed php_ns and each namespaces entry but left registry->php_ns and registry->namespaces pointing at the freed memory, and it then destroyed node_list, which runs node destructors. A destructor that calls gc_collect_cycles() reaches php_dom_xpath_callback_ns_get_gc() through those still-set fields and iterates freed memory.

Reachable from userland by calling DOMXPath::__construct() a second time on an object that already has php:function registrations and a populated node list, which is the path at ext/dom/xpath.c that tears the registry down on a live object.

[devnexen]

fix looks fine but is the test succeeding even with USE_ZEND_ALLOC=0 for you ?

[iliaal]

Yes on the patched build: 10 out of 10 through run-tests and 20 out of 20 running the reproducer directly, both with USE_ZEND_ALLOC=0.

Unpatched it is flaky rather than clean. The reproducer aborts about half the time, 11 of 20, on the debug assertion at xpath_callbacks.c:98, while the .phpt itself only trips 1 in 10 that way. Under -m it is a reliable LEAK before and PASS after, so valgrind is the signal I would trust here rather than the plain run.

[devnexen]

would like this one too, it should crash w/o your fix

...
class GcElement extends DOMElement {
    public function __destruct() { gc_collect_cycles(); }
}
class Holder { public $self; public function cb($n) { return true; } }
$doc = new DOMDocument();
$doc->loadXML('<r><a/><b/><c/></r>');
$doc->registerNodeClass(DOMElement::class, GcElement::class);
$xp = new DOMXPath($doc);
$xp->registerNamespace('php', 'http://php.net/xpath');
$h = new Holder(); $h->self = $h;
$xp->registerPhpFunctions(['cb' => [$h, 'cb']]);
$xp->query('/r/*[php:function("cb", .)]');
unset($h);
$tmp = $xp; unset($tmp);
$xp->__construct($doc);
var_dump($xp->query('/r/a')->length);
...

[iliaal]

Added, and it is a much better pin than mine was. With USE_ZEND_ALLOC=0 it fails 10 out of 10 unpatched, 6 assertion aborts and 4 segfaults, where my version only tripped about half the time. The self-referencing Holder is what does it: the fcc keeps the object alive, so the collector actually walks into the freed functions table instead of glancing at it.

Two changes to it. I put USE_ZEND_ALLOC=0 in an --ENV-- section so it pins without needing valgrind, following ext/fileinfo/tests/bug71527.phpt. And I added a sibling using registerPhpFunctionNS(), since both tests otherwise only reach registry->php_ns and the patch also detaches registry->namespaces; that one is 10 out of 10 too. Separate files rather than one, because the second case gets disarmed by the first when they share a process.