From: Nikita Popov <nikita.ppv@gmail.com>
Date: Wed, 10 Jun 2020 08:25:50 +0000 (+0200)
Subject: Fix bug #65006
X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=94df2f697fa1434bfca0d729a14abd53f3e2586e;p=php

Fix bug #65006

The "callable name" may be the same for multiple distinct callables.
The code already worked around this for the case of instance methods,
but there are other cases in which callable names clash, such as
the use of self:: reported in the referenced bug.

Rather than trying to generate a unique name for callables, compare
the content of the alfi structures. This is less efficient if there
are many autoload functions, but autoload *registration* does not
need to be particularly efficient.

As a side-effect, this no longer permits unregistering non-callables.
---

diff --git a/NEWS b/NEWS
index 970282d4d7..0d872d739d 100644
--- a/NEWS
+++ b/NEWS
@@ -163,6 +163,8 @@ PHP                                                                        NEWS
 - SPL:
   . Fixed bug #71236 (Second call of spl_autoload_register() does nothing if it
     has no arguments). (Nikita)
+  . Fixed bug #65006 (spl_autoload_register fails with multiple callables using
+    self, same method). (Nikita)
 
 - Standard:
   . Implemented FR #78638 (__PHP_Incomplete_Class should be final). (Laruence)
diff --git a/ext/spl/php_spl.c b/ext/spl/php_spl.c
index 1a555f0ed5..ced58ab20d 100644
--- a/ext/spl/php_spl.c
+++ b/ext/spl/php_spl.c
@@ -369,13 +369,11 @@ PHP_FUNCTION(spl_autoload_extensions)
 typedef struct {
 	zend_function *func_ptr;
 	zend_object *obj;
-	zval closure;
+	zend_object *closure;
 	zend_class_entry *ce;
 } autoload_func_info;
 
