From ee15cafc6f5f40ddf1700ce00071855b324494f5 Mon Sep 17 00:00:00 2001 From: Ilija Tovilo Date: Thu, 2 Mar 2023 00:05:50 +0100 Subject: [PATCH 1/2] Fix readonly+clone JIT issues --- .../readonly_props/readonly_clone_error4.phpt | 44 +++++++++++ .../readonly_props/readonly_clone_error5.phpt | 76 +++++++++++++++++++ ext/opcache/jit/zend_jit_arm64.dasc | 19 +++++ ext/opcache/jit/zend_jit_helpers.c | 29 +++++-- ext/opcache/jit/zend_jit_x86.dasc | 21 ++++- 5 files changed, 182 insertions(+), 7 deletions(-) create mode 100644 Zend/tests/readonly_props/readonly_clone_error4.phpt create mode 100644 Zend/tests/readonly_props/readonly_clone_error5.phpt diff --git a/Zend/tests/readonly_props/readonly_clone_error4.phpt b/Zend/tests/readonly_props/readonly_clone_error4.phpt new file mode 100644 index 0000000000000..f9edcbcbaa18e --- /dev/null +++ b/Zend/tests/readonly_props/readonly_clone_error4.phpt @@ -0,0 +1,44 @@ +--TEST-- +Readonly property cannot be op-assigned 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"; +} + +try { + clone $foo; +} catch (Error $exception) { + echo $exception->getMessage() . "\n"; +} + +?> +--EXPECT-- +object(Foo)#2 (1) { + ["bar"]=> + int(3) +} +Cannot modify readonly property Foo::$bar +object(Foo)#2 (1) { + ["bar"]=> + int(3) +} +Cannot modify readonly property Foo::$bar diff --git a/Zend/tests/readonly_props/readonly_clone_error5.phpt b/Zend/tests/readonly_props/readonly_clone_error5.phpt new file mode 100644 index 0000000000000..c6651b54edb85 --- /dev/null +++ b/Zend/tests/readonly_props/readonly_clone_error5.phpt @@ -0,0 +1,76 @@ +--TEST-- +Readonly property clone indirect variation for JIT +--FILE-- +prop[] = 1; + } +} + +trait CloneSetTwiceTrait { + public function __clone() { + $this->prop[] = 1; + $this->prop[] = 2; + } +} + +class TestSetOnce { + use CloneSetOnceTrait; + public readonly array $prop; + public function __construct() { + $this->prop = []; + } +} + +class TestSetTwice { + use CloneSetTwiceTrait; + public readonly array $prop; + public function __construct() { + $this->prop = []; + } +} + +try { + var_dump(clone (new TestSetOnce())); +} catch (Error $e) { + echo $e->getMessage(), "\n"; +} + +try { + var_dump(clone (new TestSetOnce())); +} catch (Error $e) { + echo $e->getMessage(), "\n"; +} + +try { + var_dump(clone (new TestSetTwice())); +} catch (Error $e) { + echo $e->getMessage(), "\n"; +} + +try { + var_dump(clone (new TestSetTwice())); +} catch (Error $e) { + echo $e->getMessage(), "\n"; +} + +?> +--EXPECT-- +object(TestSetOnce)#2 (1) { + ["prop"]=> + array(1) { + [0]=> + int(1) + } +} +object(TestSetOnce)#1 (1) { + ["prop"]=> + array(1) { + [0]=> + int(1) + } +} +Cannot modify readonly property TestSetTwice::$prop +Cannot modify readonly property TestSetTwice::$prop diff --git a/ext/opcache/jit/zend_jit_arm64.dasc b/ext/opcache/jit/zend_jit_arm64.dasc index 8df7d4f88eae7..1e630acbb0c46 100644 --- a/ext/opcache/jit/zend_jit_arm64.dasc +++ b/ext/opcache/jit/zend_jit_arm64.dasc @@ -12242,6 +12242,15 @@ static int zend_jit_fetch_obj(dasm_State **Dst, | SET_ZVAL_TYPE_INFO res_addr, IS_OBJECT_EX, TMP1w, TMP2 | b >9 |2: + | ldr REG0w, [FCARG1x, #offsetof(zval, u2.extra)] + | TST_32_WITH_CONST REG0w, IS_PROP_REINITABLE, TMP1w + | beq >6 + | SET_ZVAL_PTR res_addr, FCARG1x, TMP1 + | SET_ZVAL_TYPE_INFO res_addr, IS_INDIRECT, TMP1w, TMP2 + | and REG0w, REG0w, #(~IS_PROP_REINITABLE) + | str REG0w, [FCARG1x, #offsetof(zval, u2.extra)] + | b >9 + |6: | mov FCARG1x, FCARG2x | SET_EX_OPLINE opline, REG0 | EXT_CALL zend_readonly_property_modification_error, REG0 @@ -12295,6 +12304,16 @@ static int zend_jit_fetch_obj(dasm_State **Dst, | b >9 |.cold_code |4: + | ldr REG0w, [FCARG1x, #(prop_info->offset + offsetof(zval, u2.extra))] + | TST_32_WITH_CONST REG0w, IS_PROP_REINITABLE, TMP1w + | beq >6 + | LOAD_ZVAL_ADDR FCARG1x, prop_addr + | SET_ZVAL_PTR res_addr, FCARG1x, TMP1 + | SET_ZVAL_TYPE_INFO res_addr, IS_INDIRECT, TMP1w, TMP2 + | and REG0w, REG0w, #(~IS_PROP_REINITABLE) + | str REG0w, [FCARG1x, #(prop_info->offset + offsetof(zval, u2.extra))] + | b >9 + |6: | LOAD_ADDR FCARG1x, prop_info | SET_EX_OPLINE opline, REG0 | EXT_CALL zend_readonly_property_modification_error, REG0 diff --git a/ext/opcache/jit/zend_jit_helpers.c b/ext/opcache/jit/zend_jit_helpers.c index f3845f25b5bd3..7636c7810531c 100644 --- a/ext/opcache/jit/zend_jit_helpers.c +++ b/ext/opcache/jit/zend_jit_helpers.c @@ -2513,7 +2513,7 @@ static void ZEND_FASTCALL zend_jit_assign_to_typed_prop(zval *property_val, zend value = &EG(uninitialized_zval); } - if (UNEXPECTED(info->flags & ZEND_ACC_READONLY)) { + if (UNEXPECTED((info->flags & ZEND_ACC_READONLY) && !(Z_PROP_FLAG_P(property_val) & IS_PROP_REINITABLE))) { zend_readonly_property_modification_error(info); if (result) { ZVAL_UNDEF(result); @@ -2532,6 +2532,8 @@ static void ZEND_FASTCALL zend_jit_assign_to_typed_prop(zval *property_val, zend return; } + Z_PROP_FLAG_P(property_val) &= ~IS_PROP_REINITABLE; + value = zend_assign_to_variable(property_val, &tmp, IS_TMP_VAR, EX_USES_STRICT_TYPES()); if (result) { ZVAL_COPY_DEREF(result, value); @@ -2570,7 +2572,7 @@ static void ZEND_FASTCALL zend_jit_assign_op_to_typed_prop(zval *zptr, zend_prop zend_execute_data *execute_data = EG(current_execute_data); zval z_copy; - if (UNEXPECTED(prop_info->flags & ZEND_ACC_READONLY)) { + if (UNEXPECTED((prop_info->flags & ZEND_ACC_READONLY) && !(Z_PROP_FLAG_P(zptr) & IS_PROP_REINITABLE))) { zend_readonly_property_modification_error(prop_info); return; } @@ -2585,6 +2587,7 @@ static void ZEND_FASTCALL zend_jit_assign_op_to_typed_prop(zval *zptr, zend_prop binary_op(&z_copy, zptr, value); if (EXPECTED(zend_verify_property_type(prop_info, &z_copy, EX_USES_STRICT_TYPES()))) { + Z_PROP_FLAG_P(zptr) &= ~IS_PROP_REINITABLE; zval_ptr_dtor(zptr); ZVAL_COPY_VALUE(zptr, &z_copy); } else { @@ -2664,7 +2667,7 @@ static void ZEND_FASTCALL zend_jit_inc_typed_prop(zval *var_ptr, zend_property_i { ZEND_ASSERT(Z_TYPE_P(var_ptr) != IS_UNDEF); - if (UNEXPECTED((prop_info->flags & ZEND_ACC_READONLY))) { + if (UNEXPECTED((prop_info->flags & ZEND_ACC_READONLY) && !(Z_PROP_FLAG_P(var_ptr) & IS_PROP_REINITABLE))) { zend_readonly_property_modification_error(prop_info); return; } @@ -2681,11 +2684,14 @@ static void ZEND_FASTCALL zend_jit_inc_typed_prop(zval *var_ptr, zend_property_i if (!(ZEND_TYPE_FULL_MASK(prop_info->type) & MAY_BE_DOUBLE)) { zend_long val = _zend_jit_throw_inc_prop_error(prop_info); ZVAL_LONG(var_ptr, val); + } else { + Z_PROP_FLAG_P(var_ptr) &= ~IS_PROP_REINITABLE; } } else if (UNEXPECTED(!zend_verify_property_type(prop_info, var_ptr, EX_USES_STRICT_TYPES()))) { zval_ptr_dtor(var_ptr); ZVAL_COPY_VALUE(var_ptr, &tmp); } else { + Z_PROP_FLAG_P(var_ptr) &= ~IS_PROP_REINITABLE; zval_ptr_dtor(&tmp); } } @@ -2694,7 +2700,7 @@ static void ZEND_FASTCALL zend_jit_dec_typed_prop(zval *var_ptr, zend_property_i { ZEND_ASSERT(Z_TYPE_P(var_ptr) != IS_UNDEF); - if (UNEXPECTED((prop_info->flags & ZEND_ACC_READONLY))) { + if (UNEXPECTED((prop_info->flags & ZEND_ACC_READONLY) && !(Z_PROP_FLAG_P(var_ptr) & IS_PROP_REINITABLE))) { zend_readonly_property_modification_error(prop_info); return; } @@ -2711,11 +2717,14 @@ static void ZEND_FASTCALL zend_jit_dec_typed_prop(zval *var_ptr, zend_property_i if (!(ZEND_TYPE_FULL_MASK(prop_info->type) & MAY_BE_DOUBLE)) { zend_long val = _zend_jit_throw_dec_prop_error(prop_info); ZVAL_LONG(var_ptr, val); + } else { + Z_PROP_FLAG_P(var_ptr) &= ~IS_PROP_REINITABLE; } } else if (UNEXPECTED(!zend_verify_property_type(prop_info, var_ptr, EX_USES_STRICT_TYPES()))) { zval_ptr_dtor(var_ptr); ZVAL_COPY_VALUE(var_ptr, &tmp); } else { + Z_PROP_FLAG_P(var_ptr) &= ~IS_PROP_REINITABLE; zval_ptr_dtor(&tmp); } } @@ -2738,7 +2747,7 @@ static void ZEND_FASTCALL zend_jit_post_inc_typed_prop(zval *var_ptr, zend_prope { ZEND_ASSERT(Z_TYPE_P(var_ptr) != IS_UNDEF); - if (UNEXPECTED((prop_info->flags & ZEND_ACC_READONLY))) { + if (UNEXPECTED((prop_info->flags & ZEND_ACC_READONLY) && !(Z_PROP_FLAG_P(var_ptr) & IS_PROP_REINITABLE))) { zend_readonly_property_modification_error(prop_info); if (result) { ZVAL_UNDEF(result); @@ -2757,11 +2766,15 @@ static void ZEND_FASTCALL zend_jit_post_inc_typed_prop(zval *var_ptr, zend_prope if (!(ZEND_TYPE_FULL_MASK(prop_info->type) & MAY_BE_DOUBLE)) { zend_long val = _zend_jit_throw_inc_prop_error(prop_info); ZVAL_LONG(var_ptr, val); + } else { + Z_PROP_FLAG_P(var_ptr) &= ~IS_PROP_REINITABLE; } } else if (UNEXPECTED(!zend_verify_property_type(prop_info, var_ptr, EX_USES_STRICT_TYPES()))) { zval_ptr_dtor(var_ptr); ZVAL_COPY_VALUE(var_ptr, result); ZVAL_UNDEF(result); + } else { + Z_PROP_FLAG_P(var_ptr) &= ~IS_PROP_REINITABLE; } } @@ -2769,7 +2782,7 @@ static void ZEND_FASTCALL zend_jit_post_dec_typed_prop(zval *var_ptr, zend_prope { ZEND_ASSERT(Z_TYPE_P(var_ptr) != IS_UNDEF); - if (UNEXPECTED((prop_info->flags & ZEND_ACC_READONLY))) { + if (UNEXPECTED((prop_info->flags & ZEND_ACC_READONLY) && !(Z_PROP_FLAG_P(var_ptr) & IS_PROP_REINITABLE))) { zend_readonly_property_modification_error(prop_info); if (result) { ZVAL_UNDEF(result); @@ -2788,11 +2801,15 @@ static void ZEND_FASTCALL zend_jit_post_dec_typed_prop(zval *var_ptr, zend_prope if (!(ZEND_TYPE_FULL_MASK(prop_info->type) & MAY_BE_DOUBLE)) { zend_long val = _zend_jit_throw_dec_prop_error(prop_info); ZVAL_LONG(var_ptr, val); + } else { + Z_PROP_FLAG_P(var_ptr) &= ~IS_PROP_REINITABLE; } } else if (UNEXPECTED(!zend_verify_property_type(prop_info, var_ptr, EX_USES_STRICT_TYPES()))) { zval_ptr_dtor(var_ptr); ZVAL_COPY_VALUE(var_ptr, result); ZVAL_UNDEF(result); + } else { + Z_PROP_FLAG_P(var_ptr) &= ~IS_PROP_REINITABLE; } } diff --git a/ext/opcache/jit/zend_jit_x86.dasc b/ext/opcache/jit/zend_jit_x86.dasc index 42785e285f7a3..583dd94d661bf 100644 --- a/ext/opcache/jit/zend_jit_x86.dasc +++ b/ext/opcache/jit/zend_jit_x86.dasc @@ -12983,6 +12983,15 @@ static int zend_jit_fetch_obj(dasm_State **Dst, | SET_ZVAL_TYPE_INFO res_addr, IS_OBJECT_EX | jmp >9 |2: + | mov eax, dword [FCARG1a + offsetof(zval, u2.extra)] + | test eax, IS_PROP_REINITABLE + | jz >6 + | SET_ZVAL_PTR res_addr, FCARG1a + | SET_ZVAL_TYPE_INFO res_addr, IS_INDIRECT + | and eax, ~IS_PROP_REINITABLE + | mov dword [FCARG1a + offsetof(zval, u2.extra)], eax + | jmp >9 + |6: | mov FCARG1a, FCARG2a | SET_EX_OPLINE opline, r0 | EXT_CALL zend_readonly_property_modification_error, r0 @@ -13034,7 +13043,7 @@ static int zend_jit_fetch_obj(dasm_State **Dst, if (opline->opcode == ZEND_FETCH_OBJ_W && (prop_info->flags & ZEND_ACC_READONLY)) { if (!type_loaded) { type_loaded = 1; - | mov edx, dword [FCARG1a + prop_info->offset + 8] + | mov edx, dword [FCARG1a + prop_info->offset + offsetof(zval, u1.type_info)] } | IF_NOT_TYPE dl, IS_OBJECT, >4 | GET_ZVAL_PTR r0, prop_addr @@ -13044,6 +13053,16 @@ static int zend_jit_fetch_obj(dasm_State **Dst, | jmp >9 |.cold_code |4: + | mov eax, dword [FCARG1a + prop_info->offset + offsetof(zval, u2.extra)] + | test eax, IS_PROP_REINITABLE + | jz >6 + | LOAD_ZVAL_ADDR r0, prop_addr + | SET_ZVAL_PTR res_addr, r0 + | SET_ZVAL_TYPE_INFO res_addr, IS_INDIRECT + | and eax, ~IS_PROP_REINITABLE + | mov dword [FCARG1a + prop_info->offset + offsetof(zval, u2.extra)], eax + | jmp >9 + |6: | LOAD_ADDR FCARG1a, prop_info | SET_EX_OPLINE opline, r0 | EXT_CALL zend_readonly_property_modification_error, r0 From 19b723bee27a5e72fd08d87364df61a4dd4bbb48 Mon Sep 17 00:00:00 2001 From: Ilija Tovilo Date: Tue, 7 Mar 2023 12:30:01 +0100 Subject: [PATCH 2/2] Simplify assembly by reusing hot path --- ext/opcache/jit/zend_jit_arm64.dasc | 14 +++++++------- ext/opcache/jit/zend_jit_x86.dasc | 14 +++++++------- 2 files changed, 14 insertions(+), 14 deletions(-) diff --git a/ext/opcache/jit/zend_jit_arm64.dasc b/ext/opcache/jit/zend_jit_arm64.dasc index 1e630acbb0c46..91f8da5533888 100644 --- a/ext/opcache/jit/zend_jit_arm64.dasc +++ b/ext/opcache/jit/zend_jit_arm64.dasc @@ -12245,11 +12245,13 @@ static int zend_jit_fetch_obj(dasm_State **Dst, | ldr REG0w, [FCARG1x, #offsetof(zval, u2.extra)] | TST_32_WITH_CONST REG0w, IS_PROP_REINITABLE, TMP1w | beq >6 - | SET_ZVAL_PTR res_addr, FCARG1x, TMP1 - | SET_ZVAL_TYPE_INFO res_addr, IS_INDIRECT, TMP1w, TMP2 | and REG0w, REG0w, #(~IS_PROP_REINITABLE) | str REG0w, [FCARG1x, #offsetof(zval, u2.extra)] - | b >9 + if (flags) { + | b >3 + } else { + | b >4 + } |6: | mov FCARG1x, FCARG2x | SET_EX_OPLINE opline, REG0 @@ -12307,12 +12309,9 @@ static int zend_jit_fetch_obj(dasm_State **Dst, | ldr REG0w, [FCARG1x, #(prop_info->offset + offsetof(zval, u2.extra))] | TST_32_WITH_CONST REG0w, IS_PROP_REINITABLE, TMP1w | beq >6 - | LOAD_ZVAL_ADDR FCARG1x, prop_addr - | SET_ZVAL_PTR res_addr, FCARG1x, TMP1 - | SET_ZVAL_TYPE_INFO res_addr, IS_INDIRECT, TMP1w, TMP2 | and REG0w, REG0w, #(~IS_PROP_REINITABLE) | str REG0w, [FCARG1x, #(prop_info->offset + offsetof(zval, u2.extra))] - | b >9 + | b >4 |6: | LOAD_ADDR FCARG1x, prop_info | SET_EX_OPLINE opline, REG0 @@ -12320,6 +12319,7 @@ static int zend_jit_fetch_obj(dasm_State **Dst, | SET_ZVAL_TYPE_INFO res_addr, _IS_ERROR, TMP1w, TMP2 | b >9 |.code + |4: } if (opline->opcode == ZEND_FETCH_OBJ_W && (opline->extended_value & ZEND_FETCH_OBJ_FLAGS) diff --git a/ext/opcache/jit/zend_jit_x86.dasc b/ext/opcache/jit/zend_jit_x86.dasc index 583dd94d661bf..27ed084fd9cc9 100644 --- a/ext/opcache/jit/zend_jit_x86.dasc +++ b/ext/opcache/jit/zend_jit_x86.dasc @@ -12986,11 +12986,13 @@ static int zend_jit_fetch_obj(dasm_State **Dst, | mov eax, dword [FCARG1a + offsetof(zval, u2.extra)] | test eax, IS_PROP_REINITABLE | jz >6 - | SET_ZVAL_PTR res_addr, FCARG1a - | SET_ZVAL_TYPE_INFO res_addr, IS_INDIRECT | and eax, ~IS_PROP_REINITABLE | mov dword [FCARG1a + offsetof(zval, u2.extra)], eax - | jmp >9 + if (flags) { + | jmp >3 + } else { + | jmp >4 + } |6: | mov FCARG1a, FCARG2a | SET_EX_OPLINE opline, r0 @@ -13056,12 +13058,9 @@ static int zend_jit_fetch_obj(dasm_State **Dst, | mov eax, dword [FCARG1a + prop_info->offset + offsetof(zval, u2.extra)] | test eax, IS_PROP_REINITABLE | jz >6 - | LOAD_ZVAL_ADDR r0, prop_addr - | SET_ZVAL_PTR res_addr, r0 - | SET_ZVAL_TYPE_INFO res_addr, IS_INDIRECT | and eax, ~IS_PROP_REINITABLE | mov dword [FCARG1a + prop_info->offset + offsetof(zval, u2.extra)], eax - | jmp >9 + | jmp >4 |6: | LOAD_ADDR FCARG1a, prop_info | SET_EX_OPLINE opline, r0 @@ -13069,6 +13068,7 @@ static int zend_jit_fetch_obj(dasm_State **Dst, | SET_ZVAL_TYPE_INFO res_addr, _IS_ERROR | jmp >9 |.code + |4: } if (opline->opcode == ZEND_FETCH_OBJ_W && (opline->extended_value & ZEND_FETCH_OBJ_FLAGS)