diff --git a/NEWS b/NEWS index 3da5464750cc..0417d9e600f9 100644 --- a/NEWS +++ b/NEWS @@ -151,6 +151,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 56049f30cf93..28d20d5c3a35 100644 --- a/UPGRADING +++ b/UPGRADING @@ -70,6 +70,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 a25cf3cd1bb7..cbd516226964 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_disallow_mutation_during_get_hash(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_disallow_mutation_during_get_hash())) { + 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_disallow_mutation_during_get_hash())) { + 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,17 @@ 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); + if (UNEXPECTED(!spl_object_storage_attach(intern, obj, &zv))) { + zval_ptr_dtor(&zv); + OBJ_RELEASE(obj); + return FAILURE; + } zval_ptr_dtor(&zv); OBJ_RELEASE(obj); } ZEND_HASH_FOREACH_END(); intern->index = 0; + return SUCCESS; } /* }}} */ #define SPL_OBJECT_STORAGE_CLASS_HAS_OVERRIDE(class_type, zstr_method) \ @@ -446,6 +474,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 +525,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_disallow_mutation_during_get_hash())) { + 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 +539,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_disallow_mutation_during_get_hash())) { + 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 +557,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_disallow_mutation_during_get_hash())) { + return; + } + if (UNEXPECTED(Z_TYPE_P(offset) != IS_OBJECT || (intern->flags & SOS_OVERRIDDEN_UNSET_DIMENSION))) { zend_std_unset_dimension(object, offset); return; @@ -535,6 +578,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 +612,9 @@ PHP_METHOD(SplObjectStorage, offsetGet) ZEND_PARSE_PARAMETERS_END(); if (spl_object_storage_get_hash(&key, intern, obj) == FAILURE) { + if (UNEXPECTED(EG(exception))) { + RETURN_THROWS(); + } RETURN_NULL(); } @@ -592,7 +641,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 +665,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 +695,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 +728,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 +824,9 @@ PHP_METHOD(SplObjectStorage, setInfo) if (zend_parse_parameters(ZEND_NUM_ARGS(), "z", &inf) == FAILURE) { RETURN_THROWS(); } + if (UNEXPECTED(spl_object_storage_disallow_mutation_during_get_hash())) { + RETURN_THROWS(); + } if ((element = zend_hash_get_current_data_ptr_ex(&intern->storage, &intern->pos)) == NULL) { RETURN_NULL(); @@ -947,6 +1021,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 +1038,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 +1141,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 +1239,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 +1261,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 ae063f1c9b03..9da6270b6d7d 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-- 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 af3fc381b562..aadbe2acba27 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 @@ $storage = new SplObjectStorage(); $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 b2ed211b304a..2602bc9e1f03 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 @@ $storage = new SplObjectStorage(); $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 000000000000..581012d86a4f --- /dev/null +++ b/ext/spl/tests/SplObjectStorage/gh21831.phpt @@ -0,0 +1,36 @@ +--TEST-- +GH-21831: SplObjectStorage::getHash() cannot mutate storage during removeAllExcept() +--FILE-- +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)