Zend: Remove zend_hash_graceful_destroy() and fix GH-8438

UPGRADING.INTERNALS

@@ -53,6 +53,8 @@ PHP 8.7 INTERNALS UPGRADE NOTES
   freed with zend_hash_destroy() + pefree().
 - The ZTS symbols compiler_globals_offset and executor_globals_offset have been
   removed: CG()/EG() now reach the globals through a __thread struct.
+- Removed zend_hash_graceful_destroy(). Use zend_hash_graceful_reverse_destroy()
+  or zend_hash_destroy() instead.
 - Removed zend_error_noreturn_unchecked().
 
 ========================

Zend/tests/gh8438.phpt

@@ -0,0 +1,29 @@
+--TEST--
+GH-8438 (HashTableIterators of empty hashtables are not poisoned upon destruction)
+--EXTENSIONS--
+zend_test
+--FILE--
+<?php
+
+$destroyFunctions = ['zend_hash_destroy', 'zend_hash_graceful_reverse_destroy'];
+$states = ['uninitialized', 'packed', 'packed_emptied', 'hash', 'hash_emptied'];
+
+foreach ($destroyFunctions as $destroy) {
+    foreach ($states as $state) {
+        echo "$destroy, $state: ";
+        var_dump(zend_test_hash_destroy_detaches_iterator($destroy, $state));
+    }
+}
+
+?>
+--EXPECT--
+zend_hash_destroy, uninitialized: bool(true)
+zend_hash_destroy, packed: bool(true)
+zend_hash_destroy, packed_emptied: bool(true)
+zend_hash_destroy, hash: bool(true)
+zend_hash_destroy, hash_emptied: bool(true)
+zend_hash_graceful_reverse_destroy, uninitialized: bool(true)
+zend_hash_graceful_reverse_destroy, packed: bool(true)
+zend_hash_graceful_reverse_destroy, packed_emptied: bool(true)
+zend_hash_graceful_reverse_destroy, hash: bool(true)
+zend_hash_graceful_reverse_destroy, hash_emptied: bool(true)

Zend/zend_hash.c

@@ -1986,7 +1986,6 @@ ZEND_API void ZEND_FASTCALL zend_hash_destroy(HashTable *ht)
 				}
 				SET_INCONSISTENT(HT_DESTROYED);
 			}
-			zend_hash_iterators_remove(ht);
 		} else {
 			Bucket *p = ht->arData;
 			Bucket *end = p + ht->nNumUsed;
@@ -2034,12 +2033,13 @@ ZEND_API void ZEND_FASTCALL zend_hash_destroy(HashTable *ht)
 					} while (++p != end);
 				}
 			}
-			zend_hash_iterators_remove(ht);
 		}
-	} else if (EXPECTED(HT_FLAGS(ht) & HASH_FLAG_UNINITIALIZED)) {
-		return;
 	}
-	pefree(HT_GET_DATA_ADDR(ht), GC_FLAGS(ht) & IS_ARRAY_PERSISTENT);
+
+	zend_hash_iterators_remove(ht);
+	if (!(HT_FLAGS(ht) & HASH_FLAG_UNINITIALIZED)) {
+		pefree(HT_GET_DATA_ADDR(ht), GC_FLAGS(ht) & IS_ARRAY_PERSISTENT);
+	}
 }
 
 ZEND_API void ZEND_FASTCALL zend_array_destroy(HashTable *ht)
@@ -2223,35 +2223,6 @@ ZEND_API void ZEND_FASTCALL zend_symtable_clean(HashTable *ht)
 	ht->nInternalPointer = 0;
 }
 
-ZEND_API void ZEND_FASTCALL zend_hash_graceful_destroy(HashTable *ht)
-{
-	uint32_t idx;
-
-	IS_CONSISTENT(ht);
-	HT_ASSERT_RC1(ht);
-
-	if (HT_IS_PACKED(ht)) {
-		zval *zv = ht->arPacked;
-
-		for (idx = 0; idx < ht->nNumUsed; idx++, zv++) {
-			if (UNEXPECTED(Z_TYPE_P(zv) == IS_UNDEF)) continue;
-			_zend_hash_packed_del_val(ht, HT_IDX_TO_HASH(idx), zv);
-		}
-	} else {
-		Bucket *p = ht->arData;
-
-		for (idx = 0; idx < ht->nNumUsed; idx++, p++) {
-			if (UNEXPECTED(Z_TYPE(p->val) == IS_UNDEF)) continue;
-			_zend_hash_del_el(ht, HT_IDX_TO_HASH(idx), p);
-		}
-	}
-	if (!(HT_FLAGS(ht) & HASH_FLAG_UNINITIALIZED)) {
-		pefree(HT_GET_DATA_ADDR(ht), GC_FLAGS(ht) & IS_ARRAY_PERSISTENT);
-	}
-
-	SET_INCONSISTENT(HT_DESTROYED);
-}
-
 ZEND_API void ZEND_FASTCALL zend_hash_graceful_reverse_destroy(HashTable *ht)
 {
 	uint32_t idx;
@@ -2280,6 +2251,7 @@ ZEND_API void ZEND_FASTCALL zend_hash_graceful_reverse_destroy(HashTable *ht)
 		}
 	}
 
