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

ext/libxml/libxml.c

@@ -777,12 +777,26 @@ static xmlParserInputPtr php_libxml_external_entity_loader(const char *URL,
 
 #undef ADD_NULL_OR_STRING_KEY
 
-	zend_call_known_fcc(&LIBXML(entity_loader_callback), &retval, 3, params, /* named_params */ NULL);
+	/* 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 copy that
+	 * would be a use-after-free of the executing callback. */
+	zend_fcall_info_cache fcc = LIBXML(entity_loader_callback);
+	/* A trampoline (__call/__callStatic) handler is heap-allocated and shared
+	 * with the global fcc, so re-registering would free it too: copy it. */
+	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);
 
 	if (Z_ISUNDEF(retval)) {
 		php_libxml_ctx_error(context,
 				"Call to user entity loader callback '%s' has failed",
-				ZSTR_VAL(LIBXML(entity_loader_callback).function_handler->common.function_name));
+				ZSTR_VAL(fcc.function_handler->common.function_name));
 	} else {
 		if (Z_TYPE(retval) == IS_STRING) {
 is_string:
@@ -791,7 +805,7 @@ static xmlParserInputPtr php_libxml_external_entity_loader(const char *URL,
 			php_stream *stream = (php_stream*)zend_fetch_resource2_ex(&retval, NULL, php_file_le_stream(), php_file_le_pstream());
 			if (UNEXPECTED(stream == NULL)) {
 				zval callable;
-				zend_get_callable_zval_from_fcc(&LIBXML(entity_loader_callback), &callable);
+				zend_get_callable_zval_from_fcc(&fcc, &callable);
 				zend_string *callable_name = zend_get_callable_name(&callable);
 				zend_string *func_name = get_active_function_or_method_name();
 				zend_type_error(
@@ -853,6 +867,7 @@ static xmlParserInputPtr php_libxml_external_entity_loader(const char *URL,
 		}
 	}
 
+	zend_fcc_dtor(&fcc);
 	zval_ptr_dtor(&params[0]);
 	zval_ptr_dtor(&params[1]);
 	zval_ptr_dtor(&params[2]);

ext/libxml/tests/libxml_set_external_entity_loader_callback_bailout.phpt

@@ -0,0 +1,40 @@
+--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

ext/libxml/tests/libxml_set_external_entity_loader_reentrancy.phpt

@@ -0,0 +1,46 @@
+--TEST--
+libxml_set_external_entity_loader(): re-registering the loader from within its own callback is safe (no use-after-free)
+--EXTENSIONS--
+dom
+--FILE--
+<?php
+class Loader {
+    public function load($public, $system, $context) {
+        /* Re-enter the setter while this callback (and its bound $this) is still
+         * on the stack. The bound object is only kept alive by the global loader
+         * fcc, so freeing it here used to trigger a use-after-free. The loader now
+         * holds its own reference for the duration of the call, so this is safe. */
+        libxml_set_external_entity_loader(null);
+        /* $this must still be valid after the re-registration. */
+        var_dump($this instanceof Loader);
+
+        $f = fopen("php://temp", "r+");
+        fwrite($f, "<!ELEMENT foo (#PCDATA)>");
+        rewind($f);
+        return $f;
+    }
+}
+
+$xml = <<<XML
+<!DOCTYPE foo PUBLIC "-//FOO/BAR" "http://example.com/foobar">
+<foo>bar</foo>
+XML;
+
+/* Pass a fresh object as a non-closure callable: after this call returns, the
+ * only reference to it is the one held by the global entity loader. */
+libxml_set_external_entity_loader([new Loader, 'load']);
+
+$dd = new DOMDocument;
+$dd->loadXML($xml);
+var_dump($dd->validate());
+
+/* The callback reset the loader to null while it was running. */
+var_dump(libxml_get_external_entity_loader());
+
+echo "Done.\n";
+?>
+--EXPECT--
+bool(true)
+bool(true)
+NULL
+Done.

ext/libxml/tests/libxml_set_external_entity_loader_swap_in_callback.phpt

@@ -0,0 +1,41 @@
+--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.

ext/libxml/tests/libxml_set_external_entity_loader_trampoline_reentrancy.phpt

@@ -0,0 +1,35 @@
+--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());
+?>
+--EXPECTF--
+bool(true)
+
+Warning: DOMDocument::validate(): I/O warning : failed to load external entity "42" in %s on line %d
+
+Warning: DOMDocument::validate(): Could not load the external subset "http://example.com/foobar" in %s on line %d
+NULL