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

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.

Author: jvoisin

ext/libxml/libxml.c

@@ -777,7 +777,10 @@ static xmlParserInputPtr php_libxml_external_entity_loader(const char *URL,
 
 #undef ADD_NULL_OR_STRING_KEY
 
+	bool old_in_callback = LIBXML(entity_loader_in_callback);
+	LIBXML(entity_loader_in_callback) = true;
 	zend_call_known_fcc(&LIBXML(entity_loader_callback), &retval, 3, params, /* named_params */ NULL);
+	LIBXML(entity_loader_in_callback) = old_in_callback;
 
 	if (Z_ISUNDEF(retval)) {
 		php_libxml_ctx_error(context,
@@ -1030,6 +1033,7 @@ static PHP_RINIT_FUNCTION(libxml)
 	 * do not affect the current request.
 	 */
 	LIBXML(entity_loader_disabled) = false;
+	LIBXML(entity_loader_in_callback) = false;
 
 	return SUCCESS;
 }
@@ -1254,6 +1258,14 @@ PHP_FUNCTION(libxml_set_external_entity_loader)
 		Z_PARAM_FUNC_NO_TRAMPOLINE_FREE_OR_NULL(fci, fcc)
 	ZEND_PARSE_PARAMETERS_END();
 
+	/* Changing the loader while it is running would free the executing
+	 * callback (and its bound object), resulting in a use-after-free. */
+	if (LIBXML(entity_loader_in_callback)) {
+		zend_release_fcall_info_cache(&fcc);
+		zend_throw_error(NULL, "Cannot change the external entity loader while it is being executed");
+		RETURN_THROWS();
+	}
+
 	/* Unset old callback if it's defined */
 	if (ZEND_FCC_INITIALIZED(LIBXML(entity_loader_callback))) {
 		zend_fcc_dtor(&LIBXML(entity_loader_callback));

ext/libxml/php_libxml.h

@@ -45,6 +45,9 @@ ZEND_BEGIN_MODULE_GLOBALS(libxml)
 	zend_llist *error_list;
 	zend_fcall_info_cache entity_loader_callback;
 	bool entity_loader_disabled;
+	/* Set while the user entity loader callback is running, so that the
+	 * callback cannot free itself by re-registering the loader (use-after-free). */
+	bool entity_loader_in_callback;
 ZEND_END_MODULE_GLOBALS(libxml)
 
 typedef struct _libxml_doc_props {

ext/libxml/tests/libxml_set_external_entity_loader_reentrancy.phpt

@@ -0,0 +1,52 @@
+--TEST--
+libxml_set_external_entity_loader(): re-registering the loader from within its own callback is rejected (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. It must throw
+         * instead. */
+        try {
+            libxml_set_external_entity_loader(null);
+        } catch (\Error $e) {
+            echo $e->getMessage(), "\n";
+        }
+        /* $this must still be valid after the rejected 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());
+
+/* Changing the loader is allowed again once the callback is no longer running. */
+libxml_set_external_entity_loader(null);
+var_dump(libxml_get_external_entity_loader());
+
+echo "Done.\n";
+?>
+--EXPECT--
+Cannot change the external entity loader while it is being executed
+bool(true)
+bool(true)
+NULL
+Done.