+	zend_hash_iterators_remove(ht);
 	if (!(HT_FLAGS(ht) & HASH_FLAG_UNINITIALIZED)) {
 		pefree(HT_GET_DATA_ADDR(ht), GC_FLAGS(ht) & IS_ARRAY_PERSISTENT);
 	}

Zend/zend_hash.h

@@ -166,7 +166,6 @@ typedef int (*apply_func_t)(zval *pDest);
 typedef int (*apply_func_arg_t)(zval *pDest, void *argument);
 typedef int (*apply_func_args_t)(zval *pDest, int num_args, va_list args, zend_hash_key *hash_key);
 
-ZEND_API void ZEND_FASTCALL zend_hash_graceful_destroy(HashTable *ht);
 ZEND_API void ZEND_FASTCALL zend_hash_graceful_reverse_destroy(HashTable *ht);
 ZEND_API void ZEND_FASTCALL zend_hash_apply(HashTable *ht, apply_func_t apply_func);
 ZEND_API void ZEND_FASTCALL zend_hash_apply_with_argument(HashTable *ht, apply_func_arg_t apply_func, void *);

ext/zend_test/test.c

@@ -2793,3 +2793,80 @@ static PHP_FUNCTION(zend_test_gh19792)
 	zend_error(E_WARNING, "a warning");
 	zend_throw_error(NULL, "an exception");
 }
+
+/* Returns whether destroying a HashTable detaches the iterators on it. */
+static PHP_FUNCTION(zend_test_hash_destroy_detaches_iterator)
+{
+	zend_string *destroy, *state;
+
+	ZEND_PARSE_PARAMETERS_START(2, 2)
+		Z_PARAM_STR(destroy)
+		Z_PARAM_STR(state)
+	ZEND_PARSE_PARAMETERS_END();
+
+	void (ZEND_FASTCALL *destroy_func)(HashTable *);
+	if (zend_string_equals_literal(destroy, "zend_hash_destroy")) {
+		destroy_func = zend_hash_destroy;
+	} else if (zend_string_equals_literal(destroy, "zend_hash_graceful_reverse_destroy")) {
+		destroy_func = zend_hash_graceful_reverse_destroy;
+	} else {
+		zend_argument_value_error(1, "must be \"zend_hash_destroy\" or \"zend_hash_graceful_reverse_destroy\"");
+		RETURN_THROWS();
+	}
+
+	bool packed;
+	bool emptied;
+	if (zend_string_equals_literal(state, "uninitialized")) {
+		packed = false;
+		emptied = false;
+	} else if (zend_string_equals_literal(state, "packed")) {
+		packed = true;
+		emptied = false;
+	} else if (zend_string_equals_literal(state, "packed_emptied")) {
+		packed = true;
+		emptied = true;
+	} else if (zend_string_equals_literal(state, "hash")) {
+		packed = false;
+		emptied = false;
+	} else if (zend_string_equals_literal(state, "hash_emptied")) {
+		packed = false;
+		emptied = true;
+	} else {
+		zend_argument_value_error(2, "must be \"uninitialized\", \"packed\", \"packed_emptied\", \"hash\", or \"hash_emptied\"");
+		RETURN_THROWS();
+	}
+
+	HashTable *ht = emalloc(sizeof(HashTable));
+	zend_hash_init(ht, 8, NULL, ZVAL_PTR_DTOR, false);
+
+	if (!zend_string_equals_literal(state, "uninitialized")) {
+		zval value;
+		ZVAL_STR(&value, zend_string_init("value", strlen("value"), false));
+		if (packed) {
+			zend_hash_index_add_new(ht, 0, &value);
+			ZEND_ASSERT(HT_IS_PACKED(ht));
+		} else {
+			zend_hash_str_add_new(ht, "key", strlen("key"), &value);
+			ZEND_ASSERT(!HT_IS_PACKED(ht));
+		}
+		if (emptied) {
+			if (packed) {
+				zend_hash_index_del(ht, 0);
+			} else {
+				zend_hash_str_del(ht, "key", strlen("key"));
+			}
+			ZEND_ASSERT(ht->nNumUsed == 0);
+		}
+	}
+
+	uint32_t iter_idx = zend_hash_iterator_add(ht, 0);
+	destroy_func(ht);
+	bool detached = EG(ht_iterators)[iter_idx].ht != ht;
+
+	/* Delete the iterator before ht is freed, so that this helper stays memory safe
+	 * when the iterator was not detached. */
+	zend_hash_iterator_del(iter_idx);
+	efree(ht);
+
+	RETURN_BOOL(detached);
+}

ext/zend_test/test.stub.php

@@ -472,6 +472,8 @@ function zend_test_uri_parser(string $uri, string $parser): array { }
     /** @compile-time-eval */
     function zend_test_gh19792(): void {}
 
+    function zend_test_hash_destroy_detaches_iterator(string $destroy, string $state): bool {}
+
     /* @param resource $stream */
     function zend_test_handle_post_data(mixed $stream, string $content_type, int $content_length): void {}