From fd33f3bf54dd0c58425f575ee756de808b6fe63f Mon Sep 17 00:00:00 2001 From: Ilija Tovilo Date: Thu, 18 Jul 2024 18:03:34 +0200 Subject: [PATCH 1/3] Disallow indirect modification on readonly properties within __clone() Indirect modification isn't allowed in __construct() because it allows references to leak, so it doesn't make much sense to allow it in __clone(). --- .../readonly_props/readonly_clone_error5.phpt | 18 ++------- .../readonly_props/readonly_clone_error7.phpt | 33 +++++++++++++++++ .../readonly_clone_success3.phpt | 37 ------------------- Zend/zend_execute.c | 2 - Zend/zend_object_handlers.c | 2 - 5 files changed, 36 insertions(+), 56 deletions(-) create mode 100644 Zend/tests/readonly_props/readonly_clone_error7.phpt delete mode 100644 Zend/tests/readonly_props/readonly_clone_success3.phpt diff --git a/Zend/tests/readonly_props/readonly_clone_error5.phpt b/Zend/tests/readonly_props/readonly_clone_error5.phpt index 84084260e9f64..a6a10fb8a27ce 100644 --- a/Zend/tests/readonly_props/readonly_clone_error5.phpt +++ b/Zend/tests/readonly_props/readonly_clone_error5.phpt @@ -57,20 +57,8 @@ try { } ?> ---EXPECTF-- -object(TestSetOnce)#%d (1) { - ["prop"]=> - array(1) { - [0]=> - int(1) - } -} -object(TestSetOnce)#%d (1) { - ["prop"]=> - array(1) { - [0]=> - int(1) - } -} +--EXPECT-- +Cannot indirectly modify readonly property TestSetOnce::$prop +Cannot indirectly modify readonly property TestSetOnce::$prop Cannot indirectly modify readonly property TestSetTwice::$prop Cannot indirectly modify readonly property TestSetTwice::$prop diff --git a/Zend/tests/readonly_props/readonly_clone_error7.phpt b/Zend/tests/readonly_props/readonly_clone_error7.phpt new file mode 100644 index 0000000000000..e34ad9eb3d719 --- /dev/null +++ b/Zend/tests/readonly_props/readonly_clone_error7.phpt @@ -0,0 +1,33 @@ +--TEST-- +__clone() cannot indirectly modify unlocked readonly properties +--FILE-- +bar['bar'] = 'bar'; + } +} + +$foo = new Foo([]); +// First call fills the cache slot +try { + var_dump(clone $foo); +} catch (Error $e) { + echo $e->getMessage(), "\n"; +} +try { + var_dump(clone $foo); +} catch (Error $e) { + echo $e->getMessage(), "\n"; +} + +?> +--EXPECT-- +Cannot indirectly modify readonly property Foo::$bar +Cannot indirectly modify readonly property Foo::$bar diff --git a/Zend/tests/readonly_props/readonly_clone_success3.phpt b/Zend/tests/readonly_props/readonly_clone_success3.phpt deleted file mode 100644 index 93c3bd8f47f62..0000000000000 --- a/Zend/tests/readonly_props/readonly_clone_success3.phpt +++ /dev/null @@ -1,37 +0,0 @@ ---TEST-- -__clone() can indirectly modify unlocked readonly properties ---FILE-- -bar['bar'] = 'bar'; - } -} - -$foo = new Foo([]); -// First call fills the cache slot -var_dump(clone $foo); -var_dump(clone $foo); - -?> ---EXPECTF-- -object(Foo)#2 (%d) { - ["bar"]=> - array(1) { - ["bar"]=> - string(3) "bar" - } -} -object(Foo)#2 (%d) { - ["bar"]=> - array(1) { - ["bar"]=> - string(3) "bar" - } -} diff --git a/Zend/zend_execute.c b/Zend/zend_execute.c index 89904af06d2ad..383523fd5f1a6 100644 --- a/Zend/zend_execute.c +++ b/Zend/zend_execute.c @@ -3364,8 +3364,6 @@ 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; } else { zend_readonly_property_indirect_modification_error(prop_info); ZVAL_ERROR(result); diff --git a/Zend/zend_object_handlers.c b/Zend/zend_object_handlers.c index 3d2ef0148a650..b365189a11dd5 100644 --- a/Zend/zend_object_handlers.c +++ b/Zend/zend_object_handlers.c @@ -696,8 +696,6 @@ 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_indirect_modification_error(prop_info); retval = &EG(uninitialized_zval); From f20966a17116f5fec712910c7effb31924c19871 Mon Sep 17 00:00:00 2001 From: Ilija Tovilo Date: Thu, 8 Aug 2024 23:48:46 +0200 Subject: [PATCH 2/3] Implement JIT changes --- ext/opcache/jit/zend_jit_ir.c | 28 ++++------------------------ 1 file changed, 4 insertions(+), 24 deletions(-) diff --git a/ext/opcache/jit/zend_jit_ir.c b/ext/opcache/jit/zend_jit_ir.c index 2c93c31cd9dfc..174b07ac10c1d 100644 --- a/ext/opcache/jit/zend_jit_ir.c +++ b/ext/opcache/jit/zend_jit_ir.c @@ -13954,15 +13954,6 @@ static int zend_jit_fetch_obj(zend_jit_ctx *jit, ir_IF_FALSE_cold(if_prop_obj); - ir_ref extra_addr = ir_ADD_OFFSET(jit_ZVAL_ADDR(jit, prop_addr), offsetof(zval, u2.extra)); - ir_ref extra = ir_LOAD_U32(extra_addr); - ir_ref if_reinitable = ir_IF(ir_AND_U32(extra, ir_CONST_U32(IS_PROP_REINITABLE))); - ir_IF_TRUE(if_reinitable); - ir_STORE(extra_addr, ir_AND_U32(extra, ir_CONST_U32(~IS_PROP_REINITABLE))); - ir_ref reinit_path = ir_END(); - - ir_IF_FALSE(if_reinitable); - jit_SET_EX_OPLINE(jit, opline); ir_CALL_1(IR_VOID, ir_CONST_FC_FUNC(zend_readonly_property_indirect_modification_error), prop_info_ref); jit_set_Z_TYPE_INFO(jit, res_addr, _IS_ERROR); @@ -13970,7 +13961,6 @@ static int zend_jit_fetch_obj(zend_jit_ctx *jit, if (flags == ZEND_FETCH_DIM_WRITE) { ir_IF_FALSE_cold(if_readonly); - ir_MERGE_WITH(reinit_path); jit_SET_EX_OPLINE(jit, opline); ir_CALL_2(IR_VOID, ir_CONST_FC_FUNC(zend_jit_check_array_promotion), prop_ref, prop_info_ref); @@ -13978,7 +13968,6 @@ static int zend_jit_fetch_obj(zend_jit_ctx *jit, ir_IF_FALSE(if_has_prop_info); } else if (flags == ZEND_FETCH_REF) { ir_IF_FALSE_cold(if_readonly); - ir_MERGE_WITH(reinit_path); ir_CALL_3(IR_VOID, ir_CONST_FC_FUNC(zend_jit_create_typed_ref), prop_ref, prop_info_ref, @@ -13986,14 +13975,11 @@ static int zend_jit_fetch_obj(zend_jit_ctx *jit, ir_END_list(end_inputs); ir_IF_FALSE(if_has_prop_info); } else { - ir_ref list = reinit_path; - ZEND_ASSERT(flags == 0); ir_IF_FALSE(if_has_prop_info); - ir_END_list(list); + ir_ref no_prop_info_path = ir_END(); ir_IF_FALSE(if_readonly); - ir_END_list(list); - ir_MERGE_list(list); + ir_MERGE_WITH(no_prop_info_path); } } } else { @@ -14031,19 +14017,12 @@ static int zend_jit_fetch_obj(zend_jit_ctx *jit, ir_END_list(end_inputs); ir_IF_FALSE_cold(if_prop_obj); - - ir_ref extra_addr = ir_ADD_OFFSET(jit_ZVAL_ADDR(jit, prop_addr), offsetof(zval, u2.extra)); - ir_ref extra = ir_LOAD_U32(extra_addr); - ir_ref if_reinitable = ir_IF(ir_AND_U32(extra, ir_CONST_U32(IS_PROP_REINITABLE))); - - ir_IF_FALSE(if_reinitable); jit_SET_EX_OPLINE(jit, opline); ir_CALL_1(IR_VOID, ir_CONST_FC_FUNC(zend_readonly_property_indirect_modification_error), ir_CONST_ADDR(prop_info)); jit_set_Z_TYPE_INFO(jit, res_addr, _IS_ERROR); ir_END_list(end_inputs); - ir_IF_TRUE(if_reinitable); - ir_STORE(extra_addr, ir_AND_U32(extra, ir_CONST_U32(~IS_PROP_REINITABLE))); + goto result_fetched; } if (opline->opcode == ZEND_FETCH_OBJ_W @@ -14117,6 +14096,7 @@ static int zend_jit_fetch_obj(zend_jit_ctx *jit, } } +result_fetched: if (op1_avoid_refcounting) { SET_STACK_REG(JIT_G(current_frame)->stack, EX_VAR_TO_NUM(opline->op1.var), ZREG_NONE); } From 2cf7993766baa3bdffdc918233cee2d6a98d7752 Mon Sep 17 00:00:00 2001 From: Ilija Tovilo Date: Fri, 9 Aug 2024 11:55:05 +0200 Subject: [PATCH 3/3] Add UPGRADING note --- UPGRADING | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/UPGRADING b/UPGRADING index 5430923d87c2c..99516dbb1a318 100644 --- a/UPGRADING +++ b/UPGRADING @@ -33,6 +33,10 @@ PHP 8.4 UPGRADE NOTES now 13 bytes longer. Total length is platform-dependent. . Encountering recursion during comparison now results in a Error exception, rather than a fatal error. + . Indirect modification of readonly properties within __clone() is no longer + allowed, e.g. $ref = &$this->readonly. This was already forbidden for + readonly initialization, and was an oversight in the "readonly + reinitialization during cloning" implementation. - DOM: . Added DOMNode::compareDocumentPosition() and DOMNode::DOCUMENT_POSITION_*