From 6bde436fd95f3d74edb32816ad2627e5717c6770 Mon Sep 17 00:00:00 2001 From: Nikita Popov Date: Thu, 24 Oct 2019 16:36:25 +0200 Subject: [PATCH 1/2] Don't call __set() on uninitialized typed properties, take 2 --- .../typed_properties_magic_set.phpt | 25 +++++++++++++++++++ Zend/zend_API.c | 10 ++++++-- Zend/zend_inheritance.c | 5 +++- Zend/zend_object_handlers.c | 7 ++++++ Zend/zend_objects.c | 2 +- Zend/zend_types.h | 3 +++ ext/opcache/zend_accelerator_util_funcs.c | 2 +- 7 files changed, 49 insertions(+), 5 deletions(-) create mode 100644 Zend/tests/type_declarations/typed_properties_magic_set.phpt diff --git a/Zend/tests/type_declarations/typed_properties_magic_set.phpt b/Zend/tests/type_declarations/typed_properties_magic_set.phpt new file mode 100644 index 0000000000000..9ba712c54b67a --- /dev/null +++ b/Zend/tests/type_declarations/typed_properties_magic_set.phpt @@ -0,0 +1,25 @@ +--TEST-- +__set() should not be invoked when setting an uninitialized typed property +--FILE-- +foo = 42; +var_dump($test->foo); +// __set will be called after unset() +unset($test->foo); +$test->foo = 42; +// __set will be called after unset() without prior initialization +$test = new Test; +unset($test->foo); +$test->foo = 42; +?> +--EXPECT-- +int(42) +__set foo = 42 +__set foo = 42 diff --git a/Zend/zend_API.c b/Zend/zend_API.c index 8fab994297a6a..bb292a32bac36 100644 --- a/Zend/zend_API.c +++ b/Zend/zend_API.c @@ -1264,12 +1264,14 @@ static zend_always_inline void _object_properties_init(zend_object *object, zend if (UNEXPECTED(class_type->type == ZEND_INTERNAL_CLASS)) { do { ZVAL_COPY_OR_DUP(dst, src); + Z_EXTRA_P(dst) = Z_EXTRA_P(src); src++; dst++; } while (src != end); } else { do { ZVAL_COPY(dst, src); + Z_EXTRA_P(dst) = Z_EXTRA_P(src); src++; dst++; } while (src != end); @@ -3683,6 +3685,7 @@ static zend_always_inline zend_bool is_persistent_class(zend_class_entry *ce) { ZEND_API int zend_declare_typed_property(zend_class_entry *ce, zend_string *name, zval *property, int access_type, zend_string *doc_comment, zend_type type) /* {{{ */ { zend_property_info *property_info, *property_info_ptr; + zval *property_default_ptr; if (ZEND_TYPE_IS_SET(type)) { ce->ce_flags |= ZEND_ACC_HAS_TYPE_HINTS; @@ -3714,7 +3717,7 @@ ZEND_API int zend_declare_typed_property(zend_class_entry *ce, zend_string *name property_info->offset = ce->default_static_members_count++; ce->default_static_members_table = perealloc(ce->default_static_members_table, sizeof(zval) * ce->default_static_members_count, ce->type == ZEND_INTERNAL_CLASS); } - ZVAL_COPY_VALUE(&ce->default_static_members_table[property_info->offset], property); + property_default_ptr = &ce->default_static_members_table[property_info->offset]; if (!ZEND_MAP_PTR(ce->static_members_table)) { ZEND_ASSERT(ce->type == ZEND_INTERNAL_CLASS); if (!EG(current_execute_data)) { @@ -3745,7 +3748,7 @@ ZEND_API int zend_declare_typed_property(zend_class_entry *ce, zend_string *name ce->properties_info_table[ce->default_properties_count - 1] = property_info; } } - ZVAL_COPY_VALUE(&ce->default_properties_table[OBJ_PROP_TO_NUM(property_info->offset)], property); + property_default_ptr = &ce->default_properties_table[OBJ_PROP_TO_NUM(property_info->offset)]; } if (ce->type & ZEND_INTERNAL_CLASS) { switch(Z_TYPE_P(property)) { @@ -3762,6 +3765,9 @@ ZEND_API int zend_declare_typed_property(zend_class_entry *ce, zend_string *name name = zend_new_interned_string(zend_string_copy(name)); } + ZVAL_COPY_VALUE(property_default_ptr, property); + Z_EXTRA_P(property_default_ptr) = Z_ISUNDEF_P(property) ? IS_PROP_UNINIT : 0; + if (access_type & ZEND_ACC_PUBLIC) { property_info->name = zend_string_copy(name); } else if (access_type & ZEND_ACC_PRIVATE) { diff --git a/Zend/zend_inheritance.c b/Zend/zend_inheritance.c index c7002d313f01d..ece379803cb6a 100644 --- a/Zend/zend_inheritance.c +++ b/Zend/zend_inheritance.c @@ -1167,7 +1167,7 @@ ZEND_API void zend_do_inheritance_ex(zend_class_entry *ce, zend_class_entry *par do { dst--; src--; - ZVAL_COPY_VALUE(dst, src); + *dst = *src; /* Copy Z_EXTRA as well */ } while (dst != end); pefree(src, ce->type == ZEND_INTERNAL_CLASS); end = ce->default_properties_table; @@ -1182,7 +1182,9 @@ ZEND_API void zend_do_inheritance_ex(zend_class_entry *ce, zend_class_entry *par do { dst--; src--; + *dst = *src; /* Copy Z_EXTRA as well */ ZVAL_COPY_OR_DUP(dst, src); + Z_EXTRA_P(dst) = Z_EXTRA_P(src); if (Z_OPT_TYPE_P(dst) == IS_CONSTANT_AST) { ce->ce_flags &= ~ZEND_ACC_CONSTANTS_UPDATED; } @@ -1193,6 +1195,7 @@ ZEND_API void zend_do_inheritance_ex(zend_class_entry *ce, zend_class_entry *par dst--; src--; ZVAL_COPY(dst, src); + Z_EXTRA_P(dst) = Z_EXTRA_P(src); if (Z_OPT_TYPE_P(dst) == IS_CONSTANT_AST) { ce->ce_flags &= ~ZEND_ACC_CONSTANTS_UPDATED; } diff --git a/Zend/zend_object_handlers.c b/Zend/zend_object_handlers.c index 09984390c9024..a920cbc9fcb8c 100644 --- a/Zend/zend_object_handlers.c +++ b/Zend/zend_object_handlers.c @@ -836,6 +836,11 @@ ZEND_API zval *zend_std_write_property(zval *object, zval *member, zval *value, zend_assign_to_variable(variable_ptr, value, IS_TMP_VAR, EG(current_execute_data) && ZEND_CALL_USES_STRICT_TYPES(EG(current_execute_data))); goto exit; } + if (Z_EXTRA_P(variable_ptr) == IS_PROP_UNINIT) { + /* Writes to uninitializde typed properties bypass __set(). */ + Z_EXTRA_P(variable_ptr) = 0; + goto write_std_property; + } } else if (EXPECTED(IS_DYNAMIC_PROPERTY_OFFSET(property_offset))) { if (EXPECTED(zobj->properties != NULL)) { if (UNEXPECTED(GC_REFCOUNT(zobj->properties) > 1)) { @@ -1113,6 +1118,8 @@ ZEND_API void zend_std_unset_property(zval *object, zval *member, void **cache_s } goto exit; } + /* Reset the IS_PROP_UNINIT flag, if it exists. */ + Z_EXTRA_P(slot) = 0; } else if (EXPECTED(IS_DYNAMIC_PROPERTY_OFFSET(property_offset)) && EXPECTED(zobj->properties != NULL)) { if (UNEXPECTED(GC_REFCOUNT(zobj->properties) > 1)) { diff --git a/Zend/zend_objects.c b/Zend/zend_objects.c index eb76887a9d1cb..eb9c095f31c94 100644 --- a/Zend/zend_objects.c +++ b/Zend/zend_objects.c @@ -209,7 +209,7 @@ ZEND_API void ZEND_FASTCALL zend_objects_clone_members(zend_object *new_object, do { i_zval_ptr_dtor(dst); - ZVAL_COPY_VALUE(dst, src); + *dst = *src; /* Copy Z_EXTRA as well */ zval_add_ref(dst); if (UNEXPECTED(Z_ISREF_P(dst)) && (ZEND_DEBUG || ZEND_REF_HAS_TYPE_SOURCES(Z_REF_P(dst)))) { diff --git a/Zend/zend_types.h b/Zend/zend_types.h index 83877e0d5d4a9..c3ce8863a2b0f 100644 --- a/Zend/zend_types.h +++ b/Zend/zend_types.h @@ -593,6 +593,9 @@ static zend_always_inline uint32_t zval_gc_info(uint32_t gc_type_info) { #define OBJ_FLAGS(obj) GC_FLAGS(obj) +/* Property flag stores in Z_EXTRA */ +#define IS_PROP_UNINIT 1 + /* Recursion protection macros must be used only for arrays and objects */ #define GC_IS_RECURSIVE(p) \ (GC_FLAGS(p) & GC_PROTECTED) diff --git a/ext/opcache/zend_accelerator_util_funcs.c b/ext/opcache/zend_accelerator_util_funcs.c index 71e98a6bdc879..22d9f58f95022 100644 --- a/ext/opcache/zend_accelerator_util_funcs.c +++ b/ext/opcache/zend_accelerator_util_funcs.c @@ -270,7 +270,7 @@ static void zend_class_copy_ctor(zend_class_entry **pce) end = src + ce->default_properties_count; ce->default_properties_table = dst; for (; src != end; src++, dst++) { - ZVAL_COPY_VALUE(dst, src); + *dst = *src; } } From 68b54e7f4a7c7f7bfef6824470489f90825e3d69 Mon Sep 17 00:00:00 2001 From: Nikita Popov Date: Fri, 25 Oct 2019 16:01:09 +0200 Subject: [PATCH 2/2] Introduce separate macros for property flag Also don't set it for static properties -- this is only relevant for instance properties, as static properties have no __set(). --- Zend/zend_API.c | 15 ++++++--------- Zend/zend_inheritance.c | 9 +++------ Zend/zend_object_handlers.c | 6 +++--- Zend/zend_objects.c | 2 +- Zend/zend_types.h | 17 ++++++++++++++--- ext/opcache/zend_accelerator_util_funcs.c | 2 +- 6 files changed, 28 insertions(+), 23 deletions(-) diff --git a/Zend/zend_API.c b/Zend/zend_API.c index bb292a32bac36..6d9d10a13f2de 100644 --- a/Zend/zend_API.c +++ b/Zend/zend_API.c @@ -1263,15 +1263,13 @@ static zend_always_inline void _object_properties_init(zend_object *object, zend if (UNEXPECTED(class_type->type == ZEND_INTERNAL_CLASS)) { do { - ZVAL_COPY_OR_DUP(dst, src); - Z_EXTRA_P(dst) = Z_EXTRA_P(src); + ZVAL_COPY_OR_DUP_PROP(dst, src); src++; dst++; } while (src != end); } else { do { - ZVAL_COPY(dst, src); - Z_EXTRA_P(dst) = Z_EXTRA_P(src); + ZVAL_COPY_PROP(dst, src); src++; dst++; } while (src != end); @@ -3685,7 +3683,6 @@ static zend_always_inline zend_bool is_persistent_class(zend_class_entry *ce) { ZEND_API int zend_declare_typed_property(zend_class_entry *ce, zend_string *name, zval *property, int access_type, zend_string *doc_comment, zend_type type) /* {{{ */ { zend_property_info *property_info, *property_info_ptr; - zval *property_default_ptr; if (ZEND_TYPE_IS_SET(type)) { ce->ce_flags |= ZEND_ACC_HAS_TYPE_HINTS; @@ -3717,7 +3714,7 @@ ZEND_API int zend_declare_typed_property(zend_class_entry *ce, zend_string *name property_info->offset = ce->default_static_members_count++; ce->default_static_members_table = perealloc(ce->default_static_members_table, sizeof(zval) * ce->default_static_members_count, ce->type == ZEND_INTERNAL_CLASS); } - property_default_ptr = &ce->default_static_members_table[property_info->offset]; + ZVAL_COPY_VALUE(&ce->default_static_members_table[property_info->offset], property); if (!ZEND_MAP_PTR(ce->static_members_table)) { ZEND_ASSERT(ce->type == ZEND_INTERNAL_CLASS); if (!EG(current_execute_data)) { @@ -3728,6 +3725,7 @@ ZEND_API int zend_declare_typed_property(zend_class_entry *ce, zend_string *name } } } else { + zval *property_default_ptr; if ((property_info_ptr = zend_hash_find_ptr(&ce->properties_info, name)) != NULL && (property_info_ptr->flags & ZEND_ACC_STATIC) == 0) { property_info->offset = property_info_ptr->offset; @@ -3749,6 +3747,8 @@ ZEND_API int zend_declare_typed_property(zend_class_entry *ce, zend_string *name } } property_default_ptr = &ce->default_properties_table[OBJ_PROP_TO_NUM(property_info->offset)]; + ZVAL_COPY_VALUE(property_default_ptr, property); + Z_PROP_FLAG_P(property_default_ptr) = Z_ISUNDEF_P(property) ? IS_PROP_UNINIT : 0; } if (ce->type & ZEND_INTERNAL_CLASS) { switch(Z_TYPE_P(property)) { @@ -3765,9 +3765,6 @@ ZEND_API int zend_declare_typed_property(zend_class_entry *ce, zend_string *name name = zend_new_interned_string(zend_string_copy(name)); } - ZVAL_COPY_VALUE(property_default_ptr, property); - Z_EXTRA_P(property_default_ptr) = Z_ISUNDEF_P(property) ? IS_PROP_UNINIT : 0; - if (access_type & ZEND_ACC_PUBLIC) { property_info->name = zend_string_copy(name); } else if (access_type & ZEND_ACC_PRIVATE) { diff --git a/Zend/zend_inheritance.c b/Zend/zend_inheritance.c index ece379803cb6a..56d9e6268f62e 100644 --- a/Zend/zend_inheritance.c +++ b/Zend/zend_inheritance.c @@ -1167,7 +1167,7 @@ ZEND_API void zend_do_inheritance_ex(zend_class_entry *ce, zend_class_entry *par do { dst--; src--; - *dst = *src; /* Copy Z_EXTRA as well */ + ZVAL_COPY_VALUE_PROP(dst, src); } while (dst != end); pefree(src, ce->type == ZEND_INTERNAL_CLASS); end = ce->default_properties_table; @@ -1182,9 +1182,7 @@ ZEND_API void zend_do_inheritance_ex(zend_class_entry *ce, zend_class_entry *par do { dst--; src--; - *dst = *src; /* Copy Z_EXTRA as well */ - ZVAL_COPY_OR_DUP(dst, src); - Z_EXTRA_P(dst) = Z_EXTRA_P(src); + ZVAL_COPY_OR_DUP_PROP(dst, src); if (Z_OPT_TYPE_P(dst) == IS_CONSTANT_AST) { ce->ce_flags &= ~ZEND_ACC_CONSTANTS_UPDATED; } @@ -1194,8 +1192,7 @@ ZEND_API void zend_do_inheritance_ex(zend_class_entry *ce, zend_class_entry *par do { dst--; src--; - ZVAL_COPY(dst, src); - Z_EXTRA_P(dst) = Z_EXTRA_P(src); + ZVAL_COPY_PROP(dst, src); if (Z_OPT_TYPE_P(dst) == IS_CONSTANT_AST) { ce->ce_flags &= ~ZEND_ACC_CONSTANTS_UPDATED; } diff --git a/Zend/zend_object_handlers.c b/Zend/zend_object_handlers.c index a920cbc9fcb8c..5ac64fffb19d7 100644 --- a/Zend/zend_object_handlers.c +++ b/Zend/zend_object_handlers.c @@ -836,9 +836,9 @@ ZEND_API zval *zend_std_write_property(zval *object, zval *member, zval *value, zend_assign_to_variable(variable_ptr, value, IS_TMP_VAR, EG(current_execute_data) && ZEND_CALL_USES_STRICT_TYPES(EG(current_execute_data))); goto exit; } - if (Z_EXTRA_P(variable_ptr) == IS_PROP_UNINIT) { + if (Z_PROP_FLAG_P(variable_ptr) == IS_PROP_UNINIT) { /* Writes to uninitializde typed properties bypass __set(). */ - Z_EXTRA_P(variable_ptr) = 0; + Z_PROP_FLAG_P(variable_ptr) = 0; goto write_std_property; } } else if (EXPECTED(IS_DYNAMIC_PROPERTY_OFFSET(property_offset))) { @@ -1119,7 +1119,7 @@ ZEND_API void zend_std_unset_property(zval *object, zval *member, void **cache_s goto exit; } /* Reset the IS_PROP_UNINIT flag, if it exists. */ - Z_EXTRA_P(slot) = 0; + Z_PROP_FLAG_P(slot) = 0; } else if (EXPECTED(IS_DYNAMIC_PROPERTY_OFFSET(property_offset)) && EXPECTED(zobj->properties != NULL)) { if (UNEXPECTED(GC_REFCOUNT(zobj->properties) > 1)) { diff --git a/Zend/zend_objects.c b/Zend/zend_objects.c index eb9c095f31c94..ac9412a1c6014 100644 --- a/Zend/zend_objects.c +++ b/Zend/zend_objects.c @@ -209,7 +209,7 @@ ZEND_API void ZEND_FASTCALL zend_objects_clone_members(zend_object *new_object, do { i_zval_ptr_dtor(dst); - *dst = *src; /* Copy Z_EXTRA as well */ + ZVAL_COPY_VALUE_PROP(dst, src); zval_add_ref(dst); if (UNEXPECTED(Z_ISREF_P(dst)) && (ZEND_DEBUG || ZEND_REF_HAS_TYPE_SOURCES(Z_REF_P(dst)))) { diff --git a/Zend/zend_types.h b/Zend/zend_types.h index c3ce8863a2b0f..171d70a9dfd5a 100644 --- a/Zend/zend_types.h +++ b/Zend/zend_types.h @@ -593,9 +593,6 @@ static zend_always_inline uint32_t zval_gc_info(uint32_t gc_type_info) { #define OBJ_FLAGS(obj) GC_FLAGS(obj) -/* Property flag stores in Z_EXTRA */ -#define IS_PROP_UNINIT 1 - /* Recursion protection macros must be used only for arrays and objects */ #define GC_IS_RECURSIVE(p) \ (GC_FLAGS(p) & GC_PROTECTED) @@ -1265,4 +1262,18 @@ static zend_always_inline uint32_t zval_delref_p(zval* pz) { } \ } while (0) +/* Properties store a flag distinguishing unset and unintialized properties + * (both use IS_UNDEF type) in the Z_EXTRA space. As such we also need to copy + * the Z_EXTRA space when copying property default values etc. We define separate + * macros for this purpose, so this workaround is easier to remove in the future. */ +#define IS_PROP_UNINIT 1 +#define Z_PROP_FLAG_P(z) Z_EXTRA_P(z) +#define ZVAL_COPY_VALUE_PROP(z, v) \ + do { *(z) = *(v); } while (0) +#define ZVAL_COPY_PROP(z, v) \ + do { ZVAL_COPY(z, v); Z_PROP_FLAG_P(z) = Z_PROP_FLAG_P(v); } while (0) +#define ZVAL_COPY_OR_DUP_PROP(z, v) \ + do { ZVAL_COPY_OR_DUP(z, v); Z_PROP_FLAG_P(z) = Z_PROP_FLAG_P(v); } while (0) + + #endif /* ZEND_TYPES_H */ diff --git a/ext/opcache/zend_accelerator_util_funcs.c b/ext/opcache/zend_accelerator_util_funcs.c index 22d9f58f95022..dc7a76b32610f 100644 --- a/ext/opcache/zend_accelerator_util_funcs.c +++ b/ext/opcache/zend_accelerator_util_funcs.c @@ -270,7 +270,7 @@ static void zend_class_copy_ctor(zend_class_entry **pce) end = src + ce->default_properties_count; ce->default_properties_table = dst; for (; src != end; src++, dst++) { - *dst = *src; + ZVAL_COPY_VALUE_PROP(dst, src); } }