From 9b6ab08657aeef4a2c66b308011d9a898b606992 Mon Sep 17 00:00:00 2001 From: Niels Dossche <7771979+nielsdos@users.noreply.github.com> Date: Thu, 6 Apr 2023 19:38:02 +0200 Subject: [PATCH] Fix number of elements after packed hash filling After a hash filling routine the number of elements are set to the fill index. However, if the fill index is larger than the number of elements, the number of elements are no longer correct. This is observable at least via count() and var_dump(). E.g. the attached test case would incorrectly show int(17) instead of int(11). Solve this by only increasing the number of elements by the actual number that got added. Instead of adding a variable that increments per iteration, I wanted to save some cycles in the iteration and simply compute the number of added elements at the end. I discovered this behaviour while fixing GH-11016, where this filling routine is easily exposed to userland via a specialised VM path [1]. Since this seems to be more a general problem with the macros, and may be triggered outside of the VM handlers, I fixed it in the macros instead of modifying the VM to fixup the number of elements. [1] https://github.com/php/php-src/blob/b2c5acbb010f4bbc7ea9b53ba9bc81d672dd0f34/Zend/zend_vm_def.h#L6132-L6141 --- Zend/zend_hash.h | 4 +- ext/zend_test/test.c | 23 ++++++++++ ext/zend_test/test.stub.php | 2 + ext/zend_test/test_arginfo.h | 8 +++- .../tests/hash_fill_packed_nr_elements.phpt | 44 +++++++++++++++++++ 5 files changed, 78 insertions(+), 3 deletions(-) create mode 100644 ext/zend_test/tests/hash_fill_packed_nr_elements.phpt diff --git a/Zend/zend_hash.h b/Zend/zend_hash.h index d9210d073182f..5726c8a919f43 100644 --- a/Zend/zend_hash.h +++ b/Zend/zend_hash.h @@ -1517,8 +1517,8 @@ static zend_always_inline void *zend_hash_get_current_data_ptr_ex(HashTable *ht, #define ZEND_HASH_FILL_GROW() do { \ if (UNEXPECTED(__fill_idx >= __fill_ht->nTableSize)) { \ + __fill_ht->nNumOfElements += __fill_idx - __fill_ht->nNumUsed; \ __fill_ht->nNumUsed = __fill_idx; \ - __fill_ht->nNumOfElements = __fill_idx; \ __fill_ht->nNextFreeElement = __fill_idx; \ zend_hash_packed_grow(__fill_ht); \ __fill_val = __fill_ht->arPacked + __fill_idx; \ @@ -1557,8 +1557,8 @@ static zend_always_inline void *zend_hash_get_current_data_ptr_ex(HashTable *ht, } while (0) #define ZEND_HASH_FILL_FINISH() do { \ + __fill_ht->nNumOfElements += __fill_idx - __fill_ht->nNumUsed; \ __fill_ht->nNumUsed = __fill_idx; \ - __fill_ht->nNumOfElements = __fill_idx; \ __fill_ht->nNextFreeElement = __fill_idx; \ __fill_ht->nInternalPointer = 0; \ } while (0) diff --git a/ext/zend_test/test.c b/ext/zend_test/test.c index 123984a5acfd4..0bf458b7be7d7 100644 --- a/ext/zend_test/test.c +++ b/ext/zend_test/test.c @@ -533,6 +533,29 @@ static ZEND_FUNCTION(zend_get_map_ptr_last) RETURN_LONG(CG(map_ptr_last)); } +static ZEND_FUNCTION(zend_test_fill_packed_array) +{ + HashTable *parameter; + + ZEND_PARSE_PARAMETERS_START(1, 1) + Z_PARAM_ARRAY_HT_EX(parameter, 0, 1) + ZEND_PARSE_PARAMETERS_END(); + + if (!HT_IS_PACKED(parameter)) { + zend_argument_value_error(1, "must be a packed array"); + RETURN_THROWS(); + } + + zend_hash_extend(parameter, parameter->nNumUsed + 10, true); + ZEND_HASH_FILL_PACKED(parameter) { + for (int i = 0; i < 10; i++) { + zval value; + ZVAL_LONG(&value, i); + ZEND_HASH_FILL_ADD(&value); + } + } ZEND_HASH_FILL_END(); +} + static zend_object *zend_test_class_new(zend_class_entry *class_type) { zend_object *obj = zend_objects_new(class_type); diff --git a/ext/zend_test/test.stub.php b/ext/zend_test/test.stub.php index 31bb04b7deedf..27f5c01d6fd66 100644 --- a/ext/zend_test/test.stub.php +++ b/ext/zend_test/test.stub.php @@ -192,6 +192,8 @@ function zend_test_zend_call_stack_use_all(): int {} function zend_test_is_string_marked_as_valid_utf8(string $string): bool {} function zend_get_map_ptr_last(): int {} + + function zend_test_fill_packed_array(array &$array): void {} } namespace ZendTestNS { diff --git a/ext/zend_test/test_arginfo.h b/ext/zend_test/test_arginfo.h index dca3a62f74ef4..c4ffe80c631d9 100644 --- a/ext/zend_test/test_arginfo.h +++ b/ext/zend_test/test_arginfo.h @@ -1,5 +1,5 @@ /* This is a generated file, edit the .stub.php file instead. - * Stub hash: 4c35a5a9f3910247c8297c21185ea3969312f518 */ + * Stub hash: ebb806c4a8442233be572a304295f47570f12102 */ ZEND_BEGIN_ARG_WITH_RETURN_TYPE_INFO_EX(arginfo_zend_test_array_return, 0, 0, IS_ARRAY, 0) ZEND_END_ARG_INFO() @@ -114,6 +114,10 @@ ZEND_END_ARG_INFO() ZEND_BEGIN_ARG_WITH_RETURN_TYPE_INFO_EX(arginfo_zend_get_map_ptr_last, 0, 0, IS_LONG, 0) ZEND_END_ARG_INFO() +ZEND_BEGIN_ARG_WITH_RETURN_TYPE_INFO_EX(arginfo_zend_test_fill_packed_array, 0, 1, IS_VOID, 0) + ZEND_ARG_TYPE_INFO(1, array, IS_ARRAY, 0) +ZEND_END_ARG_INFO() + ZEND_BEGIN_ARG_WITH_RETURN_TYPE_INFO_EX(arginfo_ZendTestNS2_namespaced_func, 0, 0, _IS_BOOL, 0) ZEND_END_ARG_INFO() @@ -217,6 +221,7 @@ static ZEND_FUNCTION(zend_test_zend_call_stack_use_all); #endif static ZEND_FUNCTION(zend_test_is_string_marked_as_valid_utf8); static ZEND_FUNCTION(zend_get_map_ptr_last); +static ZEND_FUNCTION(zend_test_fill_packed_array); static ZEND_FUNCTION(ZendTestNS2_namespaced_func); static ZEND_FUNCTION(ZendTestNS2_namespaced_deprecated_func); static ZEND_FUNCTION(ZendTestNS2_ZendSubNS_namespaced_func); @@ -277,6 +282,7 @@ static const zend_function_entry ext_functions[] = { #endif ZEND_FE(zend_test_is_string_marked_as_valid_utf8, arginfo_zend_test_is_string_marked_as_valid_utf8) ZEND_FE(zend_get_map_ptr_last, arginfo_zend_get_map_ptr_last) + ZEND_FE(zend_test_fill_packed_array, arginfo_zend_test_fill_packed_array) ZEND_NS_FALIAS("ZendTestNS2", namespaced_func, ZendTestNS2_namespaced_func, arginfo_ZendTestNS2_namespaced_func) ZEND_NS_DEP_FALIAS("ZendTestNS2", namespaced_deprecated_func, ZendTestNS2_namespaced_deprecated_func, arginfo_ZendTestNS2_namespaced_deprecated_func) ZEND_NS_FALIAS("ZendTestNS2", namespaced_aliased_func, zend_test_void_return, arginfo_ZendTestNS2_namespaced_aliased_func) diff --git a/ext/zend_test/tests/hash_fill_packed_nr_elements.phpt b/ext/zend_test/tests/hash_fill_packed_nr_elements.phpt new file mode 100644 index 0000000000000..77f22b2d9cd15 --- /dev/null +++ b/ext/zend_test/tests/hash_fill_packed_nr_elements.phpt @@ -0,0 +1,44 @@ +--TEST-- +Test hash packed fill number of elements +--EXTENSIONS-- +zend_test +--FILE-- + 0]; +zend_test_fill_packed_array($my_array); + +var_dump($my_array); +var_dump(count($my_array)); + +?> +--EXPECT-- +array(11) { + [6]=> + int(0) + [7]=> + int(0) + [8]=> + int(1) + [9]=> + int(2) + [10]=> + int(3) + [11]=> + int(4) + [12]=> + int(5) + [13]=> + int(6) + [14]=> + int(7) + [15]=> + int(8) + [16]=> + int(9) +} +int(11)