From 34f1450a605aac36bd887c824f0f23916899066a Mon Sep 17 00:00:00 2001 From: Niels Dossche <7771979+nielsdos@users.noreply.github.com> Date: Sat, 21 Dec 2024 19:05:18 +0100 Subject: [PATCH 1/4] Fix GH-15833: Segmentation fault (access null pointer) in ext/spl/spl_array.c We're accessing the object properties table directly in spl, but we're not accounting for lazy objects. Upon accessing we should trigger the initialization as spl is doing direct manipulations on the object property table and expects a real object. --- ext/spl/spl_array.c | 5 +++++ ext/spl/tests/gh15833.phpt | 22 ++++++++++++++++++++++ 2 files changed, 27 insertions(+) create mode 100644 ext/spl/tests/gh15833.phpt diff --git a/ext/spl/spl_array.c b/ext/spl/spl_array.c index 0acbc98611966..f0d655c5a4704 100644 --- a/ext/spl/spl_array.c +++ b/ext/spl/spl_array.c @@ -74,6 +74,11 @@ static inline HashTable **spl_array_get_hash_table_ptr(spl_array_object* intern) return &Z_ARRVAL(intern->array); } else { zend_object *obj = Z_OBJ(intern->array); + /* Since we're directly playing with the properties table, we shall initialize the lazy object directly. + * If we don't, it's possible to continue working with the wrong object in case we're using a proxy. */ + if (UNEXPECTED(zend_lazy_object_must_init(obj))) { + obj = zend_lazy_object_init(obj); + } /* rebuild properties */ zend_std_get_properties_ex(obj); if (GC_REFCOUNT(obj->properties) > 1) { diff --git a/ext/spl/tests/gh15833.phpt b/ext/spl/tests/gh15833.phpt new file mode 100644 index 0000000000000..d658a770e80f5 --- /dev/null +++ b/ext/spl/tests/gh15833.phpt @@ -0,0 +1,22 @@ +--TEST-- +GH-15833 (Segmentation fault (access null pointer) in ext/spl/spl_array.c) +--CREDITS-- +YuanchengJiang +--FILE-- +newLazyProxy(function ($obj) { + $obj = new C(); + return $obj; +}); +$recursiveArrayIterator = new RecursiveArrayIterator($obj); +var_dump($recursiveArrayIterator->current()); +$recursiveArrayIterator->next(); +var_dump($recursiveArrayIterator->current()); +?> +--EXPECT-- +int(1) +NULL From 013582b12d7994262ae3ac198be8439586a045f9 Mon Sep 17 00:00:00 2001 From: Niels Dossche <7771979+nielsdos@users.noreply.github.com> Date: Sun, 22 Dec 2024 16:39:08 +0100 Subject: [PATCH 2/4] fix? --- ext/spl/spl_array.c | 5 ++++ .../tests/{gh15833.phpt => gh15833_1.phpt} | 0 ext/spl/tests/gh15833_2.phpt | 26 +++++++++++++++++++ 3 files changed, 31 insertions(+) rename ext/spl/tests/{gh15833.phpt => gh15833_1.phpt} (100%) create mode 100644 ext/spl/tests/gh15833_2.phpt diff --git a/ext/spl/spl_array.c b/ext/spl/spl_array.c index f0d655c5a4704..a60585712a872 100644 --- a/ext/spl/spl_array.c +++ b/ext/spl/spl_array.c @@ -78,6 +78,11 @@ static inline HashTable **spl_array_get_hash_table_ptr(spl_array_object* intern) * If we don't, it's possible to continue working with the wrong object in case we're using a proxy. */ if (UNEXPECTED(zend_lazy_object_must_init(obj))) { obj = zend_lazy_object_init(obj); + if (UNEXPECTED(!obj)) { + zval_ptr_dtor(&intern->array); + ZVAL_ARR(&intern->array, zend_new_array(0)); + return &Z_ARRVAL(intern->array); + } } /* rebuild properties */ zend_std_get_properties_ex(obj); diff --git a/ext/spl/tests/gh15833.phpt b/ext/spl/tests/gh15833_1.phpt similarity index 100% rename from ext/spl/tests/gh15833.phpt rename to ext/spl/tests/gh15833_1.phpt diff --git a/ext/spl/tests/gh15833_2.phpt b/ext/spl/tests/gh15833_2.phpt new file mode 100644 index 0000000000000..719c4900cb96a --- /dev/null +++ b/ext/spl/tests/gh15833_2.phpt @@ -0,0 +1,26 @@ +--TEST-- +GH-15833 (Segmentation fault (access null pointer) in ext/spl/spl_array.c) +--CREDITS-- +YuanchengJiang +--FILE-- +newLazyProxy(function ($obj) { + throw new Error('foo'); +}); +$recursiveArrayIterator = new RecursiveArrayIterator($obj); +try { + var_dump($recursiveArrayIterator->current()); +} catch (Error $e) { + echo $e->getMessage(), "\n"; +} +var_dump($recursiveArrayIterator->current()); +$recursiveArrayIterator->next(); +var_dump($recursiveArrayIterator->current()); +?> +--EXPECT-- +int(1) +NULL From 7b3a1f12f14ef79db6f7f4feafebe95736229076 Mon Sep 17 00:00:00 2001 From: Niels Dossche <7771979+nielsdos@users.noreply.github.com> Date: Sun, 22 Dec 2024 16:40:23 +0100 Subject: [PATCH 3/4] update test contents too ofc --- ext/spl/tests/gh15833_2.phpt | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/ext/spl/tests/gh15833_2.phpt b/ext/spl/tests/gh15833_2.phpt index 719c4900cb96a..534321e91b41e 100644 --- a/ext/spl/tests/gh15833_2.phpt +++ b/ext/spl/tests/gh15833_2.phpt @@ -22,5 +22,6 @@ $recursiveArrayIterator->next(); var_dump($recursiveArrayIterator->current()); ?> --EXPECT-- -int(1) +foo +NULL NULL From 456c6320ddf2fc3694d0257a3490322e23acb32f Mon Sep 17 00:00:00 2001 From: Niels Dossche <7771979+nielsdos@users.noreply.github.com> Date: Wed, 25 Dec 2024 22:52:17 +0100 Subject: [PATCH 4/4] review --- ext/spl/spl_array.c | 21 ++++++++++++++++++--- ext/spl/tests/gh15833_2.phpt | 28 +++++++++++++++++++++------- 2 files changed, 39 insertions(+), 10 deletions(-) diff --git a/ext/spl/spl_array.c b/ext/spl/spl_array.c index a60585712a872..db7f4fff9bb04 100644 --- a/ext/spl/spl_array.c +++ b/ext/spl/spl_array.c @@ -40,6 +40,7 @@ PHPAPI zend_class_entry *spl_ce_ArrayObject; typedef struct _spl_array_object { zval array; + HashTable *sentinel_array; uint32_t ht_iter; int ar_flags; unsigned char nApplyCount; @@ -79,11 +80,14 @@ static inline HashTable **spl_array_get_hash_table_ptr(spl_array_object* intern) if (UNEXPECTED(zend_lazy_object_must_init(obj))) { obj = zend_lazy_object_init(obj); if (UNEXPECTED(!obj)) { - zval_ptr_dtor(&intern->array); - ZVAL_ARR(&intern->array, zend_new_array(0)); - return &Z_ARRVAL(intern->array); + if (!intern->sentinel_array) { + intern->sentinel_array = zend_new_array(0); + } + return &intern->sentinel_array; } } + /* should no longer be lazy */ + ZEND_ASSERT(!zend_lazy_object_must_init(obj)); /* rebuild properties */ zend_std_get_properties_ex(obj); if (GC_REFCOUNT(obj->properties) > 1) { @@ -139,6 +143,10 @@ static void spl_array_object_free_storage(zend_object *object) zend_hash_iterator_del(intern->ht_iter); } + if (UNEXPECTED(intern->sentinel_array)) { + zend_array_release(intern->sentinel_array); + } + zend_object_std_dtor(&intern->std); zval_ptr_dtor(&intern->array); @@ -499,6 +507,9 @@ static void spl_array_write_dimension_ex(int check_inherited, zend_object *objec uint32_t refcount = 0; if (!offset || Z_TYPE_P(offset) == IS_NULL) { ht = spl_array_get_hash_table(intern); + if (UNEXPECTED(ht == intern->sentinel_array)) { + return; + } refcount = spl_array_set_refcount(intern->is_child, ht, 1); zend_hash_next_index_insert(ht, value); @@ -515,6 +526,10 @@ static void spl_array_write_dimension_ex(int check_inherited, zend_object *objec } ht = spl_array_get_hash_table(intern); + if (UNEXPECTED(ht == intern->sentinel_array)) { + spl_hash_key_release(&key); + return; + } refcount = spl_array_set_refcount(intern->is_child, ht, 1); if (key.key) { zend_hash_update_ind(ht, key.key, value); diff --git a/ext/spl/tests/gh15833_2.phpt b/ext/spl/tests/gh15833_2.phpt index 534321e91b41e..8f30921741fe3 100644 --- a/ext/spl/tests/gh15833_2.phpt +++ b/ext/spl/tests/gh15833_2.phpt @@ -9,7 +9,8 @@ class C { } $reflector = new ReflectionClass(C::class); $obj = $reflector->newLazyProxy(function ($obj) { - throw new Error('foo'); + static $counter = 0; + throw new Error('nope ' . ($counter++)); }); $recursiveArrayIterator = new RecursiveArrayIterator($obj); try { @@ -17,11 +18,24 @@ try { } catch (Error $e) { echo $e->getMessage(), "\n"; } -var_dump($recursiveArrayIterator->current()); -$recursiveArrayIterator->next(); -var_dump($recursiveArrayIterator->current()); +try { + var_dump($recursiveArrayIterator->current()); +} catch (Error $e) { + echo $e->getMessage(), "\n"; +} +try { + $recursiveArrayIterator->next(); +} catch (Error $e) { + echo $e->getMessage(), "\n"; +} +try { + var_dump($recursiveArrayIterator->current()); +} catch (Error $e) { + echo $e->getMessage(), "\n"; +} ?> --EXPECT-- -foo -NULL -NULL +nope 0 +nope 1 +nope 2 +nope 3