-static void autoload_func_info_dtor(zval *element)
-{
-	autoload_func_info *alfi = (autoload_func_info*)Z_PTR_P(element);
+static void autoload_func_info_destroy(autoload_func_info *alfi) {
 	if (alfi->obj) {
 		zend_object_release(alfi->obj);
 	}
@@ -384,12 +382,43 @@ static void autoload_func_info_dtor(zval *element)
 		zend_string_release_ex(alfi->func_ptr->common.function_name, 0);
 		zend_free_trampoline(alfi->func_ptr);
 	}
-	if (!Z_ISUNDEF(alfi->closure)) {
-		zval_ptr_dtor(&alfi->closure);
+	if (alfi->closure) {
+		zend_object_release(alfi->closure);
 	}
 	efree(alfi);
 }
 
+static void autoload_func_info_zval_dtor(zval *element)
+{
+	autoload_func_info_destroy(Z_PTR_P(element));
+}
+
+static autoload_func_info *autoload_func_info_from_fci(
+		zend_fcall_info *fci, zend_fcall_info_cache *fcc) {
+	autoload_func_info *alfi = emalloc(sizeof(autoload_func_info));
+	alfi->ce = fcc->calling_scope;
+	alfi->func_ptr = fcc->function_handler;
+	alfi->obj = fcc->object;
+	if (alfi->obj) {
+		GC_ADDREF(alfi->obj);
+	}
+	if (Z_TYPE(fci->function_name) == IS_OBJECT) {
+		alfi->closure = Z_OBJ(fci->function_name);
+		GC_ADDREF(alfi->closure);
+	} else {
+		alfi->closure = NULL;
+	}
+	return alfi;
+}
+
+static zend_bool autoload_func_info_equals(
+		const autoload_func_info *alfi1, const autoload_func_info *alfi2) {
+	return alfi1->func_ptr == alfi2->func_ptr
+		&& alfi1->obj == alfi2->obj
+		&& alfi1->ce == alfi2->ce
+		&& alfi1->closure == alfi2->closure;
+}
+
 static zend_class_entry *spl_perform_autoload(zend_string *class_name, zend_string *lc_name) {
 	if (!SPL_G(autoload_functions)) {
 		return NULL;
@@ -454,17 +483,29 @@ PHP_FUNCTION(spl_autoload_call)
 		zend_hash_rehash(ht);						        	\
 	} while (0)
 
+static Bucket *spl_find_registered_function(autoload_func_info *find_alfi) {
+	if (!SPL_G(autoload_functions)) {
+		return NULL;
+	}
+
+	autoload_func_info *alfi;
+	ZEND_HASH_FOREACH_PTR(SPL_G(autoload_functions), alfi) {
+		if (autoload_func_info_equals(alfi, find_alfi)) {
+			return _p;
+		}
+	} ZEND_HASH_FOREACH_END();
+	return NULL;
+}
+
 /* {{{ proto bool spl_autoload_register([mixed autoload_function [, bool throw [, bool prepend]]])
  Register given function as autoloader */
 PHP_FUNCTION(spl_autoload_register)
 {
-	zend_string *func_name;
-	zend_string *lc_name;
 	zend_bool do_throw = 1;
 	zend_bool prepend  = 0;
-	autoload_func_info alfi;
 	zend_fcall_info fci = {0};
 	zend_fcall_info_cache fcc;
+	autoload_func_info *alfi;
 
 	ZEND_PARSE_PARAMETERS_START(0, 3)
 		Z_PARAM_OPTIONAL
@@ -480,94 +521,45 @@ PHP_FUNCTION(spl_autoload_register)
 
 	if (!SPL_G(autoload_functions)) {
 		ALLOC_HASHTABLE(SPL_G(autoload_functions));
-		zend_hash_init(SPL_G(autoload_functions), 1, NULL, autoload_func_info_dtor, 0);
+		zend_hash_init(SPL_G(autoload_functions), 1, NULL, autoload_func_info_zval_dtor, 0);
+		/* Initialize as non-packed hash table for prepend functionality. */
+		zend_hash_real_init_mixed(SPL_G(autoload_functions));
 	}
 
 	/* If first arg is not null */
 	if (ZEND_FCI_INITIALIZED(fci)) {
-		alfi.ce = fcc.calling_scope;
-		alfi.func_ptr = fcc.function_handler;
-		alfi.obj = !(alfi.func_ptr->common.fn_flags & ZEND_ACC_STATIC) ? fcc.object : NULL;
-
 		if (fcc.function_handler->type == ZEND_INTERNAL_FUNCTION &&
 			fcc.function_handler->internal_function.handler == zif_spl_autoload_call) {
 			zend_argument_value_error(1, "must not be the spl_autoload_call() function");
 			RETURN_THROWS();
 		}
 
-		/* fci.function_name contains the callable zval */
-		func_name = zend_get_callable_name(&fci.function_name);
-		if (Z_TYPE(fci.function_name) == IS_OBJECT) {
-			ZVAL_COPY(&alfi.closure, &fci.function_name);
-			lc_name = zend_string_alloc(ZSTR_LEN(func_name) + sizeof(uint32_t), 0);
-			zend_str_tolower_copy(ZSTR_VAL(lc_name), ZSTR_VAL(func_name), ZSTR_LEN(func_name));
-			memcpy(ZSTR_VAL(lc_name) + ZSTR_LEN(func_name), &Z_OBJ_HANDLE(fci.function_name), sizeof(uint32_t));
-			ZSTR_VAL(lc_name)[ZSTR_LEN(lc_name)] = '\0';
-		} else {
-			ZVAL_UNDEF(&alfi.closure);
-			/* Skip leading \ */
-			if (ZSTR_VAL(func_name)[0] == '\\') {
-				lc_name = zend_string_alloc(ZSTR_LEN(func_name) - 1, 0);
-				zend_str_tolower_copy(ZSTR_VAL(lc_name), ZSTR_VAL(func_name) + 1, ZSTR_LEN(func_name) - 1);
-			} else {
-				lc_name = zend_string_tolower(func_name);
-			}
-		}
-		zend_string_release(func_name);
-
-		if (zend_hash_exists(SPL_G(autoload_functions), lc_name)) {
-			if (!Z_ISUNDEF(alfi.closure)) {
-				Z_DELREF_P(&alfi.closure);
-			}
-			goto skip;
-		}
-
-		if (alfi.obj) {
-			/* add object id to the hash to ensure uniqueness, for more reference look at bug #40091 */
-			lc_name = zend_string_extend(lc_name, ZSTR_LEN(lc_name) + sizeof(uint32_t), 0);
-			memcpy(ZSTR_VAL(lc_name) + ZSTR_LEN(lc_name) - sizeof(uint32_t), &alfi.obj->handle, sizeof(uint32_t));
-			ZSTR_VAL(lc_name)[ZSTR_LEN(lc_name)] = '\0';
-			GC_ADDREF(alfi.obj);
-		}
-
-		if (UNEXPECTED(alfi.func_ptr == &EG(trampoline))) {
+		alfi = autoload_func_info_from_fci(&fci, &fcc);
+		if (UNEXPECTED(alfi->func_ptr == &EG(trampoline))) {
 			zend_function *copy = emalloc(sizeof(zend_op_array));
 
-			memcpy(copy, alfi.func_ptr, sizeof(zend_op_array));
-			alfi.func_ptr->common.function_name = NULL;
-			alfi.func_ptr = copy;
-		}
-		if (zend_hash_add_mem(SPL_G(autoload_functions), lc_name, &alfi, sizeof(autoload_func_info)) == NULL) {
-			if (alfi.obj && !(alfi.func_ptr->common.fn_flags & ZEND_ACC_STATIC)) {
-				GC_DELREF(alfi.obj);
-			}
-			if (!Z_ISUNDEF(alfi.closure)) {
-				Z_DELREF(alfi.closure);
-			}
-			if (UNEXPECTED(alfi.func_ptr->common.fn_flags & ZEND_ACC_CALL_VIA_TRAMPOLINE)) {
-				zend_string_release_ex(alfi.func_ptr->common.function_name, 0);
-				zend_free_trampoline(alfi.func_ptr);
-			}
-		}
-		if (prepend && SPL_G(autoload_functions)->nNumOfElements > 1) {
-			/* Move the newly created element to the head of the hashtable */
-			HT_MOVE_TAIL_TO_HEAD(SPL_G(autoload_functions));
+			memcpy(copy, alfi->func_ptr, sizeof(zend_op_array));
+			alfi->func_ptr->common.function_name = NULL;
+			alfi->func_ptr = copy;
 		}
-skip:
-		zend_string_release_ex(lc_name, 0);
 	} else {
-		autoload_func_info spl_alfi;
-		spl_alfi.func_ptr = zend_hash_str_find_ptr(
+		alfi = emalloc(sizeof(autoload_func_info));
+		alfi->func_ptr = zend_hash_str_find_ptr(
 			CG(function_table), "spl_autoload", sizeof("spl_autoload") - 1);
-		spl_alfi.obj = NULL;
-		spl_alfi.ce = NULL;
-		ZVAL_UNDEF(&spl_alfi.closure);
-		zend_hash_add_mem(SPL_G(autoload_functions), spl_alfi.func_ptr->common.function_name,
-				&spl_alfi, sizeof(autoload_func_info));
-		if (prepend && SPL_G(autoload_functions)->nNumOfElements > 1) {
-			/* Move the newly created element to the head of the hashtable */
-			HT_MOVE_TAIL_TO_HEAD(SPL_G(autoload_functions));
-		}
+		alfi->obj = NULL;
+		alfi->ce = NULL;
+		alfi->closure = NULL;
+	}
+
+	if (spl_find_registered_function(alfi)) {
+		autoload_func_info_destroy(alfi);
+		RETURN_TRUE;
+	}
+
+	zend_hash_next_index_insert_ptr(SPL_G(autoload_functions), alfi);
+	if (prepend && SPL_G(autoload_functions)->nNumOfElements > 1) {
+		/* Move the newly created element to the head of the hashtable */
+		HT_MOVE_TAIL_TO_HEAD(SPL_G(autoload_functions));
 	}
 
 	RETURN_TRUE;
@@ -577,68 +569,29 @@ skip:
  Unregister given function as autoloader */
 PHP_FUNCTION(spl_autoload_unregister)
 {
-	zend_string *func_name = NULL;
-	char *error = NULL;
-	zend_string *lc_name;
-	zval *zcallable;
-	int success = FAILURE;
-	zend_object *obj_ptr;
+	zend_fcall_info fci;
 	zend_fcall_info_cache fcc;
 
-	if (zend_parse_parameters(ZEND_NUM_ARGS(), "z", &zcallable) == FAILURE) {
+	if (zend_parse_parameters(ZEND_NUM_ARGS(), "f", &fci, &fcc) == FAILURE) {
 		RETURN_THROWS();
 	}
 
-	if (!zend_is_callable_ex(zcallable, NULL, IS_CALLABLE_CHECK_SYNTAX_ONLY, &func_name, &fcc, &error)) {
-		zend_throw_exception_ex(spl_ce_LogicException, 0, "Unable to unregister invalid function (%s)", error);
-		if (error) {
-			efree(error);
-		}
-		if (func_name) {
-			zend_string_release_ex(func_name, 0);
-		}
-		RETURN_FALSE;
-	}
-	obj_ptr = fcc.object;
-	if (error) {
-		efree(error);
-	}
-
-	if (Z_TYPE_P(zcallable) == IS_OBJECT) {
-		lc_name = zend_string_alloc(ZSTR_LEN(func_name) + sizeof(uint32_t), 0);
-		zend_str_tolower_copy(ZSTR_VAL(lc_name), ZSTR_VAL(func_name), ZSTR_LEN(func_name));
-		memcpy(ZSTR_VAL(lc_name) + ZSTR_LEN(func_name), &Z_OBJ_HANDLE_P(zcallable), sizeof(uint32_t));
-		ZSTR_VAL(lc_name)[ZSTR_LEN(lc_name)] = '\0';
-	} else {
-		/* Skip leading \ */
-		if (ZSTR_VAL(func_name)[0] == '\\') {
-			lc_name = zend_string_alloc(ZSTR_LEN(func_name) - 1, 0);
-			zend_str_tolower_copy(ZSTR_VAL(lc_name), ZSTR_VAL(func_name) + 1, ZSTR_LEN(func_name) - 1);
-		} else {
-			lc_name = zend_string_tolower(func_name);
-		}
+	if (zend_string_equals_literal(
+			fcc.function_handler->common.function_name, "spl_autoload_call")) {
+		/* Don't destroy the hash table, as we might be iterating over it right now. */
+		zend_hash_clean(SPL_G(autoload_functions));
+		RETURN_TRUE;
 	}
-	zend_string_release_ex(func_name, 0);
 
-	if (SPL_G(autoload_functions)) {
-		if (zend_string_equals_literal(lc_name, "spl_autoload_call")) {
-			/* Don't destroy the hash table, as we might be iterating over it right now. */
-			zend_hash_clean(SPL_G(autoload_functions));
-			success = SUCCESS;
-		} else {
-			/* remove specific */
-			success = zend_hash_del(SPL_G(autoload_functions), lc_name);
-			if (success != SUCCESS && obj_ptr) {
-				lc_name = zend_string_extend(lc_name, ZSTR_LEN(lc_name) + sizeof(uint32_t), 0);
-				memcpy(ZSTR_VAL(lc_name) + ZSTR_LEN(lc_name) - sizeof(uint32_t), &obj_ptr->handle, sizeof(uint32_t));
-				ZSTR_VAL(lc_name)[ZSTR_LEN(lc_name)] = '\0';
-				success = zend_hash_del(SPL_G(autoload_functions), lc_name);
-			}
-		}
+	autoload_func_info *alfi = autoload_func_info_from_fci(&fci, &fcc);
+	Bucket *p = spl_find_registered_function(alfi);
+	autoload_func_info_destroy(alfi);
+	if (p) {
+		zend_hash_del_bucket(SPL_G(autoload_functions), p);
+		RETURN_TRUE;
 	}
 
-	zend_string_release_ex(lc_name, 0);
-	RETURN_BOOL(success == SUCCESS);
+	RETURN_FALSE;
 } /* }}} */
 
 /* {{{ proto false|array spl_autoload_functions()
@@ -654,9 +607,11 @@ PHP_FUNCTION(spl_autoload_functions)
 	array_init(return_value);
 	if (SPL_G(autoload_functions)) {
 		ZEND_HASH_FOREACH_PTR(SPL_G(autoload_functions), alfi) {
-			if (!Z_ISUNDEF(alfi->closure)) {
-				Z_ADDREF(alfi->closure);
-				add_next_index_zval(return_value, &alfi->closure);
+			if (alfi->closure) {
+				zval obj_zv;
+				ZVAL_OBJ(&obj_zv, alfi->closure);
+				Z_ADDREF(obj_zv);
+				add_next_index_zval(return_value, &obj_zv);
 			} else if (alfi->func_ptr->common.scope) {
 				zval tmp;
 
diff --git a/ext/spl/php_spl.stub.php b/ext/spl/php_spl.stub.php
index a4c6b2ab22..23c858dbb9 100755
--- a/ext/spl/php_spl.stub.php
+++ b/ext/spl/php_spl.stub.php
@@ -18,7 +18,7 @@ function spl_autoload_functions(): array {}
 
 function spl_autoload_register(?callable $autoload_function = null, bool $throw = true, bool $prepend = false): bool {}
 
-function spl_autoload_unregister($autoload_function): bool {}
+function spl_autoload_unregister(callable $autoload_function): bool {}
 
 function spl_classes(): array {}
 
diff --git a/ext/spl/php_spl_arginfo.h b/ext/spl/php_spl_arginfo.h
index abe9392071..419dc965c6 100644
--- a/ext/spl/php_spl_arginfo.h
+++ b/ext/spl/php_spl_arginfo.h
@@ -35,7 +35,7 @@ ZEND_BEGIN_ARG_WITH_RETURN_TYPE_INFO_EX(arginfo_spl_autoload_register, 0, 0, _IS
 ZEND_END_ARG_INFO()
 
 ZEND_BEGIN_ARG_WITH_RETURN_TYPE_INFO_EX(arginfo_spl_autoload_unregister, 0, 1, _IS_BOOL, 0)
-	ZEND_ARG_INFO(0, autoload_function)
+	ZEND_ARG_TYPE_INFO(0, autoload_function, IS_CALLABLE, 0)
 ZEND_END_ARG_INFO()
 
 #define arginfo_spl_classes arginfo_spl_autoload_functions
diff --git a/ext/spl/tests/bug65006.phpt b/ext/spl/tests/bug65006.phpt
new file mode 100644
index 0000000000..1393936c5a
--- /dev/null
+++ b/ext/spl/tests/bug65006.phpt
@@ -0,0 +1,41 @@
+--TEST--
+Bug #65006: spl_autoload_register fails with multiple callables using self, same method
+--FILE--
+<?php
+
+class first {
+	public static function init() {
+		spl_autoload_register(array('self','load'));
+	}
+	public static function load($class) {}
+}
+
+class second {
+	public static function init() {
+		spl_autoload_register(array('self','load'));
+	}
+	public static function load($class){}
+}
+
+first::init();
+second::init();
+var_dump(spl_autoload_functions());
+
+?>
+--EXPECT--
+array(2) {
+  [0]=>
+  array(2) {
+    [0]=>
+    string(5) "first"
+    [1]=>
+    string(4) "load"
+  }
+  [1]=>
+  array(2) {
+    [0]=>
+    string(6) "second"
+    [1]=>
+    string(4) "load"
+  }
+}
diff --git a/ext/spl/tests/spl_autoload_008.phpt b/ext/spl/tests/spl_autoload_008.phpt
index 53cf8d4ece..77b2ab5627 100644
--- a/ext/spl/tests/spl_autoload_008.phpt
+++ b/ext/spl/tests/spl_autoload_008.phpt
@@ -42,18 +42,23 @@ foreach($funcs as $idx => $func)
 {
     echo "====$idx====\n";
 
-    try
-    {
-        var_dump($func);
+    var_dump($func);
+    try {
         spl_autoload_register($func);
-        if (count(spl_autoload_functions()))
-        {
-            echo "registered\n";
+    } catch (TypeError $e) {
+        echo get_class($e) . ': ' . $e->getMessage() . \PHP_EOL;
+        var_dump(count(spl_autoload_functions()));
+        continue;
+    }
 
+    if (count(spl_autoload_functions())) {
+        echo "registered\n";
+
+        try {
             var_dump(class_exists("NoExistingTestClass", true));
+        } catch (Exception $e) {
+            echo get_class($e) . ': ' . $e->getMessage() . \PHP_EOL;
         }
-    } catch(\TypeError|\Exception $e) {
-        echo get_class($e) . ': ' . $e->getMessage() . \PHP_EOL;
     }
 
     spl_autoload_unregister($func);