From f0441744dbb6fabadda44913d242c7e31b4188fa Mon Sep 17 00:00:00 2001 From: Ilija Tovilo Date: Tue, 17 Dec 2024 20:02:31 +0100 Subject: [PATCH 1/4] Fix incorrect dynamic prop offset in hooked prop iterator Fixes GH-17200 Closes GH-17203 --- NEWS | 2 ++ Zend/tests/property_hooks/gh17200.phpt | 26 ++++++++++++++++++++++++++ Zend/zend_property_hooks.c | 25 ++++++++++++++++++++----- 3 files changed, 48 insertions(+), 5 deletions(-) create mode 100644 Zend/tests/property_hooks/gh17200.phpt diff --git a/NEWS b/NEWS index 99c5d6ecf3aea..bb783c7018c19 100644 --- a/NEWS +++ b/NEWS @@ -19,6 +19,8 @@ PHP NEWS (nielsdos) . Fixed bug GH-17101 (AST->string does not reproduce constructor property promotion correctly). (nielsdos) + . Fixed bug GH-17200 (Incorrect dynamic prop offset in hooked prop iterator). + (ilutov) - DBA: . Skip test if inifile is disabled. (orlitzky) diff --git a/Zend/tests/property_hooks/gh17200.phpt b/Zend/tests/property_hooks/gh17200.phpt new file mode 100644 index 0000000000000..eafbe7e67853b --- /dev/null +++ b/Zend/tests/property_hooks/gh17200.phpt @@ -0,0 +1,26 @@ +--TEST-- +GH-17200: Incorrect dynamic property offset +--FILE-- + $this->prop; + } +} + +$b = new B(); +var_dump($b); +echo json_encode($b), "\n"; + +?> +--EXPECTF-- +object(B)#%d (1) { + ["prop"]=> + NULL +} +{"prop":null} diff --git a/Zend/zend_property_hooks.c b/Zend/zend_property_hooks.c index d1db597d3a32d..01a8afb169373 100644 --- a/Zend/zend_property_hooks.c +++ b/Zend/zend_property_hooks.c @@ -28,6 +28,7 @@ typedef struct { bool declared_props_done; zval declared_props; bool dynamic_props_done; + uint32_t dynamic_prop_offset; uint32_t dynamic_prop_it; zval current_key; zval current_data; @@ -36,9 +37,19 @@ typedef struct { static zend_result zho_it_valid(zend_object_iterator *iter); static void zho_it_move_forward(zend_object_iterator *iter); -static uint32_t zho_num_backed_props(zend_object *zobj) +static uint32_t zho_find_dynamic_prop_offset(zend_array *properties) { - return zobj->ce->default_properties_count; + uint32_t offset = 0; + zval *value; + + ZEND_HASH_MAP_FOREACH_VAL(properties, value) { + if (Z_TYPE_P(value) != IS_INDIRECT) { + break; + } + offset++; + } ZEND_HASH_FOREACH_END(); + + return offset; } static zend_array *zho_build_properties_ex(zend_object *zobj, bool check_access, bool force_ptr, bool include_dynamic_props) @@ -106,7 +117,10 @@ static zend_array *zho_build_properties_ex(zend_object *zobj, bool check_access, if (include_dynamic_props && zobj->properties) { zend_string *prop_name; zval *prop_value; - ZEND_HASH_FOREACH_STR_KEY_VAL_FROM(zobj->properties, prop_name, prop_value, zho_num_backed_props(zobj)) { + ZEND_HASH_FOREACH_STR_KEY_VAL(zobj->properties, prop_name, prop_value) { + if (Z_TYPE_P(prop_value) == IS_INDIRECT) { + continue; + } zval *tmp = _zend_hash_append(properties, prop_name, prop_value); Z_TRY_ADDREF_P(tmp); } ZEND_HASH_FOREACH_END(); @@ -132,7 +146,8 @@ static void zho_dynamic_it_init(zend_hooked_object_iterator *hooked_iter) zend_object *zobj = Z_OBJ_P(&hooked_iter->it.data); zend_array *properties = zobj->handlers->get_properties(zobj); hooked_iter->dynamic_props_done = false; - hooked_iter->dynamic_prop_it = zend_hash_iterator_add(properties, zho_num_backed_props(zobj)); + hooked_iter->dynamic_prop_offset = zho_find_dynamic_prop_offset(properties); + hooked_iter->dynamic_prop_it = zend_hash_iterator_add(properties, hooked_iter->dynamic_prop_offset); } static void zho_it_get_current_key(zend_object_iterator *iter, zval *key); @@ -324,7 +339,7 @@ static void zho_it_rewind(zend_object_iterator *iter) zend_hash_internal_pointer_reset(properties); hooked_iter->declared_props_done = !zend_hash_num_elements(properties); hooked_iter->dynamic_props_done = false; - EG(ht_iterators)[hooked_iter->dynamic_prop_it].pos = zho_num_backed_props(Z_OBJ(iter->data)); + EG(ht_iterators)[hooked_iter->dynamic_prop_it].pos = hooked_iter->dynamic_prop_offset; } static HashTable *zho_it_get_gc(zend_object_iterator *iter, zval **table, int *n) From e69317b501c97599312a22f4adf8d48e28cea9e7 Mon Sep 17 00:00:00 2001 From: Ilija Tovilo Date: Tue, 17 Dec 2024 15:17:52 +0100 Subject: [PATCH 2/4] Reduce gc stack usage for strings (and resources) Adding strings to the worklist is useless, because they never contribute to cycles. The assembly size on x86_64 does not change. This significantly improves performance in this synthetic benchmark by 33%. function test($a) {} $a = new stdClass(); $a->self = $a; $a->prop1 = str_repeat('a', 10); $a->prop2 = str_repeat('a', 10); $a->prop3 = str_repeat('a', 10); $a->prop4 = str_repeat('a', 10); $a->prop5 = str_repeat('a', 10); $a->prop6 = str_repeat('a', 10); $a->prop7 = str_repeat('a', 10); $a->prop8 = str_repeat('a', 10); $a->prop9 = str_repeat('a', 10); $a->prop10 = str_repeat('a', 10); for ($i = 0; $i < 10_000_000; $i++) { test($a); gc_collect_cycles(); } This requires adding IS_TYPE_COLLECTABLE to IS_REFERENCE_EX to ensure these values continue to be pushed onto the stack. Luckily, IS_TYPE_COLLECTABLE is currently only used in gc_check_possible_root(), where the checked value cannot be a reference. Note that this changes the output of gc_collect_cycles(). Non-cyclic, refcounted values no longer count towards the total reported values collected. Also, there is some obvious overlap with GH-17130. This change should be good nonetheless, especially if we can remove the GC_COLLECTABLE(Z_COUNTED_P(zv)) condition in PHP 9 and rely on Z_COLLECTABLE_P() exclusively, given we can assume an object doesn't become cyclic at runtime anymore. Closes GH-17194 --- UPGRADING | 2 ++ Zend/zend_gc.c | 80 +++++++++++++++++++++++------------------------ Zend/zend_types.h | 5 ++- 3 files changed, 46 insertions(+), 41 deletions(-) diff --git a/UPGRADING b/UPGRADING index c328936968c98..4a1032e4d9868 100644 --- a/UPGRADING +++ b/UPGRADING @@ -34,6 +34,8 @@ PHP 8.5 UPGRADE NOTES $object. If compared to a statically unknown value $object == $true, it would always return false. This behavior was consolidated to always follow the behavior of (bool) $object. + . The return value of gc_collect_cycles() no longer includes strings and + resources that were indirectly collected through cycles. - Intl: . The extension now requires at least ICU 57.1. diff --git a/Zend/zend_gc.c b/Zend/zend_gc.c index a966a106def33..69bbca242b4ff 100644 --- a/Zend/zend_gc.c +++ b/Zend/zend_gc.c @@ -818,7 +818,7 @@ static void gc_scan_black(zend_refcounted *ref, gc_stack *stack) zval *entry = (zval*) Z_PTR_P(zv); zval *weakmap = zv+1; ZEND_ASSERT(Z_REFCOUNTED_P(weakmap)); - if (Z_OPT_REFCOUNTED_P(entry)) { + if (Z_OPT_COLLECTABLE_P(entry)) { GC_UNSET_FROM_WEAKMAP_KEY(entry); if (GC_REF_CHECK_COLOR(Z_COUNTED_P(weakmap), GC_GREY)) { /* Weakmap was scanned in gc_mark_roots, we must @@ -855,7 +855,7 @@ static void gc_scan_black(zend_refcounted *ref, gc_stack *stack) ZEND_ASSERT(Z_TYPE_P(zv+1) == IS_PTR); zval *key = zv; zval *entry = (zval*) Z_PTR_P(zv+1); - if (Z_OPT_REFCOUNTED_P(entry)) { + if (Z_OPT_COLLECTABLE_P(entry)) { GC_UNSET_FROM_WEAKMAP(entry); if (GC_REF_CHECK_COLOR(Z_COUNTED_P(key), GC_GREY)) { /* Key was scanned in gc_mark_roots, we must @@ -893,7 +893,7 @@ static void gc_scan_black(zend_refcounted *ref, gc_stack *stack) if (!GC_REF_CHECK_COLOR(ht, GC_BLACK)) { GC_REF_SET_BLACK(ht); for (; n != 0; n--) { - if (Z_REFCOUNTED_P(zv)) { + if (Z_COLLECTABLE_P(zv)) { ref = Z_COUNTED_P(zv); GC_ADDREF(ref); if (!GC_REF_CHECK_COLOR(ref, GC_BLACK)) { @@ -909,14 +909,14 @@ static void gc_scan_black(zend_refcounted *ref, gc_stack *stack) handle_zvals: for (; n != 0; n--) { - if (Z_REFCOUNTED_P(zv)) { + if (Z_COLLECTABLE_P(zv)) { ref = Z_COUNTED_P(zv); GC_ADDREF(ref); if (!GC_REF_CHECK_COLOR(ref, GC_BLACK)) { GC_REF_SET_BLACK(ref); zv++; while (--n) { - if (Z_REFCOUNTED_P(zv)) { + if (Z_COLLECTABLE_P(zv)) { zend_refcounted *ref = Z_COUNTED_P(zv); GC_ADDREF(ref); if (!GC_REF_CHECK_COLOR(ref, GC_BLACK)) { @@ -948,7 +948,7 @@ static void gc_scan_black(zend_refcounted *ref, gc_stack *stack) if (Z_TYPE_P(zv) == IS_INDIRECT) { zv = Z_INDIRECT_P(zv); } - if (Z_REFCOUNTED_P(zv)) { + if (Z_COLLECTABLE_P(zv)) { ref = Z_COUNTED_P(zv); GC_ADDREF(ref); if (!GC_REF_CHECK_COLOR(ref, GC_BLACK)) { @@ -959,7 +959,7 @@ static void gc_scan_black(zend_refcounted *ref, gc_stack *stack) if (Z_TYPE_P(zv) == IS_INDIRECT) { zv = Z_INDIRECT_P(zv); } - if (Z_REFCOUNTED_P(zv)) { + if (Z_COLLECTABLE_P(zv)) { zend_refcounted *ref = Z_COUNTED_P(zv); GC_ADDREF(ref); if (!GC_REF_CHECK_COLOR(ref, GC_BLACK)) { @@ -975,7 +975,7 @@ static void gc_scan_black(zend_refcounted *ref, gc_stack *stack) p++; } } else if (GC_TYPE(ref) == IS_REFERENCE) { - if (Z_REFCOUNTED(((zend_reference*)ref)->val)) { + if (Z_COLLECTABLE(((zend_reference*)ref)->val)) { ref = Z_COUNTED(((zend_reference*)ref)->val); GC_ADDREF(ref); if (!GC_REF_CHECK_COLOR(ref, GC_BLACK)) { @@ -1019,7 +1019,7 @@ static void gc_mark_grey(zend_refcounted *ref, gc_stack *stack) zval *entry = (zval*) Z_PTR_P(zv); zval *weakmap = zv+1; ZEND_ASSERT(Z_REFCOUNTED_P(weakmap)); - if (Z_REFCOUNTED_P(entry)) { + if (Z_COLLECTABLE_P(entry)) { GC_SET_FROM_WEAKMAP_KEY(entry); ref = Z_COUNTED_P(entry); /* Only DELREF if the contribution from the weakmap has @@ -1043,7 +1043,7 @@ static void gc_mark_grey(zend_refcounted *ref, gc_stack *stack) for (; n != 0; n--) { ZEND_ASSERT(Z_TYPE_P(zv) == IS_PTR); zval *entry = (zval*) Z_PTR_P(zv); - if (Z_REFCOUNTED_P(entry)) { + if (Z_COLLECTABLE_P(entry)) { GC_SET_FROM_WEAKMAP(entry); ref = Z_COUNTED_P(entry); /* Only DELREF if the contribution from the weakmap key @@ -1069,7 +1069,7 @@ static void gc_mark_grey(zend_refcounted *ref, gc_stack *stack) if (!GC_REF_CHECK_COLOR(ht, GC_GREY)) { GC_REF_SET_COLOR(ht, GC_GREY); for (; n != 0; n--) { - if (Z_REFCOUNTED_P(zv)) { + if (Z_COLLECTABLE_P(zv)) { ref = Z_COUNTED_P(zv); GC_DELREF(ref); if (!GC_REF_CHECK_COLOR(ref, GC_GREY)) { @@ -1084,14 +1084,14 @@ static void gc_mark_grey(zend_refcounted *ref, gc_stack *stack) } handle_zvals: for (; n != 0; n--) { - if (Z_REFCOUNTED_P(zv)) { + if (Z_COLLECTABLE_P(zv)) { ref = Z_COUNTED_P(zv); GC_DELREF(ref); if (!GC_REF_CHECK_COLOR(ref, GC_GREY)) { GC_REF_SET_COLOR(ref, GC_GREY); zv++; while (--n) { - if (Z_REFCOUNTED_P(zv)) { + if (Z_COLLECTABLE_P(zv)) { zend_refcounted *ref = Z_COUNTED_P(zv); GC_DELREF(ref); if (!GC_REF_CHECK_COLOR(ref, GC_GREY)) { @@ -1123,7 +1123,7 @@ static void gc_mark_grey(zend_refcounted *ref, gc_stack *stack) if (Z_TYPE_P(zv) == IS_INDIRECT) { zv = Z_INDIRECT_P(zv); } - if (Z_REFCOUNTED_P(zv)) { + if (Z_COLLECTABLE_P(zv)) { ref = Z_COUNTED_P(zv); GC_DELREF(ref); if (!GC_REF_CHECK_COLOR(ref, GC_GREY)) { @@ -1134,7 +1134,7 @@ static void gc_mark_grey(zend_refcounted *ref, gc_stack *stack) if (Z_TYPE_P(zv) == IS_INDIRECT) { zv = Z_INDIRECT_P(zv); } - if (Z_REFCOUNTED_P(zv)) { + if (Z_COLLECTABLE_P(zv)) { zend_refcounted *ref = Z_COUNTED_P(zv); GC_DELREF(ref); if (!GC_REF_CHECK_COLOR(ref, GC_GREY)) { @@ -1150,7 +1150,7 @@ static void gc_mark_grey(zend_refcounted *ref, gc_stack *stack) p++; } } else if (GC_TYPE(ref) == IS_REFERENCE) { - if (Z_REFCOUNTED(((zend_reference*)ref)->val)) { + if (Z_COLLECTABLE(((zend_reference*)ref)->val)) { ref = Z_COUNTED(((zend_reference*)ref)->val); GC_DELREF(ref); if (!GC_REF_CHECK_COLOR(ref, GC_GREY)) { @@ -1263,7 +1263,7 @@ static void gc_scan(zend_refcounted *ref, gc_stack *stack) for (; n != 0; n--) { ZEND_ASSERT(Z_TYPE_P(zv) == IS_PTR); zval *entry = (zval*) Z_PTR_P(zv); - if (Z_OPT_REFCOUNTED_P(entry)) { + if (Z_OPT_COLLECTABLE_P(entry)) { ref = Z_COUNTED_P(entry); if (GC_REF_CHECK_COLOR(ref, GC_GREY)) { GC_REF_SET_COLOR(ref, GC_WHITE); @@ -1282,7 +1282,7 @@ static void gc_scan(zend_refcounted *ref, gc_stack *stack) GC_REF_SET_COLOR(ht, GC_WHITE); GC_STACK_PUSH((zend_refcounted *) ht); for (; n != 0; n--) { - if (Z_REFCOUNTED_P(zv)) { + if (Z_COLLECTABLE_P(zv)) { ref = Z_COUNTED_P(zv); if (GC_REF_CHECK_COLOR(ref, GC_GREY)) { GC_REF_SET_COLOR(ref, GC_WHITE); @@ -1297,13 +1297,13 @@ static void gc_scan(zend_refcounted *ref, gc_stack *stack) handle_zvals: for (; n != 0; n--) { - if (Z_REFCOUNTED_P(zv)) { + if (Z_COLLECTABLE_P(zv)) { ref = Z_COUNTED_P(zv); if (GC_REF_CHECK_COLOR(ref, GC_GREY)) { GC_REF_SET_COLOR(ref, GC_WHITE); zv++; while (--n) { - if (Z_REFCOUNTED_P(zv)) { + if (Z_COLLECTABLE_P(zv)) { zend_refcounted *ref = Z_COUNTED_P(zv); if (GC_REF_CHECK_COLOR(ref, GC_GREY)) { GC_REF_SET_COLOR(ref, GC_WHITE); @@ -1335,7 +1335,7 @@ static void gc_scan(zend_refcounted *ref, gc_stack *stack) if (Z_TYPE_P(zv) == IS_INDIRECT) { zv = Z_INDIRECT_P(zv); } - if (Z_REFCOUNTED_P(zv)) { + if (Z_COLLECTABLE_P(zv)) { ref = Z_COUNTED_P(zv); if (GC_REF_CHECK_COLOR(ref, GC_GREY)) { GC_REF_SET_COLOR(ref, GC_WHITE); @@ -1345,7 +1345,7 @@ static void gc_scan(zend_refcounted *ref, gc_stack *stack) if (Z_TYPE_P(zv) == IS_INDIRECT) { zv = Z_INDIRECT_P(zv); } - if (Z_REFCOUNTED_P(zv)) { + if (Z_COLLECTABLE_P(zv)) { zend_refcounted *ref = Z_COUNTED_P(zv); if (GC_REF_CHECK_COLOR(ref, GC_GREY)) { GC_REF_SET_COLOR(ref, GC_WHITE); @@ -1360,7 +1360,7 @@ static void gc_scan(zend_refcounted *ref, gc_stack *stack) p++; } } else if (GC_TYPE(ref) == IS_REFERENCE) { - if (Z_REFCOUNTED(((zend_reference*)ref)->val)) { + if (Z_COLLECTABLE(((zend_reference*)ref)->val)) { ref = Z_COUNTED(((zend_reference*)ref)->val); if (GC_REF_CHECK_COLOR(ref, GC_GREY)) { GC_REF_SET_COLOR(ref, GC_WHITE); @@ -1473,7 +1473,7 @@ static int gc_collect_white(zend_refcounted *ref, uint32_t *flags, gc_stack *sta for (; n != 0; n--) { ZEND_ASSERT(Z_TYPE_P(zv) == IS_PTR); zval *entry = (zval*) Z_PTR_P(zv); - if (Z_REFCOUNTED_P(entry) && GC_FROM_WEAKMAP_KEY(entry)) { + if (Z_COLLECTABLE_P(entry) && GC_FROM_WEAKMAP_KEY(entry)) { GC_UNSET_FROM_WEAKMAP_KEY(entry); GC_UNSET_FROM_WEAKMAP(entry); ref = Z_COUNTED_P(entry); @@ -1494,7 +1494,7 @@ static int gc_collect_white(zend_refcounted *ref, uint32_t *flags, gc_stack *sta for (; n != 0; n--) { ZEND_ASSERT(Z_TYPE_P(zv) == IS_PTR); zval *entry = (zval*) Z_PTR_P(zv); - if (Z_REFCOUNTED_P(entry) && GC_FROM_WEAKMAP(entry)) { + if (Z_COLLECTABLE_P(entry) && GC_FROM_WEAKMAP(entry)) { GC_UNSET_FROM_WEAKMAP_KEY(entry); GC_UNSET_FROM_WEAKMAP(entry); ref = Z_COUNTED_P(entry); @@ -1517,7 +1517,7 @@ static int gc_collect_white(zend_refcounted *ref, uint32_t *flags, gc_stack *sta if (GC_REF_CHECK_COLOR(ht, GC_WHITE)) { GC_REF_SET_BLACK(ht); for (; n != 0; n--) { - if (Z_REFCOUNTED_P(zv)) { + if (Z_COLLECTABLE_P(zv)) { ref = Z_COUNTED_P(zv); GC_ADDREF(ref); if (GC_REF_CHECK_COLOR(ref, GC_WHITE)) { @@ -1533,14 +1533,14 @@ static int gc_collect_white(zend_refcounted *ref, uint32_t *flags, gc_stack *sta handle_zvals: for (; n != 0; n--) { - if (Z_REFCOUNTED_P(zv)) { + if (Z_COLLECTABLE_P(zv)) { ref = Z_COUNTED_P(zv); GC_ADDREF(ref); if (GC_REF_CHECK_COLOR(ref, GC_WHITE)) { GC_REF_SET_BLACK(ref); zv++; while (--n) { - if (Z_REFCOUNTED_P(zv)) { + if (Z_COLLECTABLE_P(zv)) { zend_refcounted *ref = Z_COUNTED_P(zv); GC_ADDREF(ref); if (GC_REF_CHECK_COLOR(ref, GC_WHITE)) { @@ -1576,7 +1576,7 @@ static int gc_collect_white(zend_refcounted *ref, uint32_t *flags, gc_stack *sta if (Z_TYPE_P(zv) == IS_INDIRECT) { zv = Z_INDIRECT_P(zv); } - if (Z_REFCOUNTED_P(zv)) { + if (Z_COLLECTABLE_P(zv)) { ref = Z_COUNTED_P(zv); GC_ADDREF(ref); if (GC_REF_CHECK_COLOR(ref, GC_WHITE)) { @@ -1587,7 +1587,7 @@ static int gc_collect_white(zend_refcounted *ref, uint32_t *flags, gc_stack *sta if (Z_TYPE_P(zv) == IS_INDIRECT) { zv = Z_INDIRECT_P(zv); } - if (Z_REFCOUNTED_P(zv)) { + if (Z_COLLECTABLE_P(zv)) { zend_refcounted *ref = Z_COUNTED_P(zv); GC_ADDREF(ref); if (GC_REF_CHECK_COLOR(ref, GC_WHITE)) { @@ -1603,7 +1603,7 @@ static int gc_collect_white(zend_refcounted *ref, uint32_t *flags, gc_stack *sta p++; } } else if (GC_TYPE(ref) == IS_REFERENCE) { - if (Z_REFCOUNTED(((zend_reference*)ref)->val)) { + if (Z_COLLECTABLE(((zend_reference*)ref)->val)) { ref = Z_COUNTED(((zend_reference*)ref)->val); GC_ADDREF(ref); if (GC_REF_CHECK_COLOR(ref, GC_WHITE)) { @@ -1681,7 +1681,7 @@ static int gc_remove_nested_data_from_buffer(zend_refcounted *ref, gc_root_buffe GC_REMOVE_FROM_BUFFER(ref); count++; } else if (GC_TYPE(ref) == IS_REFERENCE) { - if (Z_REFCOUNTED(((zend_reference*)ref)->val)) { + if (Z_COLLECTABLE(((zend_reference*)ref)->val)) { ref = Z_COUNTED(((zend_reference*)ref)->val); goto tail_call; } @@ -1704,7 +1704,7 @@ static int gc_remove_nested_data_from_buffer(zend_refcounted *ref, gc_root_buffe for (; n != 0; n--) { ZEND_ASSERT(Z_TYPE_P(zv) == IS_PTR); zval *entry = (zval*) Z_PTR_P(zv); - if (Z_OPT_REFCOUNTED_P(entry)) { + if (Z_OPT_COLLECTABLE_P(entry)) { ref = Z_COUNTED_P(entry); GC_STACK_PUSH(ref); } @@ -1717,7 +1717,7 @@ static int gc_remove_nested_data_from_buffer(zend_refcounted *ref, gc_root_buffe zv = table; if (UNEXPECTED(ht)) { for (; n != 0; n--) { - if (Z_REFCOUNTED_P(zv)) { + if (Z_COLLECTABLE_P(zv)) { ref = Z_COUNTED_P(zv); GC_STACK_PUSH(ref); } @@ -1732,11 +1732,11 @@ static int gc_remove_nested_data_from_buffer(zend_refcounted *ref, gc_root_buffe handle_zvals: for (; n != 0; n--) { - if (Z_REFCOUNTED_P(zv)) { + if (Z_COLLECTABLE_P(zv)) { ref = Z_COUNTED_P(zv); zv++; while (--n) { - if (Z_REFCOUNTED_P(zv)) { + if (Z_COLLECTABLE_P(zv)) { zend_refcounted *ref = Z_COUNTED_P(zv); GC_STACK_PUSH(ref); } @@ -1763,7 +1763,7 @@ static int gc_remove_nested_data_from_buffer(zend_refcounted *ref, gc_root_buffe if (Z_TYPE_P(zv) == IS_INDIRECT) { zv = Z_INDIRECT_P(zv); } - if (Z_REFCOUNTED_P(zv)) { + if (Z_COLLECTABLE_P(zv)) { ref = Z_COUNTED_P(zv); p++; while (--n) { @@ -1771,7 +1771,7 @@ static int gc_remove_nested_data_from_buffer(zend_refcounted *ref, gc_root_buffe if (Z_TYPE_P(zv) == IS_INDIRECT) { zv = Z_INDIRECT_P(zv); } - if (Z_REFCOUNTED_P(zv)) { + if (Z_COLLECTABLE_P(zv)) { zend_refcounted *ref = Z_COUNTED_P(zv); GC_STACK_PUSH(ref); } @@ -2175,7 +2175,7 @@ static void zend_gc_check_root_tmpvars(void) { if (kind == ZEND_LIVE_TMPVAR || kind == ZEND_LIVE_LOOP) { uint32_t var_num = range->var & ~ZEND_LIVE_MASK; zval *var = ZEND_CALL_VAR(ex, var_num); - if (Z_REFCOUNTED_P(var)) { + if (Z_COLLECTABLE_P(var)) { gc_check_possible_root(Z_COUNTED_P(var)); } } @@ -2205,7 +2205,7 @@ static void zend_gc_remove_root_tmpvars(void) { if (kind == ZEND_LIVE_TMPVAR || kind == ZEND_LIVE_LOOP) { uint32_t var_num = range->var & ~ZEND_LIVE_MASK; zval *var = ZEND_CALL_VAR(ex, var_num); - if (Z_REFCOUNTED_P(var)) { + if (Z_COLLECTABLE_P(var)) { GC_REMOVE_FROM_BUFFER(Z_COUNTED_P(var)); } } diff --git a/Zend/zend_types.h b/Zend/zend_types.h index f7d234c92c622..f839cec3b3667 100644 --- a/Zend/zend_types.h +++ b/Zend/zend_types.h @@ -808,7 +808,7 @@ static zend_always_inline uint32_t zval_gc_info(uint32_t gc_type_info) { #define IS_ARRAY_EX (IS_ARRAY | (IS_TYPE_REFCOUNTED << Z_TYPE_FLAGS_SHIFT) | (IS_TYPE_COLLECTABLE << Z_TYPE_FLAGS_SHIFT)) #define IS_OBJECT_EX (IS_OBJECT | (IS_TYPE_REFCOUNTED << Z_TYPE_FLAGS_SHIFT) | (IS_TYPE_COLLECTABLE << Z_TYPE_FLAGS_SHIFT)) #define IS_RESOURCE_EX (IS_RESOURCE | (IS_TYPE_REFCOUNTED << Z_TYPE_FLAGS_SHIFT)) -#define IS_REFERENCE_EX (IS_REFERENCE | (IS_TYPE_REFCOUNTED << Z_TYPE_FLAGS_SHIFT)) +#define IS_REFERENCE_EX (IS_REFERENCE | (IS_TYPE_REFCOUNTED << Z_TYPE_FLAGS_SHIFT) | (IS_TYPE_COLLECTABLE << Z_TYPE_FLAGS_SHIFT)) #define IS_CONSTANT_AST_EX (IS_CONSTANT_AST | (IS_TYPE_REFCOUNTED << Z_TYPE_FLAGS_SHIFT)) @@ -943,6 +943,9 @@ static zend_always_inline uint32_t zval_gc_info(uint32_t gc_type_info) { #define Z_OPT_REFCOUNTED(zval) Z_TYPE_INFO_REFCOUNTED(Z_TYPE_INFO(zval)) #define Z_OPT_REFCOUNTED_P(zval_p) Z_OPT_REFCOUNTED(*(zval_p)) +#define Z_OPT_COLLECTABLE(zval) ((Z_TYPE_INFO(zval) & (IS_TYPE_COLLECTABLE << Z_TYPE_FLAGS_SHIFT)) != 0) +#define Z_OPT_COLLECTABLE_P(zval_p) Z_OPT_COLLECTABLE(*(zval_p)) + /* deprecated: (COPYABLE is the same as IS_ARRAY) */ #define Z_OPT_COPYABLE(zval) (Z_OPT_TYPE(zval) == IS_ARRAY) #define Z_OPT_COPYABLE_P(zval_p) Z_OPT_COPYABLE(*(zval_p)) From cbe9d67efc7dbb3b1691a99c807d78a420a88a1d Mon Sep 17 00:00:00 2001 From: Ilija Tovilo Date: Wed, 18 Dec 2024 15:15:11 +0100 Subject: [PATCH 3/4] Add tests for GH-17151 --- ext/opcache/tests/jit/gh17151_1.phpt | 22 +++++++++++++++++++++ ext/opcache/tests/jit/gh17151_2.phpt | 29 ++++++++++++++++++++++++++++ ext/opcache/tests/jit/gh17151_3.phpt | 25 ++++++++++++++++++++++++ 3 files changed, 76 insertions(+) create mode 100644 ext/opcache/tests/jit/gh17151_1.phpt create mode 100644 ext/opcache/tests/jit/gh17151_2.phpt create mode 100644 ext/opcache/tests/jit/gh17151_3.phpt diff --git a/ext/opcache/tests/jit/gh17151_1.phpt b/ext/opcache/tests/jit/gh17151_1.phpt new file mode 100644 index 0000000000000..57c9bd142b72f --- /dev/null +++ b/ext/opcache/tests/jit/gh17151_1.phpt @@ -0,0 +1,22 @@ +--TEST-- +GH-17151: ZEND_FETCH_OBJ_R may modify RC of op1 +--FILE-- +bar; + var_dump($x); +} + +test(); + +?> +--EXPECTF-- +object(C)#%d (0) { +} diff --git a/ext/opcache/tests/jit/gh17151_2.phpt b/ext/opcache/tests/jit/gh17151_2.phpt new file mode 100644 index 0000000000000..26e1acbab7d9e --- /dev/null +++ b/ext/opcache/tests/jit/gh17151_2.phpt @@ -0,0 +1,29 @@ +--TEST-- +GH-17151: ZEND_FETCH_OBJ_R may modify RC of op1 +--FILE-- +bar; +} + +test(); +echo "Done\n"; + +?> +--EXPECT-- +C::__destruct +Done diff --git a/ext/opcache/tests/jit/gh17151_3.phpt b/ext/opcache/tests/jit/gh17151_3.phpt new file mode 100644 index 0000000000000..5e42d357a68a7 --- /dev/null +++ b/ext/opcache/tests/jit/gh17151_3.phpt @@ -0,0 +1,25 @@ +--TEST-- +GH-17151: Method calls may modify RC of ZEND_INIT_METHOD_CALL op1 +--FILE-- +storeThis(); + $c = null; +} + +test(); + +?> +===DONE=== +--EXPECT-- +===DONE=== From 6666cc83c53c2e83a100890838b5f6246bf3eb25 Mon Sep 17 00:00:00 2001 From: Dmitry Stogov Date: Wed, 18 Dec 2024 15:38:49 +0100 Subject: [PATCH 4/4] Fix RC inference of op1 of FETCH_OBJ and INIT_METHOD_CALL Fixes GH-17151 Closes GH-17152 --- NEWS | 2 ++ Zend/Optimizer/zend_inference.c | 4 ++++ ext/opcache/jit/zend_jit_ir.c | 1 + 3 files changed, 7 insertions(+) diff --git a/NEWS b/NEWS index bb783c7018c19..83a3337c652c8 100644 --- a/NEWS +++ b/NEWS @@ -51,6 +51,8 @@ PHP NEWS . opcache_get_configuration() properly reports jit_prof_threshold. (cmb) . Fixed bug GH-17140 (Assertion failure in JIT trace exit with ZEND_FETCH_DIM_FUNC_ARG). (nielsdos, Dmitry) + . Fixed bug GH-17151 (Incorrect RC inference of op1 of FETCH_OBJ and + INIT_METHOD_CALL). (Dmitry, ilutov) - PCNTL: . Fix memory leak in cleanup code of pcntl_exec() when a non stringable diff --git a/Zend/Optimizer/zend_inference.c b/Zend/Optimizer/zend_inference.c index 7169238893159..fc6b9b421b628 100644 --- a/Zend/Optimizer/zend_inference.c +++ b/Zend/Optimizer/zend_inference.c @@ -1968,6 +1968,10 @@ static uint32_t get_ssa_alias_types(zend_ssa_alias_kind alias) { /* TODO: support for array keys and ($str . "")*/ \ __type |= MAY_BE_RCN; \ } \ + if ((__type & MAY_BE_RC1) && (__type & MAY_BE_OBJECT)) {\ + /* TODO: object may be captured by magic handlers */\ + __type |= MAY_BE_RCN; \ + } \ if (__ssa_var->alias) { \ __type |= get_ssa_alias_types(__ssa_var->alias); \ } \ diff --git a/ext/opcache/jit/zend_jit_ir.c b/ext/opcache/jit/zend_jit_ir.c index 843d3ae90d84c..5661fec934f7d 100644 --- a/ext/opcache/jit/zend_jit_ir.c +++ b/ext/opcache/jit/zend_jit_ir.c @@ -14426,6 +14426,7 @@ static int zend_jit_fetch_obj(zend_jit_ctx *jit, ir_MERGE_list(slow_inputs); jit_SET_EX_OPLINE(jit, opline); + op1_info |= MAY_BE_RC1 | MAY_BE_RCN; /* object may be captured/released in magic handler */ if (opline->opcode == ZEND_FETCH_OBJ_W) { ir_CALL_1(IR_VOID, ir_CONST_FC_FUNC(zend_jit_fetch_obj_w_slow), obj_ref); ir_END_list(end_inputs);