From 40ef5b8fa56438a47fed67df79efb4bdf9638185 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?M=C3=A1t=C3=A9=20Kocsis?= Date: Fri, 3 Mar 2023 11:23:11 +0100 Subject: [PATCH 1/3] Do not allow sideeffects when readonly property modification fails --- .../readonly_assign_no_sideeffect.phpt | 34 +++++++++++++++++++ Zend/zend_execute.c | 15 ++++---- Zend/zend_object_handlers.c | 18 +++++----- 3 files changed, 52 insertions(+), 15 deletions(-) create mode 100644 Zend/tests/readonly_props/readonly_assign_no_sideeffect.phpt diff --git a/Zend/tests/readonly_props/readonly_assign_no_sideeffect.phpt b/Zend/tests/readonly_props/readonly_assign_no_sideeffect.phpt new file mode 100644 index 0000000000000..5cc35d2efa6d3 --- /dev/null +++ b/Zend/tests/readonly_props/readonly_assign_no_sideeffect.phpt @@ -0,0 +1,34 @@ +--TEST-- +Test that there can be no side-effect when a readonly property modification fails +--FILE-- +bar = new S(); + } +} + +class S { + public function __toString() { + var_dump("Side-effect in __toString()"); + return ""; + } +} + +$foo = new Foo(""); + +try { + $foo->write(); +} catch (Error $e) { + echo $e->getMessage() . "\n"; +} + +?> +--EXPECT-- +Cannot modify readonly property Foo::$bar diff --git a/Zend/zend_execute.c b/Zend/zend_execute.c index 052cd0c3f49e9..3ff127bc31491 100644 --- a/Zend/zend_execute.c +++ b/Zend/zend_execute.c @@ -1009,6 +1009,13 @@ static zend_never_inline zval* zend_assign_to_typed_prop(zend_property_info *inf { zval tmp; + if (UNEXPECTED( + (info->flags & ZEND_ACC_READONLY) && !(Z_PROP_FLAG_P(property_val) & IS_PROP_REINITABLE) + )) { + zend_readonly_property_modification_error(info); + return &EG(uninitialized_zval); + } + ZVAL_DEREF(value); ZVAL_COPY(&tmp, value); @@ -1018,13 +1025,7 @@ static zend_never_inline zval* zend_assign_to_typed_prop(zend_property_info *inf } if (UNEXPECTED(info->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); - } + Z_PROP_FLAG_P(property_val) &= ~IS_PROP_REINITABLE; } 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 5545f7ef7c646..a145bd0139402 100644 --- a/Zend/zend_object_handlers.c +++ b/Zend/zend_object_handlers.c @@ -812,6 +812,15 @@ 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) && !(Z_PROP_FLAG_P(variable_ptr) & IS_PROP_REINITABLE) + )) { + 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); @@ -829,14 +838,7 @@ ZEND_API zval *zend_std_write_property(zend_object *zobj, zend_string *name, zva 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 { - zval_ptr_dtor(&tmp); - zend_readonly_property_modification_error(prop_info); - variable_ptr = &EG(error_zval); - goto exit; - } + Z_PROP_FLAG_P(variable_ptr) &= ~IS_PROP_REINITABLE; } value = &tmp; } From ebe358cf5f46d6dd0b260ce00cf0efd6e9125b67 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?M=C3=A1t=C3=A9=20Kocsis?= Date: Fri, 3 Mar 2023 13:13:06 +0100 Subject: [PATCH 2/3] Code review fixes --- .../readonly_props/readonly_assign_no_sideeffect.phpt | 3 +-- Zend/zend_execute.c | 8 ++------ Zend/zend_object_handlers.c | 4 +--- 3 files changed, 4 insertions(+), 11 deletions(-) diff --git a/Zend/tests/readonly_props/readonly_assign_no_sideeffect.phpt b/Zend/tests/readonly_props/readonly_assign_no_sideeffect.phpt index 5cc35d2efa6d3..29218d837d7fb 100644 --- a/Zend/tests/readonly_props/readonly_assign_no_sideeffect.phpt +++ b/Zend/tests/readonly_props/readonly_assign_no_sideeffect.phpt @@ -8,8 +8,7 @@ class Foo { public readonly string $bar ) {} - public function write() - { + public function write() { $this->bar = new S(); } } diff --git a/Zend/zend_execute.c b/Zend/zend_execute.c index 3ff127bc31491..d7d41937867c9 100644 --- a/Zend/zend_execute.c +++ b/Zend/zend_execute.c @@ -1009,9 +1009,7 @@ static zend_never_inline zval* zend_assign_to_typed_prop(zend_property_info *inf { zval tmp; - if (UNEXPECTED( - (info->flags & ZEND_ACC_READONLY) && !(Z_PROP_FLAG_P(property_val) & IS_PROP_REINITABLE) - )) { + if (UNEXPECTED((info->flags & ZEND_ACC_READONLY) && !(Z_PROP_FLAG_P(property_val) & IS_PROP_REINITABLE))) { zend_readonly_property_modification_error(info); return &EG(uninitialized_zval); } @@ -1024,9 +1022,7 @@ static zend_never_inline zval* zend_assign_to_typed_prop(zend_property_info *inf return &EG(uninitialized_zval); } - if (UNEXPECTED(info->flags & ZEND_ACC_READONLY)) { - Z_PROP_FLAG_P(property_val) &= ~IS_PROP_REINITABLE; - } + Z_PROP_FLAG_P(property_val) &= ~IS_PROP_REINITABLE; 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 a145bd0139402..6f90b2ef9dd34 100644 --- a/Zend/zend_object_handlers.c +++ b/Zend/zend_object_handlers.c @@ -837,9 +837,7 @@ 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)) { - Z_PROP_FLAG_P(variable_ptr) &= ~IS_PROP_REINITABLE; - } + Z_PROP_FLAG_P(variable_ptr) &= ~IS_PROP_REINITABLE; value = &tmp; } From 1ca556271e03e7b194ebbb1a1651883ae06c4bb6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?M=C3=A1t=C3=A9=20Kocsis?= Date: Fri, 3 Mar 2023 19:52:08 +0100 Subject: [PATCH 3/3] New line fix --- Zend/zend_object_handlers.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/Zend/zend_object_handlers.c b/Zend/zend_object_handlers.c index 6f90b2ef9dd34..872c3df5a1011 100644 --- a/Zend/zend_object_handlers.c +++ b/Zend/zend_object_handlers.c @@ -812,9 +812,7 @@ 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) && !(Z_PROP_FLAG_P(variable_ptr) & IS_PROP_REINITABLE) - )) { + if (UNEXPECTED((prop_info->flags & ZEND_ACC_READONLY) && !(Z_PROP_FLAG_P(variable_ptr) & IS_PROP_REINITABLE))) { Z_TRY_DELREF_P(value); zend_readonly_property_modification_error(prop_info); variable_ptr = &EG(error_zval);