Store aliased name of trait method

Currently, trait methods are aliased will continue to use the
original function name. In a few places in the codebase, we will
try to look up the actual method name instead. However, this does
not work if an aliased method is used indirectly
(https://bugs.php.net/bug.php?id=69180).

I think it would be better to instead actually change the method
name to the alias. This is in principle easy: We have to allow
function_name to be changed even if op array is otherwise shared
(similar to static_variables). This means we need to addref/release
the function_name separately, but I don't think there is a
performance concern here (especially as everything is usually
interned).

There is a bit of complication in opcache, where we need to make
sure that the function name is released the correct number of times
(interning may overwrite the name in the original op_array, but we
need to release it as many times as the op_array is shared).

Fixes bug #69180.
Fixes bug #74939.
Closes GH-5226.

NEWS

@@ -89,6 +89,9 @@ PHP                                                                        NEWS
     scope). (Nikita)
   . Fixed bug #77325 (ReflectionClassConstant::$class returns wrong class when
     extending). (Nikita)
+  . Fixed bug #69180 (Reflection does not honor trait conflict resolution /
+    method aliasing). (Nikita)
+  . Fixed bug #74939 (Nested traits' aliased methods are lowercased). (Nikita)
 
 - Session:
   . Fixed bug #78624 (session_gc return value for user defined session

Zend/tests/bug65579.phpt

@@ -25,5 +25,5 @@ array(2) {
   [0]=>
   string(10) "testMethod"
   [1]=>
-  string(25) "testmethodfromparenttrait"
+  string(25) "testMethodFromParentTrait"
 }

Zend/zend_API.c

@@ -4228,55 +4228,6 @@ ZEND_API void zend_restore_error_handling(zend_error_handling *saved) /* {{{ */
 }
 /* }}} */
 
