From 02a80c5b826ba86d2b315d5babfa340230ab20c6 Mon Sep 17 00:00:00 2001 From: George Peter Banyard Date: Mon, 14 Aug 2023 22:34:56 +0100 Subject: [PATCH] Fix various bugs related to DNF types - GH-11958: DNF types in trait properties do not get bound properly - GH-11883: Memory leak in zend_type_release() for non-arena allocated DNF types - Internal trait bound to userland class would not be arena allocated - Property DNF types were not properly deep copied during lazy loading Co-authored-by: Ilija Tovilo Co-authored-by: ju1ius --- NEWS | 10 +++- .../variance/invalid_invariance1_var.phpt | 18 +++++++ .../internal_trait_use_typed_union.phpt | 39 ++++++++++++++ Zend/zend_inheritance.c | 53 +++++++++++-------- Zend/zend_opcode.c | 12 +---- ext/zend_test/test.stub.php | 1 + ext/zend_test/test_arginfo.h | 15 +++++- 7 files changed, 113 insertions(+), 35 deletions(-) create mode 100644 Zend/tests/type_declarations/dnf_types/variance/invalid_invariance1_var.phpt create mode 100644 Zend/tests/type_declarations/union_types/internal_trait_use_typed_union.phpt diff --git a/NEWS b/NEWS index 1d657909a34..8d8884ef775 100644 --- a/NEWS +++ b/NEWS @@ -14,6 +14,14 @@ PHP NEWS - Core: . Fixed strerror_r detection at configuration time. (Kévin Dunglas) + . Fixed trait typed properties using a DNF type not being correctly bound. + (Girgias) + . Fixed trait property types not being arena allocated if copied from + an internal trait. (Girgias) + . Fixed deep copy of property DNF type during lazy class load. + (Girgias, ilutov) + . Fixed memory freeing of DNF types for non arena allocated types. + (Girgias, ju1ius) - DOM: . Fix DOMEntity field getter bugs. (nielsdos) @@ -156,7 +164,7 @@ PHP NEWS - Standard: . Fix serialization of RC1 objects appearing in object graph twice. (ilutov) - + - Streams: . Fixed bug GH-11735 (Use-after-free when unregistering user stream wrapper from itself). (ilutov) diff --git a/Zend/tests/type_declarations/dnf_types/variance/invalid_invariance1_var.phpt b/Zend/tests/type_declarations/dnf_types/variance/invalid_invariance1_var.phpt new file mode 100644 index 00000000000..1a8a31f066e --- /dev/null +++ b/Zend/tests/type_declarations/dnf_types/variance/invalid_invariance1_var.phpt @@ -0,0 +1,18 @@ +--TEST-- +Property types must be invariant +--FILE-- + +--EXPECTF-- +Fatal error: Type of B::$prop must be (X&Y&Z)|L (as in class A) in %s on line %d diff --git a/Zend/tests/type_declarations/union_types/internal_trait_use_typed_union.phpt b/Zend/tests/type_declarations/union_types/internal_trait_use_typed_union.phpt new file mode 100644 index 00000000000..62bbefc8f5a --- /dev/null +++ b/Zend/tests/type_declarations/union_types/internal_trait_use_typed_union.phpt @@ -0,0 +1,39 @@ +--TEST-- +Internal trait used typed property (union type) +--EXTENSIONS-- +zend_test +--FILE-- +getType(); +$types = $union->getTypes(); +var_dump($types, (string)$types[0], (string)$types[1]); + +?> +===DONE=== +--EXPECT-- +object(C)#1 (1) { + ["testProp"]=> + NULL + ["classUnionProp"]=> + uninitialized(Traversable|Countable) +} +array(2) { + [0]=> + object(ReflectionNamedType)#4 (0) { + } + [1]=> + object(ReflectionNamedType)#5 (0) { + } +} +string(11) "Traversable" +string(9) "Countable" +===DONE=== diff --git a/Zend/zend_inheritance.c b/Zend/zend_inheritance.c index 6b168a8b411..b34823569fc 100644 --- a/Zend/zend_inheritance.c +++ b/Zend/zend_inheritance.c @@ -57,20 +57,33 @@ static void ZEND_COLD emit_incompatible_method_error( const zend_function *parent, zend_class_entry *parent_scope, inheritance_status status); -static void zend_type_copy_ctor(zend_type *type, bool persistent) { - if (ZEND_TYPE_HAS_LIST(*type)) { - zend_type_list *old_list = ZEND_TYPE_LIST(*type); - size_t size = ZEND_TYPE_LIST_SIZE(old_list->num_types); - zend_type_list *new_list = ZEND_TYPE_USES_ARENA(*type) - ? zend_arena_alloc(&CG(arena), size) : pemalloc(size, persistent); - memcpy(new_list, old_list, ZEND_TYPE_LIST_SIZE(old_list->num_types)); - ZEND_TYPE_SET_PTR(*type, new_list); +static void zend_type_copy_ctor(zend_type *const type, bool use_arena, bool persistent); - zend_type *list_type; - ZEND_TYPE_LIST_FOREACH(new_list, list_type) { - ZEND_ASSERT(ZEND_TYPE_HAS_NAME(*list_type)); - zend_string_addref(ZEND_TYPE_NAME(*list_type)); - } ZEND_TYPE_LIST_FOREACH_END(); +static void zend_type_list_copy_ctor( + zend_type *const parent_type, + bool use_arena, + bool persistent +) { + const zend_type_list *const old_list = ZEND_TYPE_LIST(*parent_type); + size_t size = ZEND_TYPE_LIST_SIZE(old_list->num_types); + zend_type_list *new_list = use_arena + ? zend_arena_alloc(&CG(arena), size) : pemalloc(size, persistent); + + memcpy(new_list, old_list, size); + ZEND_TYPE_SET_LIST(*parent_type, new_list); + if (use_arena) { + ZEND_TYPE_FULL_MASK(*parent_type) |= _ZEND_TYPE_ARENA_BIT; + } + + zend_type *list_type; + ZEND_TYPE_LIST_FOREACH(new_list, list_type) { + zend_type_copy_ctor(list_type, use_arena, persistent); + } ZEND_TYPE_LIST_FOREACH_END(); +} + +static void zend_type_copy_ctor(zend_type *const type, bool use_arena, bool persistent) { + if (ZEND_TYPE_HAS_LIST(*type)) { + zend_type_list_copy_ctor(type, use_arena, persistent); } else if (ZEND_TYPE_HAS_NAME(*type)) { zend_string_addref(ZEND_TYPE_NAME(*type)); } @@ -2401,7 +2414,8 @@ static void zend_do_traits_property_binding(zend_class_entry *ce, zend_class_ent doc_comment = property_info->doc_comment ? zend_string_copy(property_info->doc_comment) : NULL; zend_type type = property_info->type; - zend_type_copy_ctor(&type, /* persistent */ 0); + /* Assumption: only userland classes can use traits, as such the type must be arena allocated */ + zend_type_copy_ctor(&type, /* use arena */ true, /* persistent */ false); new_prop = zend_declare_typed_property(ce, prop_name, prop_value, flags, doc_comment, type); if (property_info->attributes) { @@ -2789,15 +2803,8 @@ static zend_class_entry *zend_lazy_class_load(zend_class_entry *pce) Z_PTR(p->val) = new_prop_info; memcpy(new_prop_info, prop_info, sizeof(zend_property_info)); new_prop_info->ce = ce; - if (ZEND_TYPE_HAS_LIST(new_prop_info->type)) { - zend_type_list *new_list; - zend_type_list *list = ZEND_TYPE_LIST(new_prop_info->type); - - new_list = zend_arena_alloc(&CG(arena), ZEND_TYPE_LIST_SIZE(list->num_types)); - memcpy(new_list, list, ZEND_TYPE_LIST_SIZE(list->num_types)); - ZEND_TYPE_SET_PTR(new_prop_info->type, list); - ZEND_TYPE_FULL_MASK(new_prop_info->type) |= _ZEND_TYPE_ARENA_BIT; - } + /* Deep copy the type information */ + zend_type_copy_ctor(&new_prop_info->type, /* use_arena */ true, /* persistent */ false); } } diff --git a/Zend/zend_opcode.c b/Zend/zend_opcode.c index 6d42379b202..3475bc4db3b 100644 --- a/Zend/zend_opcode.c +++ b/Zend/zend_opcode.c @@ -110,17 +110,9 @@ ZEND_API void destroy_zend_function(zend_function *function) ZEND_API void zend_type_release(zend_type type, bool persistent) { if (ZEND_TYPE_HAS_LIST(type)) { - zend_type *list_type, *sublist_type; + zend_type *list_type; ZEND_TYPE_LIST_FOREACH(ZEND_TYPE_LIST(type), list_type) { - if (ZEND_TYPE_HAS_LIST(*list_type)) { - ZEND_TYPE_LIST_FOREACH(ZEND_TYPE_LIST(*list_type), sublist_type) { - if (ZEND_TYPE_HAS_NAME(*sublist_type)) { - zend_string_release(ZEND_TYPE_NAME(*sublist_type)); - } - } ZEND_TYPE_LIST_FOREACH_END(); - } else if (ZEND_TYPE_HAS_NAME(*list_type)) { - zend_string_release(ZEND_TYPE_NAME(*list_type)); - } + zend_type_release(*list_type, persistent); } ZEND_TYPE_LIST_FOREACH_END(); if (!ZEND_TYPE_USES_ARENA(type)) { pefree(ZEND_TYPE_LIST(type), persistent); diff --git a/ext/zend_test/test.stub.php b/ext/zend_test/test.stub.php index dabf8943242..5ebe6b574b6 100644 --- a/ext/zend_test/test.stub.php +++ b/ext/zend_test/test.stub.php @@ -55,6 +55,7 @@ namespace { trait _ZendTestTrait { /** @var mixed */ public $testProp; + public Traversable|Countable $classUnionProp; public function testMethod(): bool {} } diff --git a/ext/zend_test/test_arginfo.h b/ext/zend_test/test_arginfo.h index ba6ff9c73bd..5a3af3879d7 100644 --- a/ext/zend_test/test_arginfo.h +++ b/ext/zend_test/test_arginfo.h @@ -1,5 +1,5 @@ /* This is a generated file, edit the .stub.php file instead. - * Stub hash: 9a8087f2c6f7fef676f4c955a3c530b83605ac9a */ + * Stub hash: fa6d8c58bdef20c6c9c8db75dbb6ede775324193 */ ZEND_BEGIN_ARG_WITH_RETURN_TYPE_INFO_EX(arginfo_zend_test_array_return, 0, 0, IS_ARRAY, 0) ZEND_END_ARG_INFO() @@ -496,6 +496,19 @@ static zend_class_entry *register_class__ZendTestTrait(void) zend_declare_property_ex(class_entry, property_testProp_name, &property_testProp_default_value, ZEND_ACC_PUBLIC, NULL); zend_string_release(property_testProp_name); + zend_string *property_classUnionProp_class_Traversable = zend_string_init("Traversable", sizeof("Traversable") - 1, 1); + zend_string *property_classUnionProp_class_Countable = zend_string_init("Countable", sizeof("Countable") - 1, 1); + zend_type_list *property_classUnionProp_type_list = malloc(ZEND_TYPE_LIST_SIZE(2)); + property_classUnionProp_type_list->num_types = 2; + property_classUnionProp_type_list->types[0] = (zend_type) ZEND_TYPE_INIT_CLASS(property_classUnionProp_class_Traversable, 0, 0); + property_classUnionProp_type_list->types[1] = (zend_type) ZEND_TYPE_INIT_CLASS(property_classUnionProp_class_Countable, 0, 0); + zend_type property_classUnionProp_type = ZEND_TYPE_INIT_UNION(property_classUnionProp_type_list, 0); + zval property_classUnionProp_default_value; + ZVAL_UNDEF(&property_classUnionProp_default_value); + zend_string *property_classUnionProp_name = zend_string_init("classUnionProp", sizeof("classUnionProp") - 1, 1); + zend_declare_typed_property(class_entry, property_classUnionProp_name, &property_classUnionProp_default_value, ZEND_ACC_PUBLIC, NULL, property_classUnionProp_type); + zend_string_release(property_classUnionProp_name); + return class_entry; }