Fix GH-21831: Forbid SplObjectStorage mutation during getHash

Author: prateekbhujel

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)
 

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.

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_running;
+
 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_running)) {
+		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(&param, obj);
+		ZVAL_UNDEF(&rv);
+		spl_object_storage_get_hash_running++;
 		zend_call_method_with_1_params(&intern->std, intern->std.ce, &intern->fptr_get_hash, "getHash", &rv, &param);
+		spl_object_storage_get_hash_running--;
 		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,20 +270,25 @@ 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) {
 		zval zv;
 		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;

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)