-ZEND_API zend_string* zend_find_alias_name(zend_class_entry *ce, zend_string *name) /* {{{ */
-{
-	zend_trait_alias *alias, **alias_ptr;
-
-	if ((alias_ptr = ce->trait_aliases)) {
-		alias = *alias_ptr;
-		while (alias) {
-			if (alias->alias && zend_string_equals_ci(alias->alias, name)) {
-				return alias->alias;
-			}
-			alias_ptr++;
-			alias = *alias_ptr;
-		}
-	}
-
-	return name;
-}
-/* }}} */
-
-ZEND_API zend_string *zend_resolve_method_name(zend_class_entry *ce, zend_function *f) /* {{{ */
-{
-	zend_function *func;
-	HashTable *function_table;
-	zend_string *name;
-
-	if (f->common.type != ZEND_USER_FUNCTION ||
-	    (f->op_array.refcount && *(f->op_array.refcount) < 2) ||
-	    !f->common.scope ||
-	    !f->common.scope->trait_aliases) {
-		return f->common.function_name;
-	}
-
-	function_table = &ce->function_table;
-	ZEND_HASH_FOREACH_STR_KEY_PTR(function_table, name, func) {
-		if (func == f) {
-			if (!name) {
-				return f->common.function_name;
-			}
-			if (ZSTR_LEN(name) == ZSTR_LEN(f->common.function_name) &&
-			    !strncasecmp(ZSTR_VAL(name), ZSTR_VAL(f->common.function_name), ZSTR_LEN(f->common.function_name))) {
-				return f->common.function_name;
-			}
-			return zend_find_alias_name(f->common.scope, name);
-		}
-	} ZEND_HASH_FOREACH_END();
-	return f->common.function_name;
-}
-/* }}} */
-
 ZEND_API ZEND_COLD const char *zend_get_object_type(const zend_class_entry *ce) /* {{{ */
 {
 	if(ce->ce_flags & ZEND_ACC_TRAIT) {

Zend/zend_API.h

@@ -575,9 +575,6 @@ static zend_always_inline int zend_forbid_dynamic_call(const char *func_name)
 	return SUCCESS;
 }
 
-ZEND_API zend_string *zend_find_alias_name(zend_class_entry *ce, zend_string *name);
-ZEND_API zend_string *zend_resolve_method_name(zend_class_entry *ce, zend_function *f);
-
 ZEND_API ZEND_COLD const char *zend_get_object_type(const zend_class_entry *ce);
 
 ZEND_API zend_bool zend_is_iterable(zval *iterable);

Zend/zend_builtin_functions.c

@@ -1058,7 +1058,6 @@ ZEND_FUNCTION(get_class_methods)
 	zend_class_entry *ce = NULL;
 	zend_class_entry *scope;
 	zend_function *mptr;
-	zend_string *key;
 
 	if (zend_parse_parameters(ZEND_NUM_ARGS(), "z", &klass) == FAILURE) {
 		RETURN_THROWS();
@@ -1077,8 +1076,7 @@ ZEND_FUNCTION(get_class_methods)
 	array_init(return_value);
 	scope = zend_get_executed_scope();
 
-	ZEND_HASH_FOREACH_STR_KEY_PTR(&ce->function_table, key, mptr) {
+	ZEND_HASH_FOREACH_PTR(&ce->function_table, mptr) {
-
 		if ((mptr->common.fn_flags & ZEND_ACC_PUBLIC)
 		 || (scope &&
 			 (((mptr->common.fn_flags & ZEND_ACC_PROTECTED) &&
@@ -1086,15 +1084,8 @@ ZEND_FUNCTION(get_class_methods)
 		   || ((mptr->common.fn_flags & ZEND_ACC_PRIVATE) &&
 			   scope == mptr->common.scope)))
 		) {
-			if (mptr->type == ZEND_USER_FUNCTION &&
+			ZVAL_STR_COPY(&method_name, mptr->common.function_name);
-				(!mptr->op_array.refcount || *mptr->op_array.refcount > 1) &&
+			zend_hash_next_index_insert_new(Z_ARRVAL_P(return_value), &method_name);
-				 key && !same_name(key, mptr->common.function_name)) {
-				ZVAL_STR_COPY(&method_name, zend_find_alias_name(mptr->common.scope, key));
-				zend_hash_next_index_insert_new(Z_ARRVAL_P(return_value), &method_name);
-			} else {
-				ZVAL_STR_COPY(&method_name, mptr->common.function_name);
-				zend_hash_next_index_insert_new(Z_ARRVAL_P(return_value), &method_name);
-			}
 		}
 	} ZEND_HASH_FOREACH_END();
 }
@@ -1955,16 +1946,9 @@ ZEND_FUNCTION(debug_print_backtrace)
 		object = (Z_TYPE(call->This) == IS_OBJECT) ? Z_OBJ(call->This) : NULL;
 
 		if (call->func) {
-			zend_string *zend_function_name;
-
 			func = call->func;
-			if (func->common.scope && func->common.scope->trait_aliases) {
+			if (func->common.function_name) {
-				zend_function_name = zend_resolve_method_name(object ? object->ce : func->common.scope, func);
+				function_name = ZSTR_VAL(func->common.function_name);
-			} else {
-				zend_function_name = func->common.function_name;
-			}
-			if (zend_function_name != NULL) {
-				function_name = ZSTR_VAL(zend_function_name);
 			} else {
 				function_name = NULL;
 			}
@@ -2184,11 +2168,7 @@ ZEND_API void zend_fetch_debug_backtrace(zval *return_value, int skip_last, int
 
 		if (call && call->func) {
 			func = call->func;
-			function_name = (func->common.scope &&
+			function_name = func->common.function_name;
-							 func->common.scope->trait_aliases) ?
-				zend_resolve_method_name(
-					(object ? object->ce : func->common.scope), func) :
-				func->common.function_name;
 		} else {
 			func = NULL;
 			function_name = NULL;

Zend/zend_closures.c

@@ -702,6 +702,7 @@ ZEND_API void zend_create_closure(zval *res, zend_function *func, zend_class_ent
 			}
 			memset(ptr, 0, func->op_array.cache_size);
 		}
+		zend_string_addref(closure->func.op_array.function_name);
 		if (closure->func.op_array.refcount) {
 			(*closure->func.op_array.refcount)++;
 		}

Zend/zend_compile.c

@@ -1063,10 +1063,10 @@ ZEND_API void function_add_ref(zend_function *function) /* {{{ */
 			ZEND_MAP_PTR_INIT(op_array->run_time_cache, zend_arena_alloc(&CG(arena), sizeof(void*)));
 			ZEND_MAP_PTR_SET(op_array->run_time_cache, NULL);
 		}
-	} else if (function->type == ZEND_INTERNAL_FUNCTION) {
+	}
-		if (function->common.function_name) {
+
-			zend_string_addref(function->common.function_name);
+	if (function->common.function_name) {
-		}
+		zend_string_addref(function->common.function_name);
 	}
 }
 /* }}} */

Zend/zend_inheritance.c

@@ -115,6 +115,9 @@ static zend_always_inline zend_function *zend_duplicate_function(zend_function *
 		if (func->op_array.refcount) {
 			(*func->op_array.refcount)++;
 		}
+		if (EXPECTED(func->op_array.function_name)) {
+			zend_string_addref(func->op_array.function_name);
+		}
 		if (is_interface
 		 || EXPECTED(!func->op_array.static_variables)) {
 			/* reuse the same op_array structure */
@@ -1577,7 +1580,7 @@ static void zend_add_magic_methods(zend_class_entry* ce, zend_string* mname, zen
 }
 /* }}} */
 
-static void zend_add_trait_method(zend_class_entry *ce, const char *name, zend_string *key, zend_function *fn, HashTable **overridden) /* {{{ */
+static void zend_add_trait_method(zend_class_entry *ce, zend_string *name, zend_string *key, zend_function *fn, HashTable **overridden) /* {{{ */
 {
 	zend_function *existing_fn = NULL;
 	zend_function *new_fn;
@@ -1622,11 +1625,11 @@ static void zend_add_trait_method(zend_class_entry *ce, const char *name, zend_s
 			/* two traits can't define the same non-abstract method */
 #if 1
 			zend_error_noreturn(E_COMPILE_ERROR, "Trait method %s has not been applied, because there are collisions with other trait methods on %s",
-				name, ZSTR_VAL(ce->name));
+				ZSTR_VAL(name), ZSTR_VAL(ce->name));
 #else		/* TODO: better error message */
 			zend_error_noreturn(E_COMPILE_ERROR, "Trait method %s::%s has not been applied as %s::%s, because of collision with %s::%s",
 				ZSTR_VAL(fn->common.scope->name), ZSTR_VAL(fn->common.function_name),
-				ZSTR_VAL(ce->name), name,
+				ZSTR_VAL(ce->name), ZSTR_VAL(name),
 				ZSTR_VAL(existing_fn->common.scope->name), ZSTR_VAL(existing_fn->common.function_name));
 #endif
 		} else {
@@ -1647,6 +1650,9 @@ static void zend_add_trait_method(zend_class_entry *ce, const char *name, zend_s
 		new_fn->op_array.fn_flags |= ZEND_ACC_TRAIT_CLONE;
 		new_fn->op_array.fn_flags &= ~ZEND_ACC_IMMUTABLE;
 	}
+
+	/* Reassign method name, in case it is an alias. */
+	new_fn->common.function_name = name;
 	function_add_ref(new_fn);
 	fn = zend_hash_update_ptr(&ce->function_table, key, new_fn);
 	zend_add_magic_methods(ce, key, fn);
@@ -1695,7 +1701,7 @@ static void zend_traits_copy_functions(zend_string *fnname, zend_function *fn, z
 				}
 
 				lcname = zend_string_tolower(alias->alias);
-				zend_add_trait_method(ce, ZSTR_VAL(alias->alias), lcname, &fn_copy, overridden);
+				zend_add_trait_method(ce, alias->alias, lcname, &fn_copy, overridden);
 				zend_string_release_ex(lcname, 0);
 
 				/* Record the trait from which this alias was resolved. */
@@ -1747,7 +1753,7 @@ static void zend_traits_copy_functions(zend_string *fnname, zend_function *fn, z
 			}
 		}
 
-		zend_add_trait_method(ce, ZSTR_VAL(fn->common.function_name), fnname, &fn_copy, overridden);
+		zend_add_trait_method(ce, fn->common.function_name, fnname, &fn_copy, overridden);
 	}
 }
 /* }}} */

Zend/zend_opcode.c

@@ -447,6 +447,10 @@ ZEND_API void destroy_op_array(zend_op_array *op_array)
 		efree(ZEND_MAP_PTR(op_array->run_time_cache));
 	}
 
+	if (op_array->function_name) {
+		zend_string_release_ex(op_array->function_name, 0);
+	}
+
 	if (!op_array->refcount || --(*op_array->refcount) > 0) {
 		return;
 	}
@@ -476,9 +480,6 @@ ZEND_API void destroy_op_array(zend_op_array *op_array)
 	}
 	efree(op_array->opcodes);
 
-	if (op_array->function_name) {
-		zend_string_release_ex(op_array->function_name, 0);
-	}
 	if (op_array->doc_comment) {
 		zend_string_release_ex(op_array->doc_comment, 0);
 	}

ext/opcache/zend_persist.c

@@ -311,6 +311,16 @@ static void zend_persist_op_array_ex(zend_op_array *op_array, zend_persistent_sc
 		EG(current_execute_data) = orig_execute_data;
 	}
 
+	if (op_array->function_name) {
+		zend_string *old_name = op_array->function_name;
+		zend_accel_store_interned_string(op_array->function_name);
+		/* Remember old function name, so it can be released multiple times if shared. */
+		if (op_array->function_name != old_name
+				&& !zend_shared_alloc_get_xlat_entry(&op_array->function_name)) {
+			zend_shared_alloc_register_xlat_entry(&op_array->function_name, old_name);
+		}
+	}
+
 	if (op_array->scope) {
 		zend_class_entry *scope = zend_shared_alloc_get_xlat_entry(op_array->scope);
 
@@ -337,10 +347,6 @@ static void zend_persist_op_array_ex(zend_op_array *op_array, zend_persistent_sc
 				op_array->literals = zend_shared_alloc_get_xlat_entry(op_array->literals);
 				ZEND_ASSERT(op_array->literals != NULL);
 			}
-			if (op_array->function_name && !IS_ACCEL_INTERNED(op_array->function_name)) {
-				op_array->function_name = zend_shared_alloc_get_xlat_entry(op_array->function_name);
-				ZEND_ASSERT(op_array->function_name != NULL);
-			}
 			if (op_array->filename) {
 				op_array->filename = zend_shared_alloc_get_xlat_entry(op_array->filename);
 				ZEND_ASSERT(op_array->filename != NULL);
@@ -502,10 +508,6 @@ static void zend_persist_op_array_ex(zend_op_array *op_array, zend_persistent_sc
 		ZEND_MAP_PTR_INIT(op_array->run_time_cache, NULL);
 	}
 
-	if (op_array->function_name && !IS_ACCEL_INTERNED(op_array->function_name)) {
-		zend_accel_store_interned_string(op_array->function_name);
-	}
-
 	if (op_array->filename) {
 		/* do not free! PHP has centralized filename storage, compiler will free it */
 		zend_accel_memdup_string(op_array->filename);
@@ -632,6 +634,14 @@ static void zend_persist_class_method(zval *zv)
 		if (op_array->refcount && --(*op_array->refcount) == 0) {
 			efree(op_array->refcount);
 		}
+
+		/* If op_array is shared, the function name refcount is still incremented for each use,
+		 * so we need to release it here. We remembered the original function name in xlat. */
+		zend_string *old_function_name =
+			zend_shared_alloc_get_xlat_entry(&old_op_array->function_name);
+		if (old_function_name) {
+			zend_string_release_ex(old_function_name, 0);
+		}
 		return;
 	}
 	if (ZCG(is_immutable_class)) {

ext/opcache/zend_persist_calc.c

@@ -171,14 +171,18 @@ static void zend_persist_type_calc(zend_type *type)
 
 static void zend_persist_op_array_calc_ex(zend_op_array *op_array)
 {
+	if (op_array->function_name) {
+		zend_string *old_name = op_array->function_name;
+		ADD_INTERNED_STRING(op_array->function_name);
+		/* Remember old function name, so it can be released multiple times if shared. */
+		if (op_array->function_name != old_name
+				&& !zend_shared_alloc_get_xlat_entry(&op_array->function_name)) {
+			zend_shared_alloc_register_xlat_entry(&op_array->function_name, old_name);
+		}
+    }
+
 	if (op_array->scope && zend_shared_alloc_get_xlat_entry(op_array->opcodes)) {
 		/* already stored */
-		if (op_array->function_name) {
-			zend_string *new_name = zend_shared_alloc_get_xlat_entry(op_array->function_name);
-			if (new_name) {
-				op_array->function_name = new_name;
-			}
-		}
 		ADD_SIZE(ZEND_ALIGNED_SIZE(zend_extensions_op_array_persist_calc(op_array)));
 		return;
 	}
@@ -211,16 +215,6 @@ static void zend_persist_op_array_calc_ex(zend_op_array *op_array)
 	zend_shared_alloc_register_xlat_entry(op_array->opcodes, op_array->opcodes);
 	ADD_SIZE(sizeof(zend_op) * op_array->last);
 
-	if (op_array->function_name) {
-		zend_string *old_name = op_array->function_name;
-		if (!zend_shared_alloc_get_xlat_entry(old_name)) {
-			ADD_INTERNED_STRING(op_array->function_name);
-			if (!zend_shared_alloc_get_xlat_entry(op_array->function_name)) {
-				zend_shared_alloc_register_xlat_entry(old_name, op_array->function_name);
-			}
-		}
-    }
-
 	if (op_array->filename) {
 		ADD_STRING(op_array->filename);
 	}
@@ -308,6 +302,14 @@ static void zend_persist_class_method_calc(zval *zv)
 		if (!ZCG(is_immutable_class)) {
 			ADD_ARENA_SIZE(sizeof(void*));
 		}
+	} else {
+		/* If op_array is shared, the function name refcount is still incremented for each use,
+		 * so we need to release it here. We remembered the original function name in xlat. */
+		zend_string *old_function_name =
+			zend_shared_alloc_get_xlat_entry(&old_op_array->function_name);
+		if (old_function_name) {
+			zend_string_release_ex(old_function_name, 0);
+		}
 	}
 }
 

ext/reflection/php_reflection.c

@@ -1210,9 +1210,7 @@ static void reflection_method_factory(zend_class_entry *ce, zend_function *metho
 		ZVAL_OBJ(&intern->obj, Z_OBJ_P(closure_object));
 	}
 
-	ZVAL_STR_COPY(reflection_prop_name(object),
+	ZVAL_STR_COPY(reflection_prop_name(object), method->common.function_name);
-		(method->common.scope && method->common.scope->trait_aliases)
-			? zend_resolve_method_name(ce, method) : method->common.function_name);
 	ZVAL_STR_COPY(reflection_prop_class(object), method->common.scope->name);
 }
 /* }}} */

ext/reflection/tests/bug69180.phpt

@@ -0,0 +1,37 @@
+--TEST--
+Bug #69180: Reflection does not honor trait conflict resolution / method aliasing
+--FILE--
+<?php
+
+trait T1
+{
+    public function foo()
+    {
+    }
+}
+
+trait T2
+{
+    use T1 { foo as bar; }
+
+    public function foo()
+    {
+    }
+}
+
+
+class C
+{
+    use T2;
+}
+
+$class = new ReflectionClass('C');
+
+foreach ($class->getMethods() as $method) {
+    var_dump($method->getName());
+}
+
+?>
+--EXPECT--
+string(3) "foo"
+string(3) "bar"