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.

diff --git a/NEWS b/NEWS
index 99430555bee24145e6f12535df7c9e9cd0676b8a..5b47d27c89f941289ed31eebd8cb9902514c710e 100644 (file)
--- a/NEWS
+++ b/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

diff --git a/Zend/tests/bug65579.phpt b/Zend/tests/bug65579.phpt
index 25d74ed4f5ee4783a9afda3f45cd64b2c85a88af..e5ed632416da91106b1dc16b84127aa0d66e3405 100644 (file)
--- a/Zend/tests/bug65579.phpt
+++ b/Zend/tests/bug65579.phpt
@@ -25,5 +25,5 @@ array(2) {
   [0]=>
   string(10) "testMethod"
   [1]=>
-  string(25) "testmethodfromparenttrait"
+  string(25) "testMethodFromParentTrait"
 }

diff --git a/Zend/zend_API.c b/Zend/zend_API.c
index 74afc4747454022f1d912f8a873f0b8369721db9..50a1d434dbddbf7bbbef6c63f967ea4627a30fac 100644 (file)
--- a/Zend/zend_API.c
+++ b/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) {

diff --git a/Zend/zend_API.h b/Zend/zend_API.h
index 4c29a0f1b928c6245381036452df631049796add..0e9315916780f3c13749dd1f0a90d5dea8992554 100644 (file)
--- a/Zend/zend_API.h
+++ b/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);

diff --git a/Zend/zend_builtin_functions.c b/Zend/zend_builtin_functions.c
index 66cf80d64b6a27a23ea3db096a29de69a019ee24..0ba6c67f9d601941aaf7a5c1ebfad7966db49f9b 100644 (file)
--- a/Zend/zend_builtin_functions.c
+++ b/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 &&
-                               (!mptr->op_array.refcount || *mptr->op_array.refcount > 1) &&
-                                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);
-                       }
+                       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) {
-                               zend_function_name = zend_resolve_method_name(object ? object->ce : func->common.scope, func);
-                       } else {
-                               zend_function_name = func->common.function_name;
-                       }
-                       if (zend_function_name != NULL) {
-                               function_name = ZSTR_VAL(zend_function_name);
+                       if (func->common.function_name) {
+                               function_name = ZSTR_VAL(func->common.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 &&
-                                                        func->common.scope->trait_aliases) ?
-                               zend_resolve_method_name(
-                                       (object ? object->ce : func->common.scope), func) :
-                               func->common.function_name;
+                       function_name = func->common.function_name;
                } else {
                        func = NULL;
                        function_name = NULL;

diff --git a/Zend/zend_closures.c b/Zend/zend_closures.c
index 06a54f943c97c1b6472408c2d2c683c7ae8f1699..02df3504ccf16be231f0422c70ca74df896541d3 100644 (file)
--- a/Zend/zend_closures.c
+++ b/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)++;
                }

diff --git a/Zend/zend_compile.c b/Zend/zend_compile.c
index 0abf274dbd25ef55db76928909aac924df1752dd..fc5b92f09cf193cbb2b05e9ee4b755cb8a2acbbb 100644 (file)
--- a/Zend/zend_compile.c
+++ b/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);
        }
 }
 /* }}} */

diff --git a/Zend/zend_inheritance.c b/Zend/zend_inheritance.c
index c875427a70a55c951b907a2bda58198d1b43bd13..eb79aab0985d8f69302bf543460dfb7f80d9cf47 100644 (file)
--- a/Zend/zend_inheritance.c
+++ b/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);
        }
 }
 /* }}} */

diff --git a/Zend/zend_opcode.c b/Zend/zend_opcode.c
index 345e8cce42ea94f8f72790856f606a858a23a8b0..49dedf5f963e4e7e4c229482287fc3619d432846 100644 (file)
--- a/Zend/zend_opcode.c
+++ b/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);
        }

diff --git a/ext/opcache/zend_persist.c b/ext/opcache/zend_persist.c
index 1bcec7caf09deab1ef4e336bac8ba33d6d5b359c..6a158e73f9e330215aebe91ad2d94d8e8c9ec6c5 100644 (file)
--- a/ext/opcache/zend_persist.c
+++ b/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)) {

diff --git a/ext/opcache/zend_persist_calc.c b/ext/opcache/zend_persist_calc.c
index 293af3b3e63f15f772d8fd46f2c598729f619b31..5de27b9efb0064407112c7b13cbed7a38d6a5af1 100644 (file)
--- a/ext/opcache/zend_persist_calc.c
+++ b/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);
+               }
        }
 }
 

diff --git a/ext/reflection/php_reflection.c b/ext/reflection/php_reflection.c
index 05a016b97cff74bbe805286834c7e85a6904cc61..94f1b1659dfe0c9b06bf0c06b483c917658c8a23 100644 (file)
--- a/ext/reflection/php_reflection.c
+++ b/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),
-               (method->common.scope && method->common.scope->trait_aliases)
-                       ? zend_resolve_method_name(ce, method) : method->common.function_name);
+       ZVAL_STR_COPY(reflection_prop_name(object), method->common.function_name);
        ZVAL_STR_COPY(reflection_prop_class(object), method->common.scope->name);
 }
 /* }}} */

diff --git a/ext/reflection/tests/bug69180.phpt b/ext/reflection/tests/bug69180.phpt
new file mode 100644 (file)
index 0000000..80d69dc
--- /dev/null
+++ b/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"