ext/libxml: reject changing the external entity loader while it is running

[jvoisin]

The loader callback ran with a borrowed $this (non-closures are not ref-held by zend_call_function), so re-registering the loader from within its own callback freed the executing callback and its bound object, causing a use-after-free. Guard it with an in-callback flag and throw instead, as already done in ext/curl, ext/sqlite3 and ext/xml.

[devnexen]

can we have the following tests ?

--TEST--
libxml_set_external_entity_loader(): the in-callback state is reset when the callback bails out
--EXTENSIONS--
dom
--INI--
memory_limit=8M
--FILE--
<?php
register_shutdown_function(function () {
    try {
        libxml_set_external_entity_loader(null);
        echo 'loader reset', PHP_EOL;
    } catch (\Error $e) {
        echo $e::class, ': ', $e->getMessage(), PHP_EOL;
    }
});

$loader = function ($public, $system, $context) {
    echo 'in loader', PHP_EOL;
    $s = str_repeat('a', 1024 * 1024 * 512);
};

libxml_set_external_entity_loader($loader);

$xml = <<<XML
<!DOCTYPE foo PUBLIC "-//FOO/BAR" "http://example.com/foobar">
<foo>bar</foo>
XML;

$dd = new DOMDocument;
$dd->loadXML($xml);
$dd->validate();

echo 'not reached', PHP_EOL;
?>
--EXPECTF--
in loader

Fatal error: Allowed memory size of %d bytes exhausted (tried to allocate %d bytes) in %s on line %d
loader reset

--TEST--
libxml_set_external_entity_loader(): the loader may swap itself out for a nested parse
--EXTENSIONS--
dom
--FILE--
<?php
$loader = null;
$loader = function ($public, $system, $context) use (&$loader) {
    libxml_set_external_entity_loader(null);
    $inner = new DOMDocument;
    $inner->loadXML('<inner/>');
    echo $inner->documentElement->nodeName, PHP_EOL;
    libxml_set_external_entity_loader($loader);

    $f = fopen('php://temp', 'r+');
    fwrite($f, '<!ELEMENT foo (#PCDATA)>');
    rewind($f);
    return $f;
};

libxml_set_external_entity_loader($loader);

$xml = <<<XML
<!DOCTYPE foo PUBLIC "-//FOO/BAR" "http://example.com/foobar">
<foo>bar</foo>
XML;

$dd = new DOMDocument;
$dd->loadXML($xml);
var_dump($dd->validate());

libxml_set_external_entity_loader(null);
var_dump(libxml_get_external_entity_loader());

echo 'Done.', PHP_EOL;
?>
--EXPECT--
inner
bool(true)
NULL
Done.

[jvoisin]

Sure

[devnexen]

Suggested change

	zend_fcall_info_cache fcc;
	/* Hold our own reference to the callback for the duration of the call.
	* The callback may re-register the loader (e.g. to run a nested parse),
	* which frees the global fcc and its bound object; without this dup that
	* would be a use-after-free of the executing callback. */
	- zend_fcall_info_cache fcc;
	- zend_fcc_dup(&fcc, &LIBXML(entity_loader_callback));
	+ zend_fcall_info_cache fcc = LIBXML(entity_loader_callback);
	+ if (UNEXPECTED(fcc.function_handler->common.fn_flags & ZEND_ACC_CALL_VIA_TRAMPOLINE)) {
	+ fcc.function_handler = emalloc(sizeof(zend_function));
	+ memcpy(fcc.function_handler, LIBXML(entity_loader_callback).function_handler, sizeof(zend_function));
	+ zend_string_addref(fcc.function_handler->common.function_name);
	+ }
	+ zend_fcc_addref(&fcc);
	zend_call_known_fcc(&fcc, &retval, 3, params, /* named_params */ NULL);

[devnexen]

here a related regression test

--- /dev/null
+++ b/ext/libxml/tests/libxml_set_external_entity_loader_trampoline_reentrancy.phpt
@@ -0,0 +1,33 @@
+--TEST--
+libxml_set_external_entity_loader(): trampoline callback resetting the loader from within itself
+--EXTENSIONS--
+dom
+--FILE--
+<?php
+class TrampolineTest {
+    public function __call(string $name, array $arguments) {
+        libxml_set_external_entity_loader(null);
+        var_dump($this instanceof TrampolineTest);
+        return 42;
+    }
+}
+
+libxml_set_external_entity_loader([new TrampolineTest, 'entity_loader']);
+
+$dd = new DOMDocument;
+$dd->loadXML(<<<XML
+<!DOCTYPE foo PUBLIC "-//FOO/BAR" "http://example.com/foobar">
+<foo>bar</foo>
+XML);
+try {
+    $dd->validate();
+} catch (\Error $e) {
+    echo $e::class, ': ', $e->getMessage(), PHP_EOL;
+}
+var_dump(libxml_get_external_entity_loader());
+?>
+--EXPECT--
+bool(true)
+TypeError: TrampolineTest::entity_loader(): Return value must be of type string|resource|null, int returned
+NULL