From d59718c7c56b6aafcc31fae588bfc4d4e745e868 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?M=C3=A1t=C3=A9=20Kocsis?= Date: Wed, 7 Sep 2022 10:08:05 +0200 Subject: [PATCH 1/6] Allow readonly properties to be reinitialized once during cloning Implements https://wiki.php.net/rfc/readonly_amendments --- .../readonly_props/readonly_clone_error1.phpt | 36 +++++++++ .../readonly_props/readonly_clone_error2.phpt | 42 +++++++++++ .../readonly_props/readonly_clone_error3.phpt | 44 +++++++++++ .../readonly_clone_success.phpt | 39 ++++++++++ .../readonly_clone_success2.phpt | 74 +++++++++++++++++++ .../readonly_clone_success3.phpt | 33 +++++++++ Zend/zend_API.c | 4 + Zend/zend_compile.h | 4 +- Zend/zend_enum.c | 14 +++- Zend/zend_execute.c | 11 ++- Zend/zend_execute.h | 1 + Zend/zend_inheritance.c | 2 +- Zend/zend_object_handlers.c | 32 +++++--- Zend/zend_objects.c | 24 +++++- Zend/zend_types.h | 1 + 15 files changed, 341 insertions(+), 20 deletions(-) create mode 100644 Zend/tests/readonly_props/readonly_clone_error1.phpt create mode 100644 Zend/tests/readonly_props/readonly_clone_error2.phpt create mode 100644 Zend/tests/readonly_props/readonly_clone_error3.phpt create mode 100644 Zend/tests/readonly_props/readonly_clone_success.phpt create mode 100644 Zend/tests/readonly_props/readonly_clone_success2.phpt create mode 100644 Zend/tests/readonly_props/readonly_clone_success3.phpt diff --git a/Zend/tests/readonly_props/readonly_clone_error1.phpt b/Zend/tests/readonly_props/readonly_clone_error1.phpt new file mode 100644 index 0000000000000..98829d175765d --- /dev/null +++ b/Zend/tests/readonly_props/readonly_clone_error1.phpt @@ -0,0 +1,36 @@ +--TEST-- +Readonly property cannot be reset twice during cloning +--FILE-- +bar = 2; + var_dump($this); + $this->bar = 3; + } +} + +$foo = new Foo(1); + +try { + clone $foo; +} catch (Error $exception) { + echo $exception->getMessage() . "\n"; +} + +echo "done"; + +?> +--EXPECT-- +object(Foo)#2 (1) { + ["bar"]=> + int(2) +} +Cannot modify readonly property Foo::$bar +done diff --git a/Zend/tests/readonly_props/readonly_clone_error2.phpt b/Zend/tests/readonly_props/readonly_clone_error2.phpt new file mode 100644 index 0000000000000..e1e42abad074c --- /dev/null +++ b/Zend/tests/readonly_props/readonly_clone_error2.phpt @@ -0,0 +1,42 @@ +--TEST-- +Readonly property cannot be reset after cloning when there is no custom clone handler +--FILE-- +bar++; + } + + public function wrongCloneNew() + { + $instance = clone $this; + $instance->baz++; + } +} + +$foo = new Foo(1, 1); + +try { + $foo->wrongCloneOld(); +} catch (Error $exception) { + echo $exception->getMessage() . "\n"; +} + +try { + $foo->wrongCloneNew(); +} catch (Error $exception) { + echo $exception->getMessage() . "\n"; +} + +?> +--EXPECT-- +Cannot modify readonly property Foo::$bar +Cannot modify readonly property Foo::$baz diff --git a/Zend/tests/readonly_props/readonly_clone_error3.phpt b/Zend/tests/readonly_props/readonly_clone_error3.phpt new file mode 100644 index 0000000000000..a51ec165c68c3 --- /dev/null +++ b/Zend/tests/readonly_props/readonly_clone_error3.phpt @@ -0,0 +1,44 @@ +--TEST-- +Readonly property cannot be reset after cloning when there is a custom clone handler +--FILE-- +bar++; + } + + public function wrongCloneNew() + { + $instance = clone $this; + $instance->baz++; + } +} + +$foo = new Foo(1, 1); + +try { + $foo->wrongCloneOld(); +} catch (Error $exception) { + echo $exception->getMessage() . "\n"; +} + +try { + $foo->wrongCloneNew(); +} catch (Error $exception) { + echo $exception->getMessage() . "\n"; +} + +?> +--EXPECT-- +Cannot modify readonly property Foo::$bar +Cannot modify readonly property Foo::$baz diff --git a/Zend/tests/readonly_props/readonly_clone_success.phpt b/Zend/tests/readonly_props/readonly_clone_success.phpt new file mode 100644 index 0000000000000..72cd9e9622b33 --- /dev/null +++ b/Zend/tests/readonly_props/readonly_clone_success.phpt @@ -0,0 +1,39 @@ +--TEST-- +Readonly property can be reset once during cloning +--FILE-- +bar++; + } +} + +$foo = new Foo(1); + +var_dump(clone $foo); + +$foo2 = clone $foo; +var_dump($foo2); + +var_dump(clone $foo2); + +?> +--EXPECTF-- +object(Foo)#%d (%d) { + ["bar"]=> + int(2) +} +object(Foo)#%d (%d) { + ["bar"]=> + int(2) +} +object(Foo)#%d (%d) { + ["bar"]=> + int(3) +} diff --git a/Zend/tests/readonly_props/readonly_clone_success2.phpt b/Zend/tests/readonly_props/readonly_clone_success2.phpt new file mode 100644 index 0000000000000..210a359325bc6 --- /dev/null +++ b/Zend/tests/readonly_props/readonly_clone_success2.phpt @@ -0,0 +1,74 @@ +--TEST-- +Test that __clone() can write to readonly properties +--FILE-- +count = ++self::$counter; + $this->foo = 0; + } + + public function count(?int $count = null): static + { + $new = clone $this; + $new->count = $count ?? ++self::$counter; + + return $new; + } + + public function __clone() + { + if (is_a(debug_backtrace(DEBUG_BACKTRACE_IGNORE_ARGS, 2)[1]['class'] ?? '', self::class, true)) { + unset($this->count); + } else { + $this->count = ++self::$counter; + } + $this->foo = 1; + } +} + +$a = new Counter(); +var_dump($a); + +var_dump(clone $a); + +$b = $a->count(); +var_dump($b); + +$c = $a->count(123); +var_dump($c); + +?> +--EXPECTF-- +object(Counter)#%d (2) { + ["count"]=> + int(1) + ["foo":"Counter":private]=> + int(0) +} +object(Counter)#%d (2) { + ["count"]=> + int(2) + ["foo":"Counter":private]=> + int(1) +} +object(Counter)#%d (2) { + ["count"]=> + int(3) + ["foo":"Counter":private]=> + int(1) +} +object(Counter)#%d (2) { + ["count"]=> + int(123) + ["foo":"Counter":private]=> + int(1) +} diff --git a/Zend/tests/readonly_props/readonly_clone_success3.phpt b/Zend/tests/readonly_props/readonly_clone_success3.phpt new file mode 100644 index 0000000000000..a4b5626203c6f --- /dev/null +++ b/Zend/tests/readonly_props/readonly_clone_success3.phpt @@ -0,0 +1,33 @@ +--TEST-- +Test that __clone() unset properties +--FILE-- +bar); + } +} + +$foo = new Foo(new stdClass()); +$foo2 = clone $foo; + +var_dump($foo); +var_dump($foo2); + +?> +--EXPECTF-- +object(Foo)#1 (%d) { + ["bar"]=> + object(stdClass)#2 (%d) { + } +} +object(Foo)#3 (%d) { + ["bar"]=> + uninitialized(stdClass) +} diff --git a/Zend/zend_API.c b/Zend/zend_API.c index 5247ca1a379af..b7006c15e35cc 100644 --- a/Zend/zend_API.c +++ b/Zend/zend_API.c @@ -4224,6 +4224,10 @@ ZEND_API zend_property_info *zend_declare_typed_property(zend_class_entry *ce, z ce->ce_flags |= ZEND_ACC_HAS_TYPE_HINTS; } + if (access_type & ZEND_ACC_READONLY) { + ce->ce_flags |= ZEND_ACC_HAS_READONLY_PROPS; + } + if (ce->type == ZEND_INTERNAL_CLASS) { property_info = pemalloc(sizeof(zend_property_info), 1); } else { diff --git a/Zend/zend_compile.h b/Zend/zend_compile.h index 22221f2265c3f..8ff8ab0f509fb 100644 --- a/Zend/zend_compile.h +++ b/Zend/zend_compile.h @@ -241,7 +241,7 @@ typedef struct _zend_oparray_context { /* or IS_CONSTANT_VISITED_MARK | | | */ #define ZEND_CLASS_CONST_IS_CASE (1 << 6) /* | | | X */ /* | | | */ -/* Class Flags (unused: 21,30,31) | | | */ +/* Class Flags (unused: 30,31) | | | */ /* =========== | | | */ /* | | | */ /* Special class types | | | */ @@ -288,6 +288,8 @@ typedef struct _zend_oparray_context { /* | | | */ /* Class is linked apart from variance obligations. | | | */ #define ZEND_ACC_NEARLY_LINKED (1 << 20) /* X | | | */ +/* Class has readonly props | | | */ +#define ZEND_ACC_HAS_READONLY_PROPS (1 << 21) /* X | | | */ /* | | | */ /* stored in opcache (may be partially) | | | */ #define ZEND_ACC_CACHED (1 << 22) /* X | | | */ diff --git a/Zend/zend_enum.c b/Zend/zend_enum.c index 435ebfeaf109c..4abca60fa85b1 100644 --- a/Zend/zend_enum.c +++ b/Zend/zend_enum.c @@ -41,9 +41,17 @@ zend_object *zend_enum_new(zval *result, zend_class_entry *ce, zend_string *case zend_object *zobj = zend_objects_new(ce); ZVAL_OBJ(result, zobj); - ZVAL_STR_COPY(OBJ_PROP_NUM(zobj, 0), case_name); + zval *zname = OBJ_PROP_NUM(zobj, 0); + ZVAL_STR_COPY(zname, case_name); + /* ZVAL_COPY does not set Z_PROP_FLAG, this needs to be cleared to avoid leaving IS_PROP_REINITABLE set */ + Z_PROP_FLAG_P(zname) = 0; + if (backing_value_zv != NULL) { - ZVAL_COPY(OBJ_PROP_NUM(zobj, 1), backing_value_zv); + zval *prop = OBJ_PROP_NUM(zobj, 1); + + ZVAL_COPY(prop, backing_value_zv); + /* ZVAL_COPY does not set Z_PROP_FLAG, this needs to be cleared to avoid leaving IS_PROP_REINITABLE set */ + Z_PROP_FLAG_P(prop) = 0; } return zobj; @@ -179,7 +187,7 @@ void zend_enum_add_interfaces(zend_class_entry *ce) if (ce->enum_backing_type != IS_UNDEF) { ce->interface_names[num_interfaces_before + 1].name = zend_string_copy(zend_ce_backed_enum->name); - ce->interface_names[num_interfaces_before + 1].lc_name = zend_string_init("backedenum", sizeof("backedenum") - 1, 0); + ce->interface_names[num_interfaces_before + 1].lc_name = zend_string_init("backedenum", sizeof("backedenum") - 1, 0); } ce->default_object_handlers = &zend_enum_object_handlers; diff --git a/Zend/zend_execute.c b/Zend/zend_execute.c index e15469c539e95..ebf0ef8628578 100644 --- a/Zend/zend_execute.c +++ b/Zend/zend_execute.c @@ -1010,8 +1010,12 @@ static zend_never_inline zval* zend_assign_to_typed_prop(zend_property_info *inf zval tmp; if (UNEXPECTED(info->flags & ZEND_ACC_READONLY)) { - zend_readonly_property_modification_error(info); - return &EG(uninitialized_zval); + if (Z_PROP_FLAG_P(property_val) & IS_PROP_REINITABLE) { + Z_PROP_FLAG_P(property_val) &= ~IS_PROP_REINITABLE; + } else { + zend_readonly_property_modification_error(info); + return &EG(uninitialized_zval); + } } ZVAL_DEREF(value); @@ -3161,6 +3165,9 @@ static zend_always_inline void zend_fetch_property_address(zval *result, zval *c ZEND_ASSERT(type == BP_VAR_W || type == BP_VAR_RW || type == BP_VAR_UNSET); if (Z_TYPE_P(ptr) == IS_OBJECT) { ZVAL_COPY(result, ptr); + } else if (Z_PROP_FLAG_P(ptr) & IS_PROP_REINITABLE) { + Z_PROP_FLAG_P(ptr) &= ~IS_PROP_REINITABLE; + ZVAL_COPY(result, ptr); } else { zend_readonly_property_modification_error(prop_info); ZVAL_ERROR(result); diff --git a/Zend/zend_execute.h b/Zend/zend_execute.h index db6bdaff86f63..323b6269ee55e 100644 --- a/Zend/zend_execute.h +++ b/Zend/zend_execute.h @@ -463,6 +463,7 @@ ZEND_API int ZEND_FASTCALL zend_handle_undef_args(zend_execute_data *call); } while (0) #define ZEND_CLASS_HAS_TYPE_HINTS(ce) ((ce->ce_flags & ZEND_ACC_HAS_TYPE_HINTS) == ZEND_ACC_HAS_TYPE_HINTS) +#define ZEND_CLASS_HAS_READONLY_PROPS(ce) ((ce->ce_flags & ZEND_ACC_HAS_READONLY_PROPS) == ZEND_ACC_HAS_READONLY_PROPS) ZEND_API bool zend_verify_property_type(const zend_property_info *info, zval *property, bool strict); ZEND_COLD void zend_verify_property_type_error(const zend_property_info *info, const zval *property); diff --git a/Zend/zend_inheritance.c b/Zend/zend_inheritance.c index 61b1e06e7cb65..028bdebbfb8f7 100644 --- a/Zend/zend_inheritance.c +++ b/Zend/zend_inheritance.c @@ -1629,7 +1629,7 @@ ZEND_API void zend_do_inheritance_ex(zend_class_entry *ce, zend_class_entry *par ce->ce_flags |= ZEND_ACC_EXPLICIT_ABSTRACT_CLASS; } } - ce->ce_flags |= parent_ce->ce_flags & (ZEND_HAS_STATIC_IN_METHODS | ZEND_ACC_HAS_TYPE_HINTS | ZEND_ACC_USE_GUARDS | ZEND_ACC_NOT_SERIALIZABLE | ZEND_ACC_ALLOW_DYNAMIC_PROPERTIES); + ce->ce_flags |= parent_ce->ce_flags & (ZEND_HAS_STATIC_IN_METHODS | ZEND_ACC_HAS_TYPE_HINTS | ZEND_ACC_HAS_READONLY_PROPS | ZEND_ACC_USE_GUARDS | ZEND_ACC_NOT_SERIALIZABLE | ZEND_ACC_ALLOW_DYNAMIC_PROPERTIES); } /* }}} */ diff --git a/Zend/zend_object_handlers.c b/Zend/zend_object_handlers.c index a0ac789e95400..aaa0e731df517 100644 --- a/Zend/zend_object_handlers.c +++ b/Zend/zend_object_handlers.c @@ -615,6 +615,8 @@ ZEND_API zval *zend_std_read_property(zend_object *zobj, zend_string *name, int * to make sure no actual modification is possible. */ ZVAL_COPY(rv, retval); retval = rv; + } else if (Z_PROP_FLAG_P(retval) & IS_PROP_REINITABLE) { + Z_PROP_FLAG_P(retval) &= ~IS_PROP_REINITABLE; } else { zend_readonly_property_modification_error(prop_info); retval = &EG(uninitialized_zval); @@ -633,7 +635,7 @@ ZEND_API zval *zend_std_read_property(zend_object *zobj, zend_string *name, int } } } - if (UNEXPECTED(Z_PROP_FLAG_P(retval) == IS_PROP_UNINIT)) { + if (UNEXPECTED(Z_PROP_FLAG_P(retval) & IS_PROP_UNINIT)) { /* Skip __get() for uninitialized typed properties */ goto uninit_error; } @@ -811,10 +813,14 @@ ZEND_API zval *zend_std_write_property(zend_object *zobj, zend_string *name, zva if (UNEXPECTED(prop_info)) { if (UNEXPECTED(prop_info->flags & ZEND_ACC_READONLY)) { - Z_TRY_DELREF_P(value); - zend_readonly_property_modification_error(prop_info); - variable_ptr = &EG(error_zval); - goto exit; + if (Z_PROP_FLAG_P(variable_ptr) & IS_PROP_REINITABLE) { + Z_PROP_FLAG_P(variable_ptr) &= ~IS_PROP_REINITABLE; + } else { + Z_TRY_DELREF_P(value); + zend_readonly_property_modification_error(prop_info); + variable_ptr = &EG(error_zval); + goto exit; + } } ZVAL_COPY_VALUE(&tmp, value); @@ -841,7 +847,7 @@ ZEND_API zval *zend_std_write_property(zend_object *zobj, zend_string *name, zva variable_ptr, value, IS_TMP_VAR, property_uses_strict_types()); goto exit; } - if (Z_PROP_FLAG_P(variable_ptr) == IS_PROP_UNINIT) { + if (Z_PROP_FLAG_P(variable_ptr) & IS_PROP_UNINIT) { /* Writes to uninitialized typed properties bypass __set(). */ goto write_std_property; } @@ -1069,7 +1075,7 @@ ZEND_API zval *zend_std_get_property_ptr_ptr(zend_object *zobj, zend_string *nam if (UNEXPECTED(Z_TYPE_P(retval) == IS_UNDEF)) { if (EXPECTED(!zobj->ce->__get) || UNEXPECTED((*zend_get_property_guard(zobj, name)) & IN_GET) || - UNEXPECTED(prop_info && Z_PROP_FLAG_P(retval) == IS_PROP_UNINIT)) { + UNEXPECTED(prop_info && (Z_PROP_FLAG_P(retval) & IS_PROP_UNINIT))) { if (UNEXPECTED(type == BP_VAR_RW || type == BP_VAR_R)) { if (UNEXPECTED(prop_info)) { zend_throw_error(NULL, @@ -1146,8 +1152,12 @@ ZEND_API void zend_std_unset_property(zend_object *zobj, zend_string *name, void if (Z_TYPE_P(slot) != IS_UNDEF) { if (UNEXPECTED(prop_info && (prop_info->flags & ZEND_ACC_READONLY))) { - zend_readonly_property_unset_error(prop_info->ce, name); - return; + if (Z_PROP_FLAG_P(slot) & IS_PROP_REINITABLE) { + Z_PROP_FLAG_P(slot) &= ~IS_PROP_REINITABLE; + } else { + zend_readonly_property_unset_error(prop_info->ce, name); + return; + } } if (UNEXPECTED(Z_ISREF_P(slot)) && (ZEND_DEBUG || ZEND_REF_HAS_TYPE_SOURCES(Z_REF_P(slot)))) { @@ -1164,7 +1174,7 @@ ZEND_API void zend_std_unset_property(zend_object *zobj, zend_string *name, void } return; } - if (UNEXPECTED(Z_PROP_FLAG_P(slot) == IS_PROP_UNINIT)) { + if (UNEXPECTED(Z_PROP_FLAG_P(slot) & IS_PROP_UNINIT)) { if (UNEXPECTED(prop_info && (prop_info->flags & ZEND_ACC_READONLY) && !verify_readonly_initialization_access(prop_info, zobj->ce, name, "unset"))) { return; @@ -1779,7 +1789,7 @@ ZEND_API int zend_std_has_property(zend_object *zobj, zend_string *name, int has if (Z_TYPE_P(value) != IS_UNDEF) { goto found; } - if (UNEXPECTED(Z_PROP_FLAG_P(value) == IS_PROP_UNINIT)) { + if (UNEXPECTED(Z_PROP_FLAG_P(value) & IS_PROP_UNINIT)) { /* Skip __isset() for uninitialized typed properties */ result = 0; goto exit; diff --git a/Zend/zend_objects.c b/Zend/zend_objects.c index df76fa0bb8dc3..6c3bd1a12c75d 100644 --- a/Zend/zend_objects.c +++ b/Zend/zend_objects.c @@ -192,6 +192,8 @@ ZEND_API zend_object* ZEND_FASTCALL zend_objects_new(zend_class_entry *ce) ZEND_API void ZEND_FASTCALL zend_objects_clone_members(zend_object *new_object, zend_object *old_object) { + bool has_clone_method = old_object->ce->clone != NULL; + if (old_object->ce->default_properties_count) { zval *src = old_object->properties_table; zval *dst = new_object->properties_table; @@ -201,6 +203,10 @@ ZEND_API void ZEND_FASTCALL zend_objects_clone_members(zend_object *new_object, i_zval_ptr_dtor(dst); ZVAL_COPY_VALUE_PROP(dst, src); zval_add_ref(dst); + if (has_clone_method) { + Z_PROP_FLAG_P(dst) |= IS_PROP_REINITABLE; + } + if (UNEXPECTED(Z_ISREF_P(dst)) && (ZEND_DEBUG || ZEND_REF_HAS_TYPE_SOURCES(Z_REF_P(dst)))) { zend_property_info *prop_info = zend_get_property_info_for_slot(new_object, dst); @@ -211,7 +217,7 @@ ZEND_API void ZEND_FASTCALL zend_objects_clone_members(zend_object *new_object, src++; dst++; } while (src != end); - } else if (old_object->properties && !old_object->ce->clone) { + } else if (old_object->properties && !has_clone_method) { /* fast copy */ if (EXPECTED(old_object->handlers == &std_object_handlers)) { if (EXPECTED(!(GC_FLAGS(old_object->properties) & IS_ARRAY_IMMUTABLE))) { @@ -245,6 +251,9 @@ ZEND_API void ZEND_FASTCALL zend_objects_clone_members(zend_object *new_object, ZVAL_COPY_VALUE(&new_prop, prop); zval_add_ref(&new_prop); } + if (has_clone_method) { + Z_PROP_FLAG_P(&new_prop) |= IS_PROP_REINITABLE; + } if (EXPECTED(key)) { _zend_hash_append(new_object->properties, key, &new_prop); } else { @@ -253,9 +262,20 @@ ZEND_API void ZEND_FASTCALL zend_objects_clone_members(zend_object *new_object, } ZEND_HASH_FOREACH_END(); } - if (old_object->ce->clone) { + if (has_clone_method) { GC_ADDREF(new_object); zend_call_known_instance_method_with_0_params(new_object->ce->clone, new_object, NULL); + + if (ZEND_CLASS_HAS_READONLY_PROPS(new_object->ce)) { + for (uint32_t i = 0; i < new_object->ce->default_properties_count; i++) { + zend_property_info *prop_info = new_object->ce->properties_info_table[i]; + if (prop_info && (prop_info->flags & ZEND_ACC_READONLY)) { + zval * prop = OBJ_PROP_NUM(new_object, i); + Z_PROP_FLAG_P(prop) &= ~IS_PROP_REINITABLE; + } + } + } + OBJ_RELEASE(new_object); } } diff --git a/Zend/zend_types.h b/Zend/zend_types.h index 3d900226e54e5..70a29342ad606 100644 --- a/Zend/zend_types.h +++ b/Zend/zend_types.h @@ -1274,6 +1274,7 @@ static zend_always_inline uint32_t zval_delref_p(zval* pz) { * 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 IS_PROP_REINITABLE 2 /* It has impact only on readonly properties */ #define Z_PROP_FLAG_P(z) Z_EXTRA_P(z) #define ZVAL_COPY_VALUE_PROP(z, v) \ do { *(z) = *(v); } while (0) From 217f83dc762202197109aedb7a9c51b4babd1c39 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?M=C3=A1t=C3=A9=20Kocsis?= Date: Fri, 24 Feb 2023 11:56:15 +0100 Subject: [PATCH 2/6] Some review fixes --- ...cess.phpt => readonly_clone_success1.phpt} | 0 .../readonly_clone_success2.phpt | 84 +++++++------------ .../readonly_clone_success3.phpt | 28 ++++--- Zend/zend_execute.c | 1 - 4 files changed, 44 insertions(+), 69 deletions(-) rename Zend/tests/readonly_props/{readonly_clone_success.phpt => readonly_clone_success1.phpt} (100%) diff --git a/Zend/tests/readonly_props/readonly_clone_success.phpt b/Zend/tests/readonly_props/readonly_clone_success1.phpt similarity index 100% rename from Zend/tests/readonly_props/readonly_clone_success.phpt rename to Zend/tests/readonly_props/readonly_clone_success1.phpt diff --git a/Zend/tests/readonly_props/readonly_clone_success2.phpt b/Zend/tests/readonly_props/readonly_clone_success2.phpt index 210a359325bc6..98faba3909518 100644 --- a/Zend/tests/readonly_props/readonly_clone_success2.phpt +++ b/Zend/tests/readonly_props/readonly_clone_success2.phpt @@ -1,74 +1,46 @@ --TEST-- -Test that __clone() can write to readonly properties +Test that __clone() unset and reassign properties --FILE-- count = ++self::$counter; - $this->foo = 0; - } - - public function count(?int $count = null): static - { - $new = clone $this; - $new->count = $count ?? ++self::$counter; - - return $new; - } +class Foo { + public function __construct( + public readonly stdClass $bar, + ) {} public function __clone() { - if (is_a(debug_backtrace(DEBUG_BACKTRACE_IGNORE_ARGS, 2)[1]['class'] ?? '', self::class, true)) { - unset($this->count); - } else { - $this->count = ++self::$counter; - } - $this->foo = 1; + unset($this->bar); + var_dump($this); + $this->bar = new stdClass(); } } -$a = new Counter(); -var_dump($a); - -var_dump(clone $a); - -$b = $a->count(); -var_dump($b); +$foo = new Foo(new stdClass()); +var_dump($foo); +$foo2 = clone $foo; -$c = $a->count(123); -var_dump($c); +var_dump($foo); +var_dump($foo2); ?> --EXPECTF-- -object(Counter)#%d (2) { - ["count"]=> - int(1) - ["foo":"Counter":private]=> - int(0) +object(Foo)#1 (%d) { + ["bar"]=> + object(stdClass)#2 (%d) { + } } -object(Counter)#%d (2) { - ["count"]=> - int(2) - ["foo":"Counter":private]=> - int(1) +object(Foo)#3 (%d) { + ["bar"]=> + uninitialized(stdClass) } -object(Counter)#%d (2) { - ["count"]=> - int(3) - ["foo":"Counter":private]=> - int(1) +object(Foo)#1 (%d) { + ["bar"]=> + object(stdClass)#2 (%d) { + } } -object(Counter)#%d (2) { - ["count"]=> - int(123) - ["foo":"Counter":private]=> - int(1) +object(Foo)#3 (%d) { + ["bar"]=> + object(stdClass)#4 (%d) { + } } diff --git a/Zend/tests/readonly_props/readonly_clone_success3.phpt b/Zend/tests/readonly_props/readonly_clone_success3.phpt index a4b5626203c6f..93c3bd8f47f62 100644 --- a/Zend/tests/readonly_props/readonly_clone_success3.phpt +++ b/Zend/tests/readonly_props/readonly_clone_success3.phpt @@ -1,33 +1,37 @@ --TEST-- -Test that __clone() unset properties +__clone() can indirectly modify unlocked readonly properties --FILE-- bar); + $this->bar['bar'] = 'bar'; } } -$foo = new Foo(new stdClass()); -$foo2 = clone $foo; - -var_dump($foo); -var_dump($foo2); +$foo = new Foo([]); +// First call fills the cache slot +var_dump(clone $foo); +var_dump(clone $foo); ?> --EXPECTF-- -object(Foo)#1 (%d) { +object(Foo)#2 (%d) { ["bar"]=> - object(stdClass)#2 (%d) { + array(1) { + ["bar"]=> + string(3) "bar" } } -object(Foo)#3 (%d) { +object(Foo)#2 (%d) { ["bar"]=> - uninitialized(stdClass) + array(1) { + ["bar"]=> + string(3) "bar" + } } diff --git a/Zend/zend_execute.c b/Zend/zend_execute.c index ebf0ef8628578..553cd3e5ad9d4 100644 --- a/Zend/zend_execute.c +++ b/Zend/zend_execute.c @@ -3167,7 +3167,6 @@ static zend_always_inline void zend_fetch_property_address(zval *result, zval *c ZVAL_COPY(result, ptr); } else if (Z_PROP_FLAG_P(ptr) & IS_PROP_REINITABLE) { Z_PROP_FLAG_P(ptr) &= ~IS_PROP_REINITABLE; - ZVAL_COPY(result, ptr); } else { zend_readonly_property_modification_error(prop_info); ZVAL_ERROR(result); From c2e48fc6e1c00b92976e6bff627c329428f99e40 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?M=C3=A1t=C3=A9=20Kocsis?= Date: Fri, 24 Feb 2023 15:29:20 +0100 Subject: [PATCH 3/6] Review fixes --- .../readonly_clone_success4.phpt | 37 +++++++++++++++++++ Zend/zend_objects.c | 10 ++--- 2 files changed, 42 insertions(+), 5 deletions(-) create mode 100644 Zend/tests/readonly_props/readonly_clone_success4.phpt diff --git a/Zend/tests/readonly_props/readonly_clone_success4.phpt b/Zend/tests/readonly_props/readonly_clone_success4.phpt new file mode 100644 index 0000000000000..8f40734dc46db --- /dev/null +++ b/Zend/tests/readonly_props/readonly_clone_success4.phpt @@ -0,0 +1,37 @@ +--TEST-- +Readonly property can be reset once during cloning even after a type error +--FILE-- +bar = "foo"; + } catch (Error $e) { + echo $e->getMessage() . "\n"; + } + + $this->bar = 1; + } +} + +$foo = new Foo(); + +var_dump(clone $foo); +var_dump(clone $foo); + +?> +--EXPECTF-- +Cannot assign string to property Foo::$bar of type int +object(Foo)#%d (%d) { + ["bar"]=> + int(1) +} +Cannot assign string to property Foo::$bar of type int +object(Foo)#%d (%d) { + ["bar"]=> + int(1) +} diff --git a/Zend/zend_objects.c b/Zend/zend_objects.c index 6c3bd1a12c75d..4d40b76854df0 100644 --- a/Zend/zend_objects.c +++ b/Zend/zend_objects.c @@ -204,6 +204,7 @@ ZEND_API void ZEND_FASTCALL zend_objects_clone_members(zend_object *new_object, ZVAL_COPY_VALUE_PROP(dst, src); zval_add_ref(dst); if (has_clone_method) { + /* Unconditionally add the IS_PROP_REINITABLE flag to avoid a potential cache miss of property_info */ Z_PROP_FLAG_P(dst) |= IS_PROP_REINITABLE; } @@ -252,6 +253,7 @@ ZEND_API void ZEND_FASTCALL zend_objects_clone_members(zend_object *new_object, zval_add_ref(&new_prop); } if (has_clone_method) { + /* Unconditionally add the IS_PROP_REINITABLE flag to avoid a potential cache miss of property_info */ Z_PROP_FLAG_P(&new_prop) |= IS_PROP_REINITABLE; } if (EXPECTED(key)) { @@ -268,11 +270,9 @@ ZEND_API void ZEND_FASTCALL zend_objects_clone_members(zend_object *new_object, if (ZEND_CLASS_HAS_READONLY_PROPS(new_object->ce)) { for (uint32_t i = 0; i < new_object->ce->default_properties_count; i++) { - zend_property_info *prop_info = new_object->ce->properties_info_table[i]; - if (prop_info && (prop_info->flags & ZEND_ACC_READONLY)) { - zval * prop = OBJ_PROP_NUM(new_object, i); - Z_PROP_FLAG_P(prop) &= ~IS_PROP_REINITABLE; - } + zval* prop = OBJ_PROP_NUM(new_object, i); + /* Unconditionally remove the IS_PROP_REINITABLE flag to avoid a potential cache miss of property_info */ + Z_PROP_FLAG_P(prop) &= ~IS_PROP_REINITABLE; } } From 556ed13b8f56fb69e0996df9b18b1d1407662249 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?M=C3=A1t=C3=A9=20Kocsis?= Date: Sat, 25 Feb 2023 16:47:11 +0100 Subject: [PATCH 4/6] Make it possible to reinitialize readonly properties after a type error --- .../readonly_clone_success4.phpt | 6 ++++-- Zend/zend_execute.c | 17 ++++++++------- Zend/zend_object_handlers.c | 21 +++++++++---------- 3 files changed, 23 insertions(+), 21 deletions(-) diff --git a/Zend/tests/readonly_props/readonly_clone_success4.phpt b/Zend/tests/readonly_props/readonly_clone_success4.phpt index 8f40734dc46db..91a117fc2dd45 100644 --- a/Zend/tests/readonly_props/readonly_clone_success4.phpt +++ b/Zend/tests/readonly_props/readonly_clone_success4.phpt @@ -4,7 +4,9 @@ Readonly property can be reset once during cloning even after a type error flags & ZEND_ACC_READONLY)) { if (Z_PROP_FLAG_P(property_val) & IS_PROP_REINITABLE) { Z_PROP_FLAG_P(property_val) &= ~IS_PROP_REINITABLE; } else { + zval_ptr_dtor(&tmp); zend_readonly_property_modification_error(info); return &EG(uninitialized_zval); } } - ZVAL_DEREF(value); - ZVAL_COPY(&tmp, value); - - if (UNEXPECTED(!i_zend_verify_property_type(info, &tmp, EX_USES_STRICT_TYPES()))) { - zval_ptr_dtor(&tmp); - return &EG(uninitialized_zval); - } - return zend_assign_to_variable(property_val, &tmp, IS_TMP_VAR, EX_USES_STRICT_TYPES()); } diff --git a/Zend/zend_object_handlers.c b/Zend/zend_object_handlers.c index aaa0e731df517..cc02680674c7b 100644 --- a/Zend/zend_object_handlers.c +++ b/Zend/zend_object_handlers.c @@ -812,17 +812,6 @@ ZEND_API zval *zend_std_write_property(zend_object *zobj, zend_string *name, zva Z_TRY_ADDREF_P(value); if (UNEXPECTED(prop_info)) { - if (UNEXPECTED(prop_info->flags & ZEND_ACC_READONLY)) { - if (Z_PROP_FLAG_P(variable_ptr) & IS_PROP_REINITABLE) { - Z_PROP_FLAG_P(variable_ptr) &= ~IS_PROP_REINITABLE; - } else { - Z_TRY_DELREF_P(value); - zend_readonly_property_modification_error(prop_info); - variable_ptr = &EG(error_zval); - goto exit; - } - } - ZVAL_COPY_VALUE(&tmp, value); // Increase refcount to prevent object from being released in __toString() GC_ADDREF(zobj); @@ -839,6 +828,16 @@ ZEND_API zval *zend_std_write_property(zend_object *zobj, zend_string *name, zva variable_ptr = &EG(error_zval); goto exit; } + if (UNEXPECTED(prop_info->flags & ZEND_ACC_READONLY)) { + if (Z_PROP_FLAG_P(variable_ptr) & IS_PROP_REINITABLE) { + Z_PROP_FLAG_P(variable_ptr) &= ~IS_PROP_REINITABLE; + } else { + Z_TRY_DELREF_P(value); + zend_readonly_property_modification_error(prop_info); + variable_ptr = &EG(error_zval); + goto exit; + } + } value = &tmp; } From 75d4d9ddbded74d9c1a6ae332b48b92e506719d8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?M=C3=A1t=C3=A9=20Kocsis?= Date: Tue, 28 Feb 2023 00:09:43 +0100 Subject: [PATCH 5/6] Fix tests --- .../readonly_clone_success4.phpt | 6 ++--- .../readonly_coercion_type_error.phpt | 23 +++++++++++++++++++ Zend/zend_object_handlers.c | 2 +- 3 files changed, 27 insertions(+), 4 deletions(-) create mode 100644 Zend/tests/readonly_props/readonly_coercion_type_error.phpt diff --git a/Zend/tests/readonly_props/readonly_clone_success4.phpt b/Zend/tests/readonly_props/readonly_clone_success4.phpt index 91a117fc2dd45..40b7a29e31265 100644 --- a/Zend/tests/readonly_props/readonly_clone_success4.phpt +++ b/Zend/tests/readonly_props/readonly_clone_success4.phpt @@ -16,7 +16,7 @@ class Foo { echo $e->getMessage() . "\n"; } - $this->bar = 1; + $this->bar = 2; } } @@ -30,10 +30,10 @@ var_dump(clone $foo); Cannot assign string to property Foo::$bar of type int object(Foo)#%d (%d) { ["bar"]=> - int(1) + int(2) } Cannot assign string to property Foo::$bar of type int object(Foo)#%d (%d) { ["bar"]=> - int(1) + int(2) } diff --git a/Zend/tests/readonly_props/readonly_coercion_type_error.phpt b/Zend/tests/readonly_props/readonly_coercion_type_error.phpt new file mode 100644 index 0000000000000..d41211d8e9981 --- /dev/null +++ b/Zend/tests/readonly_props/readonly_coercion_type_error.phpt @@ -0,0 +1,23 @@ +--TEST-- +Test failing readonly assignment with coercion +--FILE-- +bar = 'bar'; + try { + $this->bar = 42; + } catch (Error $e) { + echo $e->getMessage(), "\n"; + } + } +} + +new Foo(); + +?> +--EXPECTF-- +Cannot modify readonly property Foo::$bar diff --git a/Zend/zend_object_handlers.c b/Zend/zend_object_handlers.c index cc02680674c7b..5545f7ef7c646 100644 --- a/Zend/zend_object_handlers.c +++ b/Zend/zend_object_handlers.c @@ -832,7 +832,7 @@ ZEND_API zval *zend_std_write_property(zend_object *zobj, zend_string *name, zva if (Z_PROP_FLAG_P(variable_ptr) & IS_PROP_REINITABLE) { Z_PROP_FLAG_P(variable_ptr) &= ~IS_PROP_REINITABLE; } else { - Z_TRY_DELREF_P(value); + zval_ptr_dtor(&tmp); zend_readonly_property_modification_error(prop_info); variable_ptr = &EG(error_zval); goto exit; From 2cf102c4aaa821ec2e92fb28a8623b3836fa04ba Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?M=C3=A1t=C3=A9=20Kocsis?= Date: Tue, 28 Feb 2023 22:51:23 +0100 Subject: [PATCH 6/6] Use bit-shift --- Zend/zend_types.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Zend/zend_types.h b/Zend/zend_types.h index 70a29342ad606..faed815401948 100644 --- a/Zend/zend_types.h +++ b/Zend/zend_types.h @@ -1273,8 +1273,8 @@ static zend_always_inline uint32_t zval_delref_p(zval* pz) { * (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 IS_PROP_REINITABLE 2 /* It has impact only on readonly properties */ +#define IS_PROP_UNINIT (1<<0) +#define IS_PROP_REINITABLE (1<<1) /* It has impact only on readonly properties */ #define Z_PROP_FLAG_P(z) Z_EXTRA_P(z) #define ZVAL_COPY_VALUE_PROP(z, v) \ do { *(z) = *(v); } while (0)