From bcac21c3b22fbe7bdd48d06714ff523ed6e68cf3 Mon Sep 17 00:00:00 2001 From: ndossche <7771979+ndossche@users.noreply.github.com> Date: Sat, 14 Mar 2026 11:30:59 +0100 Subject: [PATCH 1/2] Fix concurrent iteration and deletion issues in SplObjectStorage --- ext/spl/spl_observer.c | 28 +++++++++-- .../SplObjectStorage/concurrent_deletion.phpt | 46 +++++++++++++++++++ .../concurrent_deletion_addall.phpt | 43 +++++++++++++++++ .../concurrent_deletion_removeexcept.phpt | 35 ++++++++++++++ 4 files changed, 147 insertions(+), 5 deletions(-) create mode 100644 ext/spl/tests/SplObjectStorage/concurrent_deletion.phpt create mode 100644 ext/spl/tests/SplObjectStorage/concurrent_deletion_addall.phpt create mode 100644 ext/spl/tests/SplObjectStorage/concurrent_deletion_removeexcept.phpt diff --git a/ext/spl/spl_observer.c b/ext/spl/spl_observer.c index 622686dbd78a6..e303b291092cd 100644 --- a/ext/spl/spl_observer.c +++ b/ext/spl/spl_observer.c @@ -240,11 +240,26 @@ static zend_result spl_object_storage_detach(spl_SplObjectStorage *intern, zend_ return ret; } /* }}}*/ +/* TODO: make this an official Zend API? */ +#define SPL_SAFE_HASH_FOREACH_PTR(_ht, _ptr) do { \ + const HashTable *__ht = (_ht); \ + size_t _size = ZEND_HASH_ELEMENT_SIZE(__ht); \ + zval *_z = __ht->arPacked; \ + for (uint32_t _idx = 0; _idx < __ht->nNumUsed; _idx++, _z = ZEND_HASH_ELEMENT_EX(__ht, _idx, _size)) { \ + if (UNEXPECTED(Z_ISUNDEF_P(_z))) continue; \ + _ptr = Z_PTR_P(_z); + static void spl_object_storage_addall(spl_SplObjectStorage *intern, spl_SplObjectStorage *other) { /* {{{ */ spl_SplObjectStorageElement *element; - ZEND_HASH_FOREACH_PTR(&other->storage, element) { - spl_object_storage_attach(intern, element->obj, &element->inf); + SPL_SAFE_HASH_FOREACH_PTR(&other->storage, element) { + zval zv; + zend_object *obj = element->obj; + GC_ADDREF(obj); + ZVAL_COPY(&zv, &element->inf); + spl_object_storage_attach(intern, obj, &zv); + zval_ptr_dtor(&zv); + OBJ_RELEASE(obj); } ZEND_HASH_FOREACH_END(); intern->index = 0; @@ -626,10 +641,13 @@ PHP_METHOD(SplObjectStorage, removeAllExcept) other = Z_SPLOBJSTORAGE_P(obj); - ZEND_HASH_FOREACH_PTR(&intern->storage, element) { - if (!spl_object_storage_contains(other, element->obj)) { - spl_object_storage_detach(intern, element->obj); + SPL_SAFE_HASH_FOREACH_PTR(&intern->storage, element) { + zend_object *obj = element->obj; + GC_ADDREF(obj); + if (!spl_object_storage_contains(other, obj)) { + spl_object_storage_detach(intern, obj); } + OBJ_RELEASE(obj); } ZEND_HASH_FOREACH_END(); zend_hash_internal_pointer_reset_ex(&intern->storage, &intern->pos); diff --git a/ext/spl/tests/SplObjectStorage/concurrent_deletion.phpt b/ext/spl/tests/SplObjectStorage/concurrent_deletion.phpt new file mode 100644 index 0000000000000..186acd15aea54 --- /dev/null +++ b/ext/spl/tests/SplObjectStorage/concurrent_deletion.phpt @@ -0,0 +1,46 @@ +--TEST-- +SplObjectStorage: Concurrent deletion during iteration +--CREDITS-- +cnitlrt +--FILE-- +removeAllExcept($other)); + +unset($victim, $other); +$victim = new SplObjectStorage(); +$other = new EvilStorage(); + +for ($i = 0; $i < 1024; $i++) { + $o = new stdClass(); + $victim[$o] = null; + $other[$o] = null; +} + +var_dump($other->addAll($victim)); +?> +--EXPECT-- +int(6737) +int(1024) diff --git a/ext/spl/tests/SplObjectStorage/concurrent_deletion_addall.phpt b/ext/spl/tests/SplObjectStorage/concurrent_deletion_addall.phpt new file mode 100644 index 0000000000000..af3fc381b5622 --- /dev/null +++ b/ext/spl/tests/SplObjectStorage/concurrent_deletion_addall.phpt @@ -0,0 +1,43 @@ +--TEST-- +SplObjectStorage: Concurrent deletion during addAll +--CREDITS-- +cnitlrt +--FILE-- +addAll($storage); + +var_dump($evil, $storage); + +?> +--EXPECTF-- +object(EvilStorage)#%d (1) { + ["storage":"SplObjectStorage":private]=> + array(1) { + [0]=> + array(2) { + ["obj"]=> + object(stdClass)#%d (0) { + } + ["inf"]=> + string(3) "foo" + } + } +} +object(SplObjectStorage)#%d (1) { + ["storage":"SplObjectStorage":private]=> + array(0) { + } +} diff --git a/ext/spl/tests/SplObjectStorage/concurrent_deletion_removeexcept.phpt b/ext/spl/tests/SplObjectStorage/concurrent_deletion_removeexcept.phpt new file mode 100644 index 0000000000000..b2ed211b304af --- /dev/null +++ b/ext/spl/tests/SplObjectStorage/concurrent_deletion_removeexcept.phpt @@ -0,0 +1,35 @@ +--TEST-- +SplObjectStorage: Concurrent deletion during removeAllExcept +--CREDITS-- +cnitlrt +--FILE-- +removeAllExcept($evil); + +var_dump($evil, $storage); + +?> +--EXPECTF-- +object(EvilStorage)#%d (1) { + ["storage":"SplObjectStorage":private]=> + array(0) { + } +} +object(SplObjectStorage)#%d (1) { + ["storage":"SplObjectStorage":private]=> + array(0) { + } +} From 4f4ecfa6ed48f757905624b24026800196559d5e Mon Sep 17 00:00:00 2001 From: ndossche <7771979+ndossche@users.noreply.github.com> Date: Sat, 14 Mar 2026 14:54:40 +0100 Subject: [PATCH 2/2] Fix 32-bit issue (related to heuristic abt pack/hash array) --- ext/spl/spl_observer.c | 13 ++++++------- .../tests/SplObjectStorage/concurrent_deletion.phpt | 4 ++-- 2 files changed, 8 insertions(+), 9 deletions(-) diff --git a/ext/spl/spl_observer.c b/ext/spl/spl_observer.c index e303b291092cd..e653d1173b246 100644 --- a/ext/spl/spl_observer.c +++ b/ext/spl/spl_observer.c @@ -243,9 +243,8 @@ static zend_result spl_object_storage_detach(spl_SplObjectStorage *intern, zend_ /* TODO: make this an official Zend API? */ #define SPL_SAFE_HASH_FOREACH_PTR(_ht, _ptr) do { \ const HashTable *__ht = (_ht); \ - size_t _size = ZEND_HASH_ELEMENT_SIZE(__ht); \ zval *_z = __ht->arPacked; \ - for (uint32_t _idx = 0; _idx < __ht->nNumUsed; _idx++, _z = ZEND_HASH_ELEMENT_EX(__ht, _idx, _size)) { \ + for (uint32_t _idx = 0; _idx < __ht->nNumUsed; _idx++, _z = ZEND_HASH_ELEMENT(__ht, _idx)) { \ if (UNEXPECTED(Z_ISUNDEF_P(_z))) continue; \ _ptr = Z_PTR_P(_z); @@ -642,12 +641,12 @@ PHP_METHOD(SplObjectStorage, removeAllExcept) other = Z_SPLOBJSTORAGE_P(obj); SPL_SAFE_HASH_FOREACH_PTR(&intern->storage, element) { - zend_object *obj = element->obj; - GC_ADDREF(obj); - if (!spl_object_storage_contains(other, obj)) { - spl_object_storage_detach(intern, obj); + zend_object *elem_obj = element->obj; + GC_ADDREF(elem_obj); + if (!spl_object_storage_contains(other, elem_obj)) { + spl_object_storage_detach(intern, elem_obj); } - OBJ_RELEASE(obj); + OBJ_RELEASE(elem_obj); } ZEND_HASH_FOREACH_END(); zend_hash_internal_pointer_reset_ex(&intern->storage, &intern->pos); diff --git a/ext/spl/tests/SplObjectStorage/concurrent_deletion.phpt b/ext/spl/tests/SplObjectStorage/concurrent_deletion.phpt index 186acd15aea54..ae063f1c9b03a 100644 --- a/ext/spl/tests/SplObjectStorage/concurrent_deletion.phpt +++ b/ext/spl/tests/SplObjectStorage/concurrent_deletion.phpt @@ -41,6 +41,6 @@ for ($i = 0; $i < 1024; $i++) { var_dump($other->addAll($victim)); ?> ---EXPECT-- -int(6737) +--EXPECTF-- +int(%d) int(1024)