Commit d694135da05 for php.net
commit d694135da0504b8dffab29157482695614d69cf2
Author: Pratik Bhujel <prateekbhujelpb@gmail.com>
Date: Mon May 4 18:28:41 2026 +0545
Fix GH-21831: Disallow SplObjectStorage mutation during getHash() (#21835)
Fixes GH-21831.
`getHash()` runs while `SplObjectStorage` is looking up an object. Letting it mutate a storage from there is unsafe, because the caller may still be iterating the table that just changed underneath it.
This makes that fail immediately with an `Error` instead of trying to handle it one caller at a time.
The existing concurrent deletion tests now cover the new rule, and GH-21831 has a direct regression test. This targets master, so the behavior change is also noted in `UPGRADING`.
diff --git a/NEWS b/NEWS
index 45524b382c7..34fbfc5bb38 100644
--- a/NEWS
+++ b/NEWS
@@ -160,6 +160,8 @@ PHP NEWS
- SPL:
. DirectoryIterator key can now work better with filesystem supporting larger
directory indexing. (David Carlier)
+ . Fixed bug GH-21831 (SplObjectStorage::removeAllExcept() use-after-free
+ with re-entrant getHash()). (Pratik Bhujel)
. Fix bugs GH-8561, GH-8562, GH-8563, and GH-8564 (Fixing various
SplFileObject iterator desync bugs). (iliaal)
diff --git a/UPGRADING b/UPGRADING
index 4cbe4a98105..3540aee482d 100644
--- a/UPGRADING
+++ b/UPGRADING
@@ -77,6 +77,8 @@ PHP 8.6 UPGRADE NOTES
logic in their updateTimestamp() method.
- SPL:
+ . SplObjectStorage::getHash() implementations may no longer mutate any
+ SplObjectStorage instance. Attempting to do so now throws an Error.
. SplFileObject::next() now advances the stream when no prior current()
call has cached a line. A subsequent current() call returns the new
line rather than the previous one.
diff --git a/ext/spl/spl_observer.c b/ext/spl/spl_observer.c
index a25cf3cd1bb..1c6156f6bbf 100644
--- a/ext/spl/spl_observer.c
+++ b/ext/spl/spl_observer.c
@@ -45,6 +45,8 @@ static zend_object_handlers spl_handler_MultipleIterator;
#define SOS_OVERRIDDEN_WRITE_DIMENSION 2
#define SOS_OVERRIDDEN_UNSET_DIMENSION 4
+ZEND_TLS uint32_t spl_object_storage_get_hash_depth;
+
typedef struct _spl_SplObjectStorage { /* {{{ */
HashTable storage;
zend_long index;
@@ -69,6 +71,16 @@ static inline spl_SplObjectStorage *spl_object_storage_from_obj(zend_object *obj
#define Z_SPLOBJSTORAGE_P(zv) spl_object_storage_from_obj(Z_OBJ_P((zv)))
+static zend_always_inline bool spl_object_storage_is_mutating_within_get_hash_call(void)
+{
+ if (UNEXPECTED(spl_object_storage_get_hash_depth)) {
+ zend_throw_error(NULL, "Modification of SplObjectStorage during getHash() is prohibited");
+ return true;
+ }
+
+ return false;
+}
+
static void spl_SplObjectStorage_free_storage(zend_object *object) /* {{{ */
{
spl_SplObjectStorage *intern = spl_object_storage_from_obj(object);
@@ -83,7 +95,10 @@ static zend_result spl_object_storage_get_hash(zend_hash_key *key, spl_SplObject
zval param;
zval rv;
ZVAL_OBJ(¶m, obj);
+ ZVAL_UNDEF(&rv);
+ spl_object_storage_get_hash_depth++;
zend_call_method_with_1_params(&intern->std, intern->std.ce, &intern->fptr_get_hash, "getHash", &rv, ¶m);
+ spl_object_storage_get_hash_depth--;
if (UNEXPECTED(Z_ISUNDEF(rv))) {
/* An exception has occurred */
return FAILURE;
@@ -176,6 +191,10 @@ static spl_SplObjectStorageElement *spl_object_storage_attach_handle(spl_SplObje
static spl_SplObjectStorageElement *spl_object_storage_attach(spl_SplObjectStorage *intern, zend_object *obj, zval *inf) /* {{{ */
{
+ if (UNEXPECTED(spl_object_storage_is_mutating_within_get_hash_call())) {
+ return NULL;
+ }
+
if (EXPECTED(!(intern->flags & SOS_OVERRIDDEN_WRITE_DIMENSION))) {
return spl_object_storage_attach_handle(intern, obj, inf);
}
@@ -221,6 +240,10 @@ static spl_SplObjectStorageElement *spl_object_storage_attach(spl_SplObjectStora
static zend_result spl_object_storage_detach(spl_SplObjectStorage *intern, zend_object *obj) /* {{{ */
{
+ if (UNEXPECTED(spl_object_storage_is_mutating_within_get_hash_call())) {
+ return FAILURE;
+ }
+
if (EXPECTED(!(intern->flags & SOS_OVERRIDDEN_UNSET_DIMENSION))) {
return zend_hash_index_del(&intern->storage, obj->handle);
}
@@ -247,7 +270,7 @@ static zend_result spl_object_storage_detach(spl_SplObjectStorage *intern, zend_
if (UNEXPECTED(Z_ISUNDEF_P(_z))) continue; \
_ptr = Z_PTR_P(_z);
-static void spl_object_storage_addall(spl_SplObjectStorage *intern, spl_SplObjectStorage *other) { /* {{{ */
+static zend_result spl_object_storage_addall(spl_SplObjectStorage *intern, spl_SplObjectStorage *other) { /* {{{ */
spl_SplObjectStorageElement *element;
SPL_SAFE_HASH_FOREACH_PTR(&other->storage, element) {
@@ -255,12 +278,16 @@ static void spl_object_storage_addall(spl_SplObjectStorage *intern, spl_SplObjec
zend_object *obj = element->obj;
GC_ADDREF(obj);
ZVAL_COPY(&zv, &element->inf);
- spl_object_storage_attach(intern, obj, &zv);
+ spl_SplObjectStorageElement *attached = spl_object_storage_attach(intern, obj, &zv);
zval_ptr_dtor(&zv);
OBJ_RELEASE(obj);
+ if (UNEXPECTED(!attached)) {
+ return FAILURE;
+ }
} ZEND_HASH_FOREACH_END();
intern->index = 0;
+ return SUCCESS;
} /* }}} */
#define SPL_OBJECT_STORAGE_CLASS_HAS_OVERRIDE(class_type, zstr_method) \
@@ -446,6 +473,9 @@ PHP_METHOD(SplObjectStorage, attach)
Z_PARAM_ZVAL(inf)
ZEND_PARSE_PARAMETERS_END();
spl_object_storage_attach(intern, obj, inf);
+ if (UNEXPECTED(EG(exception))) {
+ RETURN_THROWS();
+ }
} /* }}} */
// todo: make spl_object_storage_has_dimension return bool as well
@@ -494,6 +524,10 @@ static zval *spl_object_storage_read_dimension(zend_object *object, zval *offset
static void spl_object_storage_write_dimension(zend_object *object, zval *offset, zval *inf)
{
spl_SplObjectStorage *intern = spl_object_storage_from_obj(object);
+ if (UNEXPECTED(spl_object_storage_is_mutating_within_get_hash_call())) {
+ return;
+ }
+
if (UNEXPECTED(offset == NULL || Z_TYPE_P(offset) != IS_OBJECT || (intern->flags & SOS_OVERRIDDEN_WRITE_DIMENSION))) {
zend_std_write_dimension(object, offset, inf);
return;
@@ -504,6 +538,10 @@ static void spl_object_storage_write_dimension(zend_object *object, zval *offset
static void spl_multiple_iterator_write_dimension(zend_object *object, zval *offset, zval *inf)
{
spl_SplObjectStorage *intern = spl_object_storage_from_obj(object);
+ if (UNEXPECTED(spl_object_storage_is_mutating_within_get_hash_call())) {
+ return;
+ }
+
if (UNEXPECTED(offset == NULL || Z_TYPE_P(offset) != IS_OBJECT || (intern->flags & SOS_OVERRIDDEN_WRITE_DIMENSION))) {
zend_std_write_dimension(object, offset, inf);
return;
@@ -518,6 +556,10 @@ static void spl_multiple_iterator_write_dimension(zend_object *object, zval *off
static void spl_object_storage_unset_dimension(zend_object *object, zval *offset)
{
spl_SplObjectStorage *intern = spl_object_storage_from_obj(object);
+ if (UNEXPECTED(spl_object_storage_is_mutating_within_get_hash_call())) {
+ return;
+ }
+
if (UNEXPECTED(Z_TYPE_P(offset) != IS_OBJECT || (intern->flags & SOS_OVERRIDDEN_UNSET_DIMENSION))) {
zend_std_unset_dimension(object, offset);
return;
@@ -535,6 +577,9 @@ PHP_METHOD(SplObjectStorage, detach)
Z_PARAM_OBJ(obj)
ZEND_PARSE_PARAMETERS_END();
spl_object_storage_detach(intern, obj);
+ if (UNEXPECTED(EG(exception))) {
+ RETURN_THROWS();
+ }
zend_hash_internal_pointer_reset_ex(&intern->storage, &intern->pos);
intern->index = 0;
@@ -566,6 +611,7 @@ PHP_METHOD(SplObjectStorage, offsetGet)
ZEND_PARSE_PARAMETERS_END();
if (spl_object_storage_get_hash(&key, intern, obj) == FAILURE) {
+ /* This may be the old NULL fallback, or an exception thrown by getHash(). */
RETURN_NULL();
}
@@ -592,7 +638,9 @@ PHP_METHOD(SplObjectStorage, addAll)
other = Z_SPLOBJSTORAGE_P(obj);
- spl_object_storage_addall(intern, other);
+ if (UNEXPECTED(spl_object_storage_addall(intern, other) == FAILURE)) {
+ RETURN_THROWS();
+ }
RETURN_LONG(zend_hash_num_elements(&intern->storage));
} /* }}} */
@@ -614,6 +662,9 @@ PHP_METHOD(SplObjectStorage, removeAll)
zend_hash_internal_pointer_reset(&other->storage);
while ((element = zend_hash_get_current_data_ptr(&other->storage)) != NULL) {
if (spl_object_storage_detach(intern, element->obj) == FAILURE) {
+ if (UNEXPECTED(EG(exception))) {
+ RETURN_THROWS();
+ }
zend_hash_move_forward(&other->storage);
}
}
@@ -641,8 +692,19 @@ PHP_METHOD(SplObjectStorage, removeAllExcept)
SPL_SAFE_HASH_FOREACH_PTR(&intern->storage, element) {
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);
+ bool contains = spl_object_storage_contains(other, elem_obj);
+ if (UNEXPECTED(EG(exception))) {
+ OBJ_RELEASE(elem_obj);
+ RETURN_THROWS();
+ }
+ if (!contains) {
+ if (spl_object_storage_detach(intern, elem_obj) == FAILURE) {
+ OBJ_RELEASE(elem_obj);
+ if (UNEXPECTED(EG(exception))) {
+ RETURN_THROWS();
+ }
+ continue;
+ }
}
OBJ_RELEASE(elem_obj);
} ZEND_HASH_FOREACH_END();
@@ -663,7 +725,13 @@ PHP_METHOD(SplObjectStorage, contains)
ZEND_PARSE_PARAMETERS_START(1, 1)
Z_PARAM_OBJ(obj)
ZEND_PARSE_PARAMETERS_END();
- RETURN_BOOL(spl_object_storage_contains(intern, obj));
+
+ bool contains = spl_object_storage_contains(intern, obj);
+ if (UNEXPECTED(EG(exception))) {
+ RETURN_THROWS();
+ }
+
+ RETURN_BOOL(contains);
} /* }}} */
/* {{{ Determine number of objects in storage */
@@ -753,6 +821,9 @@ PHP_METHOD(SplObjectStorage, setInfo)
if (zend_parse_parameters(ZEND_NUM_ARGS(), "z", &inf) == FAILURE) {
RETURN_THROWS();
}
+ if (UNEXPECTED(spl_object_storage_is_mutating_within_get_hash_call())) {
+ RETURN_THROWS();
+ }
if ((element = zend_hash_get_current_data_ptr_ex(&intern->storage, &intern->pos)) == NULL) {
RETURN_NULL();
@@ -947,6 +1018,10 @@ PHP_METHOD(SplObjectStorage, unserialize)
if (spl_object_storage_get_hash(&key, intern, Z_OBJ_P(entry)) == FAILURE) {
zval_ptr_dtor(&inf);
+ if (EG(exception)) {
+ PHP_VAR_UNSERIALIZE_DESTROY(var_hash);
+ RETURN_THROWS();
+ }
goto outexcept;
}
pelement = spl_object_storage_get(intern, &key);
@@ -960,6 +1035,14 @@ PHP_METHOD(SplObjectStorage, unserialize)
var_push_dtor(&var_hash, &obj);
}
element = spl_object_storage_attach(intern, Z_OBJ_P(entry), Z_ISUNDEF(inf)?NULL:&inf);
+ if (UNEXPECTED(!element)) {
+ zval_ptr_dtor(&inf);
+ if (EG(exception)) {
+ PHP_VAR_UNSERIALIZE_DESTROY(var_hash);
+ RETURN_THROWS();
+ }
+ goto outexcept;
+ }
var_replace(&var_hash, &inf, &element->inf);
zval_ptr_dtor(&inf);
}
@@ -1055,7 +1138,9 @@ PHP_METHOD(SplObjectStorage, __unserialize)
}
ZVAL_DEREF(val);
- spl_object_storage_attach(intern, Z_OBJ_P(key), val);
+ if (UNEXPECTED(!spl_object_storage_attach(intern, Z_OBJ_P(key), val))) {
+ RETURN_THROWS();
+ }
key = NULL;
} else {
key = val;
@@ -1151,8 +1236,14 @@ PHP_METHOD(MultipleIterator, attachIterator)
}
spl_object_storage_attach(intern, iterator, &zinfo);
+ if (UNEXPECTED(EG(exception))) {
+ RETURN_THROWS();
+ }
} else {
spl_object_storage_attach(intern, iterator, NULL);
+ if (UNEXPECTED(EG(exception))) {
+ RETURN_THROWS();
+ }
}
}
/* }}} */
@@ -1167,6 +1258,9 @@ PHP_METHOD(MultipleIterator, detachIterator)
RETURN_THROWS();
}
spl_object_storage_detach(intern, Z_OBJ_P(iterator));
+ if (UNEXPECTED(EG(exception))) {
+ RETURN_THROWS();
+ }
zend_hash_internal_pointer_reset_ex(&intern->storage, &intern->pos);
intern->index = 0;
diff --git a/ext/spl/tests/SplObjectStorage/concurrent_deletion.phpt b/ext/spl/tests/SplObjectStorage/concurrent_deletion.phpt
index ae063f1c9b0..9da6270b6d7 100644
--- a/ext/spl/tests/SplObjectStorage/concurrent_deletion.phpt
+++ b/ext/spl/tests/SplObjectStorage/concurrent_deletion.phpt
@@ -1,46 +1,62 @@
--TEST--
-SplObjectStorage: Concurrent deletion during iteration
+SplObjectStorage: Mutation during getHash is prohibited
--CREDITS--
cnitlrt
--FILE--
<?php
class EvilStorage extends SplObjectStorage {
+ public bool $mutate = false;
+
public function getHash($obj): string {
global $victim;
- static $counter = 0;
- if ($counter++ < 1024*2) {
- // Re-entrant mutation during removeAllExcept iteration
- for ($i = 0; $i < 5; $i++) {
- $victim[new stdClass()] = null;
- }
+ if ($this->mutate) {
+ $victim[new stdClass()] = null;
}
return spl_object_hash($obj);
}
}
+function populate(SplObjectStorage $victim, SplObjectStorage $other): void {
+ for ($i = 0; $i < 1024; $i++) {
+ $o = new stdClass();
+ $victim[$o] = null;
+ $other[$o] = null;
+ }
+}
+
$victim = new SplObjectStorage();
$other = new EvilStorage();
-for ($i = 0; $i < 1024; $i++) {
- $o = new stdClass();
- $victim[$o] = null;
- $other[$o] = null;
+populate($victim, $other);
+$other->mutate = true;
+
+try {
+ $victim->removeAllExcept($other);
+} catch (Error $e) {
+ echo $e->getMessage(), "\n";
}
-var_dump($victim->removeAllExcept($other));
+var_dump(count($victim), count($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;
+populate($victim, $other);
+$other->mutate = true;
+
+try {
+ $other->addAll($victim);
+} catch (Error $e) {
+ echo $e->getMessage(), "\n";
}
-var_dump($other->addAll($victim));
+var_dump(count($victim), count($other));
?>
---EXPECTF--
-int(%d)
+--EXPECT--
+Modification of SplObjectStorage during getHash() is prohibited
+int(1024)
+int(1024)
+Modification of SplObjectStorage during getHash() is prohibited
+int(1024)
int(1024)
diff --git a/ext/spl/tests/SplObjectStorage/concurrent_deletion_addall.phpt b/ext/spl/tests/SplObjectStorage/concurrent_deletion_addall.phpt
index af3fc381b56..aadbe2acba2 100644
--- a/ext/spl/tests/SplObjectStorage/concurrent_deletion_addall.phpt
+++ b/ext/spl/tests/SplObjectStorage/concurrent_deletion_addall.phpt
@@ -1,5 +1,5 @@
--TEST--
-SplObjectStorage: Concurrent deletion during addAll
+SplObjectStorage: Mutation during getHash is prohibited during addAll
--CREDITS--
cnitlrt
--FILE--
@@ -17,27 +17,16 @@ public function getHash($obj): string {
$storage[new stdClass] = 'foo';
$evil = new EvilStorage();
-$evil->addAll($storage);
+try {
+ $evil->addAll($storage);
+} catch (Error $e) {
+ echo $e->getMessage(), "\n";
+}
-var_dump($evil, $storage);
+var_dump(count($evil), count($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) {
- }
-}
+--EXPECT--
+Modification of SplObjectStorage during getHash() is prohibited
+int(0)
+int(1)
diff --git a/ext/spl/tests/SplObjectStorage/concurrent_deletion_removeexcept.phpt b/ext/spl/tests/SplObjectStorage/concurrent_deletion_removeexcept.phpt
index b2ed211b304..2602bc9e1f0 100644
--- a/ext/spl/tests/SplObjectStorage/concurrent_deletion_removeexcept.phpt
+++ b/ext/spl/tests/SplObjectStorage/concurrent_deletion_removeexcept.phpt
@@ -1,5 +1,5 @@
--TEST--
-SplObjectStorage: Concurrent deletion during removeAllExcept
+SplObjectStorage: Mutation during getHash is prohibited during removeAllExcept
--CREDITS--
cnitlrt
--FILE--
@@ -17,19 +17,16 @@ public function getHash($obj): string {
$storage[new stdClass] = 'foo';
$evil = new EvilStorage();
-$storage->removeAllExcept($evil);
+try {
+ $storage->removeAllExcept($evil);
+} catch (Error $e) {
+ echo $e->getMessage(), "\n";
+}
-var_dump($evil, $storage);
+var_dump(count($evil), count($storage));
?>
---EXPECTF--
-object(EvilStorage)#%d (1) {
- ["storage":"SplObjectStorage":private]=>
- array(0) {
- }
-}
-object(SplObjectStorage)#%d (1) {
- ["storage":"SplObjectStorage":private]=>
- array(0) {
- }
-}
+--EXPECT--
+Modification of SplObjectStorage during getHash() is prohibited
+int(0)
+int(1)
diff --git a/ext/spl/tests/SplObjectStorage/gh21831.phpt b/ext/spl/tests/SplObjectStorage/gh21831.phpt
new file mode 100644
index 00000000000..581012d86a4
--- /dev/null
+++ b/ext/spl/tests/SplObjectStorage/gh21831.phpt
@@ -0,0 +1,36 @@
+--TEST--
+GH-21831: SplObjectStorage::getHash() cannot mutate storage during removeAllExcept()
+--FILE--
+<?php
+
+class FilterStorage extends SplObjectStorage {
+ public ?SplObjectStorage $other = null;
+
+ public function getHash($obj): string {
+ if ($this->other) {
+ $this->other->offsetUnset($obj);
+ $this->other = null;
+ }
+
+ return 'x';
+ }
+}
+
+$storage = new SplObjectStorage();
+$storage[new stdClass()] = null;
+
+$filter = new FilterStorage();
+$filter->other = $storage;
+
+try {
+ $storage->removeAllExcept($filter);
+} catch (Error $e) {
+ echo $e->getMessage(), "\n";
+}
+
+var_dump(count($storage));
+
+?>
+--EXPECT--
+Modification of SplObjectStorage during getHash() is prohibited
+int(1)