From a21065e6ebd4e9aed4e8be05fa691dfa5b0aa41d Mon Sep 17 00:00:00 2001 From: Ilija Tovilo Date: Mon, 31 Mar 2025 22:06:17 +0200 Subject: [PATCH 1/3] Use-after-free in extract() with EXTR_REFS Fixes GH-18209 Closes GH-18211 --- NEWS | 1 + ext/standard/array.c | 4 +++- ext/standard/tests/gh18209.phpt | 23 +++++++++++++++++++++++ 3 files changed, 27 insertions(+), 1 deletion(-) create mode 100644 ext/standard/tests/gh18209.phpt diff --git a/NEWS b/NEWS index c08d732697cb3..37a320d27be07 100644 --- a/NEWS +++ b/NEWS @@ -9,6 +9,7 @@ PHP NEWS - Standard: . Fixed bug GH-18145 (php8ts crashes in php_clear_stat_cache()). (Jakub Zelenka) + . Fixed bug GH-18209 (Use-after-free in extract() with EXTR_REFS). (ilutov) 10 Apr 2025, PHP 8.3.20 diff --git a/ext/standard/array.c b/ext/standard/array.c index 7382e1e9f8bd9..b89deb241f046 100644 --- a/ext/standard/array.c +++ b/ext/standard/array.c @@ -1863,8 +1863,10 @@ static zend_long php_extract_ref_overwrite(zend_array *arr, zend_array *symbol_t } else { ZVAL_MAKE_REF_EX(entry, 2); } - zval_ptr_dtor(orig_var); + zval garbage; + ZVAL_COPY_VALUE(&garbage, orig_var); ZVAL_REF(orig_var, Z_REF_P(entry)); + zval_ptr_dtor(&garbage); } else { if (Z_ISREF_P(entry)) { Z_ADDREF_P(entry); diff --git a/ext/standard/tests/gh18209.phpt b/ext/standard/tests/gh18209.phpt new file mode 100644 index 0000000000000..6a759639f7dcb --- /dev/null +++ b/ext/standard/tests/gh18209.phpt @@ -0,0 +1,23 @@ +--TEST-- +GH-18209: Use-after-free in extract() with EXTR_REFS +--CREDITS-- +Noam Rathaus (nrathaus) +--FILE-- + 42]; +extract($array, EXTR_REFS); +var_dump($b); + +?> +--EXPECT-- +int(42) +int(43) From 2ede2009671ba9e73900daf26abdee9e05fe9aab Mon Sep 17 00:00:00 2001 From: Ilija Tovilo Date: Tue, 1 Apr 2025 17:11:58 +0200 Subject: [PATCH 2/3] [skip ci] Drop Zend/tests/traits/error_001.phpt The test doesn't test what it says it does. And depending on what it is actually trying to test, it is redundant with either Zend/tests/traits/error_009.phpt, Zend/tests/traits/error_010.phpt or Zend/tests/traits/language015.phpt. --- Zend/tests/traits/error_001.phpt | 28 ---------------------------- 1 file changed, 28 deletions(-) delete mode 100644 Zend/tests/traits/error_001.phpt diff --git a/Zend/tests/traits/error_001.phpt b/Zend/tests/traits/error_001.phpt deleted file mode 100644 index a7889da41e2b7..0000000000000 --- a/Zend/tests/traits/error_001.phpt +++ /dev/null @@ -1,28 +0,0 @@ ---TEST-- -Trying to use instanceof for a method twice ---FILE-- - ---EXPECTF-- -Fatal error: Class A cannot extend trait foo in %s on line %d From 011795bcbebc0d7022d70ded6bf114c3b424ec71 Mon Sep 17 00:00:00 2001 From: Ilija Tovilo Date: Fri, 13 Sep 2024 17:57:04 +0200 Subject: [PATCH 3/3] Bind traits before parent class This more accurately matches the "copy & paste" semantics described in the documentation. Abstract trait methods diverge from this behavior, given that a parent method can satisfy trait methods used in the child. In that case, the method is not copied, but the check is performed after the parent has been bound. Fixes GH-15753 Fixes GH-16198 Close GH-15878 --- NEWS | 1 + UPGRADING | 3 + Zend/tests/traits/constant_015.phpt | 16 +---- Zend/tests/traits/gh15753.phpt | 23 +++++++ Zend/tests/traits/gh16198.phpt | 36 +++++++++++ Zend/tests/traits/gh16198_2.phpt | 27 ++++++++ Zend/zend_inheritance.c | 99 ++++++++++++++--------------- 7 files changed, 141 insertions(+), 64 deletions(-) create mode 100644 Zend/tests/traits/gh15753.phpt create mode 100644 Zend/tests/traits/gh16198.phpt create mode 100644 Zend/tests/traits/gh16198_2.phpt diff --git a/NEWS b/NEWS index 14667511d6651..1a635fc47cef6 100644 --- a/NEWS +++ b/NEWS @@ -41,6 +41,7 @@ PHP NEWS . Added the (void) cast to indicate that not using a value is intentional. (timwolla) . Added get_error_handler(), get_exception_handler() functions. (Arnaud) + . Fixed bug GH-15753 and GH-16198 (Bind traits before parent class). (ilutov) - Curl: . Added curl_multi_get_handles(). (timwolla) diff --git a/UPGRADING b/UPGRADING index 542513a16402b..791660451359a 100644 --- a/UPGRADING +++ b/UPGRADING @@ -41,6 +41,9 @@ PHP 8.5 UPGRADE NOTES . The tick handlers are now deactivated after all shutdown functions, destructors have run and the output handlers have been cleaned up. This is a consequence of fixing GH-18033. + . Traits are now bound before the parent class. This is a subtle behavioral + change, but should closer match user expectations, demonstrated by GH-15753 + and GH-16198. - Intl: . The extension now requires at least ICU 57.1. diff --git a/Zend/tests/traits/constant_015.phpt b/Zend/tests/traits/constant_015.phpt index 24507ea9e59cd..1b219340506b8 100644 --- a/Zend/tests/traits/constant_015.phpt +++ b/Zend/tests/traits/constant_015.phpt @@ -1,5 +1,5 @@ --TEST-- -The same name constant of a trait used in a class that inherits a constant defined in a parent can be defined only if they are compatible. +Final class constant in parent may not be overridden by child through trait --FILE-- --EXPECTF-- -Fatal error: BaseClass2 and TestTrait2 define the same constant (Constant) in the composition of DerivedClass2. However, the definition differs and is considered incompatible. Class was composed in %s on line %d +Fatal error: DerivedClass1::Constant cannot override final constant BaseClass1::Constant in %s on line %d diff --git a/Zend/tests/traits/gh15753.phpt b/Zend/tests/traits/gh15753.phpt new file mode 100644 index 0000000000000..21e32d96c09e3 --- /dev/null +++ b/Zend/tests/traits/gh15753.phpt @@ -0,0 +1,23 @@ +--TEST-- +GH-15753: Trait can override property declared in parent class of used class +--FILE-- +prop); + +?> +--EXPECT-- +int(2) diff --git a/Zend/tests/traits/gh16198.phpt b/Zend/tests/traits/gh16198.phpt new file mode 100644 index 0000000000000..f44926cf5ce24 --- /dev/null +++ b/Zend/tests/traits/gh16198.phpt @@ -0,0 +1,36 @@ +--TEST-- +GH-16198: Incorrect trait constant conflict when declared via trait +--FILE-- + +===DONE=== +--EXPECT-- +===DONE=== diff --git a/Zend/tests/traits/gh16198_2.phpt b/Zend/tests/traits/gh16198_2.phpt new file mode 100644 index 0000000000000..7599ea1a11b49 --- /dev/null +++ b/Zend/tests/traits/gh16198_2.phpt @@ -0,0 +1,27 @@ +--TEST-- +GH-16198: Usage of super in cloned trait method +--FILE-- + +--EXPECT-- +string(13) "P::__destruct" diff --git a/Zend/zend_inheritance.c b/Zend/zend_inheritance.c index bcccf978e2a01..8747e11d151fc 100644 --- a/Zend/zend_inheritance.c +++ b/Zend/zend_inheritance.c @@ -1135,7 +1135,12 @@ static inheritance_status do_inheritance_check_on_method( #define SEPARATE_METHOD() do { \ if ((flags & ZEND_INHERITANCE_LAZY_CHILD_CLONE) \ - && child_scope != ce && child->type == ZEND_USER_FUNCTION) { \ + && child_scope != ce \ + /* Trait methods have already been separated at this point. However, their */ \ + /* scope isn't fixed until after inheritance checks to preserve the scope */ \ + /* in error messages. Skip them here explicitly. */ \ + && !(child_scope->ce_flags & ZEND_ACC_TRAIT) \ + && child->type == ZEND_USER_FUNCTION) { \ /* op_array wasn't duplicated yet */ \ zend_function *new_function = zend_arena_alloc(&CG(arena), sizeof(zend_op_array)); \ memcpy(new_function, child, sizeof(zend_op_array)); \ @@ -2350,7 +2355,6 @@ static void zend_add_trait_method(zend_class_entry *ce, zend_string *name, zend_ { zend_function *existing_fn = NULL; zend_function *new_fn; - bool check_inheritance = false; if ((existing_fn = zend_hash_find_ptr(&ce->function_table, key)) != NULL) { /* if it is the same function with the same visibility and has not been assigned a class scope yet, regardless @@ -2384,8 +2388,6 @@ static void zend_add_trait_method(zend_class_entry *ce, zend_string *name, zend_ ZSTR_VAL(fn->common.scope->name), ZSTR_VAL(fn->common.function_name), ZSTR_VAL(ce->name), ZSTR_VAL(name), ZSTR_VAL(existing_fn->common.scope->name), ZSTR_VAL(existing_fn->common.function_name)); - } else { - check_inheritance = true; } } @@ -2405,19 +2407,6 @@ static void zend_add_trait_method(zend_class_entry *ce, zend_string *name, zend_ function_add_ref(new_fn); fn = zend_hash_update_ptr(&ce->function_table, key, new_fn); zend_add_magic_method(ce, fn, key); - - if (check_inheritance) { - /* Inherited members are overridden by members inserted by traits. - * Check whether the trait method fulfills the inheritance requirements. */ - uint32_t flags = ZEND_INHERITANCE_CHECK_PROTO | ZEND_INHERITANCE_CHECK_VISIBILITY; - if (!(existing_fn->common.scope->ce_flags & ZEND_ACC_TRAIT)) { - flags |= ZEND_INHERITANCE_SET_CHILD_CHANGED |ZEND_INHERITANCE_SET_CHILD_PROTO | - ZEND_INHERITANCE_RESET_CHILD_OVERRIDE; - } - do_inheritance_check_on_method( - fn, fixup_trait_scope(fn, ce), existing_fn, fixup_trait_scope(existing_fn, ce), - ce, NULL, flags); - } } /* }}} */ @@ -2695,7 +2684,7 @@ static void zend_traits_init_trait_structures(zend_class_entry *ce, zend_class_e } /* }}} */ -static void zend_do_traits_method_binding(zend_class_entry *ce, zend_class_entry **traits, HashTable **exclude_tables, zend_class_entry **aliases) /* {{{ */ +static void zend_do_traits_method_binding(zend_class_entry *ce, zend_class_entry **traits, HashTable **exclude_tables, zend_class_entry **aliases, bool verify_abstract, bool *contains_abstract_methods) /* {{{ */ { uint32_t i; zend_string *key; @@ -2706,6 +2695,11 @@ static void zend_do_traits_method_binding(zend_class_entry *ce, zend_class_entry if (traits[i]) { /* copies functions, applies defined aliasing, and excludes unused trait methods */ ZEND_HASH_MAP_FOREACH_STR_KEY_PTR(&traits[i]->function_table, key, fn) { + bool is_abstract = (bool) (fn->common.fn_flags & ZEND_ACC_ABSTRACT); + *contains_abstract_methods |= is_abstract; + if (verify_abstract != is_abstract) { + continue; + } zend_traits_copy_functions(key, fn, ce, exclude_tables[i], aliases); } ZEND_HASH_FOREACH_END(); @@ -2720,15 +2714,16 @@ static void zend_do_traits_method_binding(zend_class_entry *ce, zend_class_entry for (i = 0; i < ce->num_traits; i++) { if (traits[i]) { ZEND_HASH_MAP_FOREACH_STR_KEY_PTR(&traits[i]->function_table, key, fn) { + bool is_abstract = (bool) (fn->common.fn_flags & ZEND_ACC_ABSTRACT); + *contains_abstract_methods |= is_abstract; + if (verify_abstract != is_abstract) { + continue; + } zend_traits_copy_functions(key, fn, ce, NULL, aliases); } ZEND_HASH_FOREACH_END(); } } } - - ZEND_HASH_MAP_FOREACH_PTR(&ce->function_table, fn) { - zend_fixup_trait_method(fn, ce); - } ZEND_HASH_FOREACH_END(); } /* }}} */ @@ -3010,33 +3005,6 @@ static void zend_do_traits_property_binding(zend_class_entry *ce, zend_class_ent } /* }}} */ -static void zend_do_bind_traits(zend_class_entry *ce, zend_class_entry **traits) /* {{{ */ -{ - HashTable **exclude_tables; - zend_class_entry **aliases; - - ZEND_ASSERT(ce->num_traits > 0); - - /* complete initialization of trait structures in ce */ - zend_traits_init_trait_structures(ce, traits, &exclude_tables, &aliases); - - /* first care about all methods to be flattened into the class */ - zend_do_traits_method_binding(ce, traits, exclude_tables, aliases); - - if (aliases) { - efree(aliases); - } - - if (exclude_tables) { - efree(exclude_tables); - } - - /* then flatten the constants and properties into it, to, mostly to notify developer about problems */ - zend_do_traits_constant_binding(ce, traits); - zend_do_traits_property_binding(ce, traits); -} -/* }}} */ - #define MAX_ABSTRACT_INFO_CNT 3 #define MAX_ABSTRACT_INFO_FMT "%s%s%s%s" #define DISPLAY_ABSTRACT_FN(idx) \ @@ -3649,6 +3617,15 @@ ZEND_API zend_class_entry *zend_do_link_class(zend_class_entry *ce, zend_string zend_link_hooked_object_iter(ce); #endif + HashTable **trait_exclude_tables; + zend_class_entry **trait_aliases; + bool trait_contains_abstract_methods = false; + if (ce->num_traits) { + zend_traits_init_trait_structures(ce, traits_and_interfaces, &trait_exclude_tables, &trait_aliases); + zend_do_traits_method_binding(ce, traits_and_interfaces, trait_exclude_tables, trait_aliases, false, &trait_contains_abstract_methods); + zend_do_traits_constant_binding(ce, traits_and_interfaces); + zend_do_traits_property_binding(ce, traits_and_interfaces); + } if (parent) { if (!(parent->ce_flags & ZEND_ACC_LINKED)) { add_dependency_obligation(ce, parent); @@ -3656,7 +3633,29 @@ ZEND_API zend_class_entry *zend_do_link_class(zend_class_entry *ce, zend_string zend_do_inheritance(ce, parent); } if (ce->num_traits) { - zend_do_bind_traits(ce, traits_and_interfaces); + if (trait_contains_abstract_methods) { + zend_do_traits_method_binding(ce, traits_and_interfaces, trait_exclude_tables, trait_aliases, true, &trait_contains_abstract_methods); + } + + if (trait_exclude_tables) { + for (i = 0; i < ce->num_traits; i++) { + if (traits_and_interfaces[i]) { + if (trait_exclude_tables[i]) { + zend_hash_destroy(trait_exclude_tables[i]); + FREE_HASHTABLE(trait_exclude_tables[i]); + } + } + } + efree(trait_exclude_tables); + } + if (trait_aliases) { + efree(trait_aliases); + } + + zend_function *fn; + ZEND_HASH_MAP_FOREACH_PTR(&ce->function_table, fn) { + zend_fixup_trait_method(fn, ce); + } ZEND_HASH_FOREACH_END(); } if (ce->num_interfaces) { /* Also copy the parent interfaces here, so we don't need to reallocate later. */