Fix concurrent iteration and deletion issues in SplObjectStorage

Closes GH-21443.

Author: ndossche

NEWS

@@ -26,6 +26,8 @@ PHP                                                                        NEWS
 - SPL:
   . Fixed bug GH-21499 (RecursiveArrayIterator getChildren UAF after parent
     free). (Girgias)
+  . Fix concurrent iteration and deletion issues in SplObjectStorage.
+    (ndossche)
 
 - Streams:
   . Fixed bug GH-21468 (Segfault in file_get_contents w/ a https URL

ext/spl/spl_observer.c

@@ -240,11 +240,25 @@ 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); \
+		zval *_z = __ht->arPacked; \
+		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);
+
 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 +640,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 *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(elem_obj);
 	} ZEND_HASH_FOREACH_END();
 
 	zend_hash_internal_pointer_reset_ex(&intern->storage, &intern->pos);

ext/spl/tests/SplObjectStorage/concurrent_deletion.phpt

@@ -0,0 +1,46 @@
+--TEST--
+SplObjectStorage: Concurrent deletion during iteration
+--CREDITS--
+cnitlrt
+--FILE--
+<?php
+class EvilStorage extends SplObjectStorage {
+    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;
+            }
+        }
+        return spl_object_hash($obj);
+    }
+}
+
+$victim = new SplObjectStorage();
+$other = new EvilStorage();
+
+for ($i = 0; $i < 1024; $i++) {
+    $o = new stdClass();
+    $victim[$o] = null;
+    $other[$o] = null;
+}
+
+var_dump($victim->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));
+?>
+--EXPECTF--
+int(%d)
+int(1024)

ext/spl/tests/SplObjectStorage/concurrent_deletion_addall.phpt

@@ -0,0 +1,43 @@
+--TEST--
+SplObjectStorage: Concurrent deletion during addAll
+--CREDITS--
+cnitlrt
+--FILE--
+<?php
+
+class EvilStorage extends SplObjectStorage {
+    public function getHash($obj): string {
+        global $storage;
+        unset($storage[$obj]);
+        return spl_object_hash($obj);
+    }
+}
+
+$storage = new SplObjectStorage();
+$storage[new stdClass] = 'foo';
+
+$evil = new EvilStorage();
+$evil->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) {
+  }
+}

ext/spl/tests/SplObjectStorage/concurrent_deletion_removeexcept.phpt

@@ -0,0 +1,35 @@
+--TEST--
+SplObjectStorage: Concurrent deletion during removeAllExcept
+--CREDITS--
+cnitlrt
+--FILE--
+<?php
+
+class EvilStorage extends SplObjectStorage {
+    public function getHash($obj): string {
+        global $storage;
+        unset($storage[$obj]);
+        return spl_object_hash($obj);
+    }
+}
+
+$storage = new SplObjectStorage();
+$storage[new stdClass] = 'foo';
+
+$evil = new EvilStorage();
+$storage->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) {
+  }
+}