From 60f87f29bbabafd04a0af7dcc8d06616a2a0127f Mon Sep 17 00:00:00 2001 From: Ilija Tovilo Date: Mon, 19 Aug 2024 15:46:20 +0200 Subject: [PATCH] Fix various hooked object iterator issues (GH-15394) Fixes GH-15187 --- NEWS | 1 + Zend/tests/property_hooks/foreach.phpt | 5 + Zend/tests/property_hooks/gh15187_1.phpt | 23 ++++ Zend/tests/property_hooks/gh15187_2.phpt | 21 ++++ Zend/tests/property_hooks/gh15187_3.phpt | 43 ++++++++ Zend/zend_compile.h | 2 + Zend/zend_property_hooks.c | 134 ++++++++++++----------- 7 files changed, 165 insertions(+), 64 deletions(-) create mode 100644 Zend/tests/property_hooks/gh15187_1.phpt create mode 100644 Zend/tests/property_hooks/gh15187_2.phpt create mode 100644 Zend/tests/property_hooks/gh15187_3.phpt diff --git a/NEWS b/NEWS index 597673c7fe1..54018fcab1f 100644 --- a/NEWS +++ b/NEWS @@ -9,6 +9,7 @@ PHP NEWS visibility are ignored). (ilutov) . Fixed bug GH-15419 (Missing readonly+hook incompatibility check for readonly classes). (ilutov) + . Fixed bug GH-15187 (Various hooked object iterator issues). (ilutov) 15 Aug 2024, PHP 8.4.0beta3 diff --git a/Zend/tests/property_hooks/foreach.phpt b/Zend/tests/property_hooks/foreach.phpt index c217bf6ac0e..c6073b50ed9 100644 --- a/Zend/tests/property_hooks/foreach.phpt +++ b/Zend/tests/property_hooks/foreach.phpt @@ -17,6 +17,11 @@ class ByRef { $this->_virtualByRef = $value; } } + public $virtualSetOnly { + set { + echo __METHOD__, "\n"; + } + } public function __construct() { $this->dynamic = 'dynamic'; } diff --git a/Zend/tests/property_hooks/gh15187_1.phpt b/Zend/tests/property_hooks/gh15187_1.phpt new file mode 100644 index 00000000000..13b72962051 --- /dev/null +++ b/Zend/tests/property_hooks/gh15187_1.phpt @@ -0,0 +1,23 @@ +--TEST-- +GH-15187: Crash in hooked iterators on properties array without dynamic properties +--FILE-- + $value; } +} + +$test = new Test(); +var_dump($test); +foreach ($test as $key => $value) { + var_dump($key, $value); +} + +?> +--EXPECTF-- +object(Test)#%d (1) { + ["prop"]=> + NULL +} +string(4) "prop" +NULL diff --git a/Zend/tests/property_hooks/gh15187_2.phpt b/Zend/tests/property_hooks/gh15187_2.phpt new file mode 100644 index 00000000000..bf8a14fd8f9 --- /dev/null +++ b/Zend/tests/property_hooks/gh15187_2.phpt @@ -0,0 +1,21 @@ +--TEST-- +GH-15187: Crash in hooked iterators on properties array without dynamic properties +--FILE-- + $value; } +} + +$test = new Test(); +var_dump($test); +foreach ($test as $key => $value) { + var_dump($key, $value); +} + +?> +--EXPECTF-- +object(Test)#%d (0) { + ["prop"]=> + uninitialized(int) +} diff --git a/Zend/tests/property_hooks/gh15187_3.phpt b/Zend/tests/property_hooks/gh15187_3.phpt new file mode 100644 index 00000000000..d82e6002f82 --- /dev/null +++ b/Zend/tests/property_hooks/gh15187_3.phpt @@ -0,0 +1,43 @@ +--TEST-- +GH-15187: Hooked iterator typed property ref tracking +--FILE-- + &$v) { + var_dump($v); + if ($k === 'c') { + try { + $v = 'foo'; + } catch (Error $e) { + echo $e->getMessage(), "\n"; + } + } + if ($k === 'd') { + $v = 'foo'; + } +} + +var_dump($c); + +?> +--EXPECTF-- +int(1) +Cannot assign string to reference held by property C::$c of type int +int(2) +object(C)#%d (2) { + ["b"]=> + uninitialized(int) + ["c"]=> + int(1) + ["d"]=> + &string(3) "foo" +} diff --git a/Zend/zend_compile.h b/Zend/zend_compile.h index c7e31877b5c..69f3ef762ca 100644 --- a/Zend/zend_compile.h +++ b/Zend/zend_compile.h @@ -438,6 +438,8 @@ typedef struct _zend_property_info { ((uint32_t)(XtOffsetOf(zend_object, properties_table) + sizeof(zval) * (num))) #define OBJ_PROP_TO_NUM(offset) \ ((offset - OBJ_PROP_TO_OFFSET(0)) / sizeof(zval)) +#define OBJ_PROP_PTR_TO_NUM(obj, prop) \ + (((char*)prop - (char*)obj->properties_table) / sizeof(zval)) typedef struct _zend_class_constant { zval value; /* flags are stored in u2 */ diff --git a/Zend/zend_property_hooks.c b/Zend/zend_property_hooks.c index d3ae945f1d8..684caafc4dd 100644 --- a/Zend/zend_property_hooks.c +++ b/Zend/zend_property_hooks.c @@ -25,12 +25,14 @@ typedef struct { bool by_ref; bool declared_props_done; zval declared_props; + bool dynamic_props_done; uint32_t dynamic_prop_it; zval current_key; zval current_data; } zend_hooked_object_iterator; static zend_result zho_it_valid(zend_object_iterator *iter); +static void zho_it_move_forward(zend_object_iterator *iter); // FIXME: This should probably be stored on zend_class_entry somewhere (e.g. through num_virtual_props). static uint32_t zho_num_backed_props(zend_object *zobj) @@ -87,13 +89,16 @@ ZEND_API zend_array *zend_hooked_object_build_properties(zend_object *zobj) static bool zho_dynamic_it_init(zend_hooked_object_iterator *hooked_iter) { - zend_object *zobj = Z_OBJ_P(&hooked_iter->it.data); - if (!zobj->properties) { - return false; - } if (hooked_iter->dynamic_prop_it != (uint32_t) -1) { return true; } + + zend_object *zobj = Z_OBJ_P(&hooked_iter->it.data); + if (!zobj->properties || zho_num_backed_props(zobj) == zobj->properties->nNumUsed) { + hooked_iter->dynamic_props_done = true; + return false; + } + hooked_iter->dynamic_prop_it = zend_hash_iterator_add(zobj->properties, zho_num_backed_props(zobj)); return true; } @@ -102,26 +107,17 @@ static void zho_it_get_current_key(zend_object_iterator *iter, zval *key); static void zho_declared_it_fetch_current(zend_object_iterator *iter) { - ZEND_ASSERT(zho_it_valid(iter) == SUCCESS); - zend_hooked_object_iterator *hooked_iter = (zend_hooked_object_iterator*)iter; - zval_ptr_dtor(&hooked_iter->current_data); - if (EG(exception)) { - return; - } - ZVAL_UNDEF(&hooked_iter->current_data); - zval_ptr_dtor(&hooked_iter->current_key); - ZVAL_UNDEF(&hooked_iter->current_key); zend_object *zobj = Z_OBJ_P(&iter->data); zend_array *properties = Z_ARR(hooked_iter->declared_props); - zval_ptr_dtor_nogc(&hooked_iter->current_key); - zend_hash_get_current_key_zval(properties, &hooked_iter->current_key); - zval *property = zend_hash_get_current_data(properties); if (Z_TYPE_P(property) == IS_PTR) { zend_property_info *prop_info = Z_PTR_P(property); zend_function *get = prop_info->hooks[ZEND_PROPERTY_HOOK_GET]; + if (!get && (prop_info->flags & ZEND_ACC_VIRTUAL)) { + return; + } if (hooked_iter->by_ref && (get == NULL || !(get->common.fn_flags & ZEND_ACC_RETURN_REFERENCE))) { @@ -129,12 +125,27 @@ static void zho_declared_it_fetch_current(zend_object_iterator *iter) ZSTR_VAL(zobj->ce->name), zend_get_unmangled_property_name(prop_info->name)); return; } - zend_read_property_ex(prop_info->ce, zobj, prop_info->name, /* silent */ true, &hooked_iter->current_data); + zval *value = zend_read_property_ex(prop_info->ce, zobj, prop_info->name, /* silent */ true, &hooked_iter->current_data); + if (value == &EG(uninitialized_zval)) { + return; + } else if (value != &hooked_iter->current_data) { + ZVAL_COPY(&hooked_iter->current_data, value); + } } else { ZVAL_DEINDIRECT(property); - if (hooked_iter->by_ref && Z_TYPE_P(property) != IS_REFERENCE) { - ZEND_ASSERT(Z_TYPE_P(property) != IS_UNDEF); + if (Z_TYPE_P(property) == IS_UNDEF) { + return; + } + if (!hooked_iter->by_ref) { + ZVAL_DEREF(property); + } else if (Z_TYPE_P(property) != IS_REFERENCE) { ZVAL_MAKE_REF(property); + + zend_class_entry *ce = zobj->ce; + zend_property_info *prop_info = ce->properties_info_table[OBJ_PROP_PTR_TO_NUM(zobj, property)]; + if (ZEND_TYPE_IS_SET(prop_info->type)) { + ZEND_REF_ADD_TYPE_SOURCE(Z_REF_P(property), prop_info); + } } ZVAL_COPY(&hooked_iter->current_data, property); } @@ -154,10 +165,8 @@ static void zho_dynamic_it_fetch_current(zend_object_iterator *iter) ZEND_ASSERT(Z_TYPE(bucket->val) != IS_UNDEF); ZVAL_MAKE_REF(&bucket->val); } - zval_ptr_dtor(&hooked_iter->current_data); ZVAL_COPY(&hooked_iter->current_data, &bucket->val); - zval_ptr_dtor_nogc(&hooked_iter->current_key); if (bucket->key) { ZVAL_STR_COPY(&hooked_iter->current_key, bucket->key); } else { @@ -168,13 +177,22 @@ static void zho_dynamic_it_fetch_current(zend_object_iterator *iter) static void zho_it_fetch_current(zend_object_iterator *iter) { zend_hooked_object_iterator *hooked_iter = (zend_hooked_object_iterator*)iter; + if (Z_TYPE(hooked_iter->current_data) != IS_UNDEF) { + return; + } - if (!hooked_iter->declared_props_done) { - zho_declared_it_fetch_current(iter); - } else if (zho_dynamic_it_init(hooked_iter)) { - zho_dynamic_it_fetch_current(iter); - } else { - ZEND_UNREACHABLE(); + while (true) { + if (!hooked_iter->declared_props_done) { + zho_declared_it_fetch_current(iter); + } else if (!hooked_iter->dynamic_props_done && zho_dynamic_it_init(hooked_iter)) { + zho_dynamic_it_fetch_current(iter); + } else { + break; + } + if (Z_TYPE(hooked_iter->current_data) != IS_UNDEF || EG(exception)) { + break; + } + zho_it_move_forward(iter); } } @@ -185,7 +203,6 @@ static void zho_it_dtor(zend_object_iterator *iter) zval_ptr_dtor(&hooked_iter->declared_props); zval_ptr_dtor_nogc(&hooked_iter->current_key); zval_ptr_dtor(&hooked_iter->current_data); - zval_ptr_dtor(&hooked_iter->current_key); if (hooked_iter->dynamic_prop_it != (uint32_t) -1) { zend_hash_iterator_del(hooked_iter->dynamic_prop_it); } @@ -194,78 +211,66 @@ static void zho_it_dtor(zend_object_iterator *iter) static zend_result zho_it_valid(zend_object_iterator *iter) { zend_hooked_object_iterator *hooked_iter = (zend_hooked_object_iterator*)iter; - if (!hooked_iter->declared_props_done) { - if (zend_hash_has_more_elements(Z_ARR(hooked_iter->declared_props)) == SUCCESS) { - return SUCCESS; - } - } - - if (zho_dynamic_it_init(hooked_iter)) { - zend_object *zobj = Z_OBJ_P(&hooked_iter->it.data); - HashPosition pos = zend_hash_iterator_pos(hooked_iter->dynamic_prop_it, zobj->properties); - return pos < zobj->properties->nNumUsed ? SUCCESS : FAILURE; - } - - return FAILURE; + zho_it_fetch_current(iter); + return Z_TYPE(hooked_iter->current_data) != IS_UNDEF ? SUCCESS : FAILURE; } static zval *zho_it_get_current_data(zend_object_iterator *iter) { zend_hooked_object_iterator *hooked_iter = (zend_hooked_object_iterator*)iter; + zho_it_fetch_current(iter); return &hooked_iter->current_data; } static void zho_it_get_current_key(zend_object_iterator *iter, zval *key) { zend_hooked_object_iterator *hooked_iter = (zend_hooked_object_iterator*)iter; + zho_it_fetch_current(iter); ZVAL_COPY(key, &hooked_iter->current_key); } static void zho_it_move_forward(zend_object_iterator *iter) { zend_hooked_object_iterator *hooked_iter = (zend_hooked_object_iterator*)iter; - zend_array *properties = Z_ARR(hooked_iter->declared_props); + + zval_ptr_dtor(&hooked_iter->current_data); + ZVAL_UNDEF(&hooked_iter->current_data); + zval_ptr_dtor_nogc(&hooked_iter->current_key); + ZVAL_UNDEF(&hooked_iter->current_key); + if (!hooked_iter->declared_props_done) { + zend_array *properties = Z_ARR(hooked_iter->declared_props); zend_hash_move_forward(properties); - if (zend_hash_has_more_elements(properties) == SUCCESS) { - zho_declared_it_fetch_current(iter); - } else { + if (zend_hash_has_more_elements(properties) != SUCCESS) { hooked_iter->declared_props_done = true; - if (zho_dynamic_it_init(hooked_iter)) { - zho_dynamic_it_fetch_current(iter); - } } - } else if (zho_dynamic_it_init(hooked_iter)) { + } else if (!hooked_iter->dynamic_props_done && zho_dynamic_it_init(hooked_iter)) { zend_array *properties = Z_OBJ(iter->data)->properties; HashPosition pos = zend_hash_iterator_pos(hooked_iter->dynamic_prop_it, properties); - Bucket *bucket = properties->arData + pos; - while (true) { - pos++; - bucket++; - if (pos >= properties->nNumUsed) { - break; - } - if (Z_TYPE_INFO_P(&bucket->val) != IS_UNDEF) { - zho_dynamic_it_fetch_current(iter); - break; - } - } + pos++; EG(ht_iterators)[hooked_iter->dynamic_prop_it].pos = pos; + if (pos >= properties->nNumUsed) { + hooked_iter->dynamic_props_done = true; + } } } static void zho_it_rewind(zend_object_iterator *iter) { zend_hooked_object_iterator *hooked_iter = (zend_hooked_object_iterator*)iter; + + zval_ptr_dtor(&hooked_iter->current_data); + ZVAL_UNDEF(&hooked_iter->current_data); + zval_ptr_dtor_nogc(&hooked_iter->current_key); + ZVAL_UNDEF(&hooked_iter->current_key); + hooked_iter->declared_props_done = false; zend_array *properties = Z_ARR(hooked_iter->declared_props); zend_hash_internal_pointer_reset(properties); + hooked_iter->dynamic_props_done = false; if (hooked_iter->dynamic_prop_it != (uint32_t) -1) { EG(ht_iterators)[hooked_iter->dynamic_prop_it].pos = zho_num_backed_props(Z_OBJ(iter->data)); } - if (zho_it_valid(iter) == SUCCESS) { - zho_it_fetch_current(iter); - } } static HashTable *zho_it_get_gc(zend_object_iterator *iter, zval **table, int *n) @@ -301,6 +306,7 @@ ZEND_API zend_object_iterator *zend_hooked_object_get_iterator(zend_class_entry iterator->declared_props_done = false; zend_array *properties = zho_build_properties_ex(Z_OBJ_P(object), true, false); ZVAL_ARR(&iterator->declared_props, properties); + iterator->dynamic_props_done = false; iterator->dynamic_prop_it = (uint32_t) -1; ZVAL_UNDEF(&iterator->current_key); ZVAL_UNDEF(&iterator->current_data);