From 647bb27008d63ea383c1316be9ead9a365bb025b Mon Sep 17 00:00:00 2001 From: Tyson Andre Date: Mon, 10 Oct 2022 14:12:41 -0400 Subject: [PATCH 1/2] Avoid crash for reset/end/next/prev() on ffi classes (And any PECLs returning `zend_empty_array` in the handler->get_properties overrides) Closes GH-9697 This is similar to the fix used in d9651a941915eb5fb5ad557090b65256fd8509b6 for array_walk. This should make it safer for php-src (and PECLs, long-term) to return the empty immutable array in `handler->get_properties` to avoid wasting memory. See https://github.com/php/php-src/issues/9697#issuecomment-1273613175 The only possible internal iterator position for the empty array is at the end of the empty array (nInternalPointer=0). The `zend_hash*del*` helpers will always set nInternalPointer to 0 when an array becomes empty, regardless of previous insertions/deletions/updates to the array. --- ext/ffi/tests/arrayPointer.phpt | 20 ++++++++++++++++++++ ext/standard/array.c | 16 ++++++++++++++++ 2 files changed, 36 insertions(+) create mode 100644 ext/ffi/tests/arrayPointer.phpt diff --git a/ext/ffi/tests/arrayPointer.phpt b/ext/ffi/tests/arrayPointer.phpt new file mode 100644 index 0000000000000..3a2b06563ade5 --- /dev/null +++ b/ext/ffi/tests/arrayPointer.phpt @@ -0,0 +1,20 @@ +--TEST-- +FFI: Test deprecated use of array helper functions on FFI classes doesn't crash +--EXTENSIONS-- +ffi +--INI-- +ffi.enable=1 +--FILE-- + +--EXPECTF-- +bool(false) +bool(false) +bool(false) +bool(false) diff --git a/ext/standard/array.c b/ext/standard/array.c index 3b807c739a874..9a814ea07da47 100644 --- a/ext/standard/array.c +++ b/ext/standard/array.c @@ -1073,6 +1073,10 @@ PHP_FUNCTION(end) ZEND_PARSE_PARAMETERS_END(); HashTable *array = get_ht_for_iap(array_zv, /* separate */ true); + if (zend_hash_num_elements(array) == 0) { + /* array->nInternalPointer is already 0 if the array is empty, even after removing elements */ + RETURN_FALSE; + } zend_hash_internal_pointer_end(array); if (USED_RET()) { @@ -1100,6 +1104,10 @@ PHP_FUNCTION(prev) ZEND_PARSE_PARAMETERS_END(); HashTable *array = get_ht_for_iap(array_zv, /* separate */ true); + if (zend_hash_num_elements(array) == 0) { + /* array->nInternalPointer is already 0 if the array is empty, even after removing elements */ + RETURN_FALSE; + } zend_hash_move_backwards(array); if (USED_RET()) { @@ -1127,6 +1135,10 @@ PHP_FUNCTION(next) ZEND_PARSE_PARAMETERS_END(); HashTable *array = get_ht_for_iap(array_zv, /* separate */ true); + if (zend_hash_num_elements(array) == 0) { + /* array->nInternalPointer is already 0 if the array is empty, even after removing elements */ + RETURN_FALSE; + } zend_hash_move_forward(array); if (USED_RET()) { @@ -1154,6 +1166,10 @@ PHP_FUNCTION(reset) ZEND_PARSE_PARAMETERS_END(); HashTable *array = get_ht_for_iap(array_zv, /* separate */ true); + if (zend_hash_num_elements(array) == 0) { + /* array->nInternalPointer is already 0 if the array is empty, even after removing elements */ + RETURN_FALSE; + } zend_hash_internal_pointer_reset(array); if (USED_RET()) { From b3937f7e09af74e9c350dc0a83102b6af0cc935c Mon Sep 17 00:00:00 2001 From: Tyson Andre Date: Fri, 3 Feb 2023 09:16:23 -0500 Subject: [PATCH 2/2] Update NEWS for reset/end/next/prev fix --- NEWS | 2 ++ 1 file changed, 2 insertions(+) diff --git a/NEWS b/NEWS index ca6ee38b7220b..f16e815291a3d 100644 --- a/NEWS +++ b/NEWS @@ -27,6 +27,8 @@ PHP NEWS . Fixed bug GH-10292 (Made the default value of the first param of srand() and mt_srand() unknown). (kocsismate) . Fix incorrect check in cs_8559_5 in map_from_unicode(). (nielsdos) + . Fix bug GH-9697 for reset/end/next/prev() attempting to move pointer of + properties table for certain internal classes such as FFI classes 02 Feb 2023, PHP 8.1.15