From 53efa1b0c69b463ea9d3606d828c036129f2dec9 Mon Sep 17 00:00:00 2001 From: Nikita Popov Date: Mon, 2 Mar 2020 11:07:57 +0100 Subject: [PATCH] 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 | 3 ++ Zend/tests/bug65579.phpt | 2 +- Zend/zend_API.c | 49 ------------------------------ Zend/zend_API.h | 3 -- Zend/zend_builtin_functions.c | 32 ++++--------------- Zend/zend_closures.c | 1 + Zend/zend_compile.c | 8 ++--- Zend/zend_inheritance.c | 16 +++++++--- Zend/zend_opcode.c | 7 +++-- ext/opcache/zend_persist.c | 26 +++++++++++----- ext/opcache/zend_persist_calc.c | 34 +++++++++++---------- ext/reflection/php_reflection.c | 4 +-- ext/reflection/tests/bug69180.phpt | 37 ++++++++++++++++++++++ 13 files changed, 104 insertions(+), 118 deletions(-) create mode 100644 ext/reflection/tests/bug69180.phpt diff --git a/NEWS b/NEWS index 99430555be..5b47d27c89 100644 --- 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 25d74ed4f5..e5ed632416 100644 --- 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 74afc47474..50a1d434db 100644 --- 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 4c29a0f1b9..0e93159167 100644 --- 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 66cf80d64b..0ba6c67f9d 100644 --- 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 06a54f943c..02df3504cc 100644 --- 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 0abf274dbd..fc5b92f09c 100644 --- 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 c875427a70..eb79aab098 100644 --- 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 345e8cce42..49dedf5f96 100644 --- 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 1bcec7caf0..6a158e73f9 100644 --- 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 293af3b3e6..5de27b9efb 100644 --- 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 05a016b97c..94f1b1659d 100644 --- 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 index 0000000000..80d69dcd5c --- /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-- +getMethods() as $method) { + var_dump($method->getName()); +} + +?> +--EXPECT-- +string(3) "foo" +string(3) "bar" -- 2.40.0