Skip to content

Assert that destroyed hash tables are not in GC buffer #24739

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
arnaud-lb merged 2 commits into from
Oct 27, 2026
Merged

Assert that destroyed hash tables are not in GC buffer #24739

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e56e425
Assert that destroyed hash tables are not in GC buffer
arnaud-lb Oct 26, 2026
a536dac
Handle array_splice()
arnaud-lb Oct 27, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 23 additions & 0 deletions Zend/tests/gh24739.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
--TEST--
array_splice() resets type info before destroying hash table
--ENV--
a=1
--FILE--
<?php

$a = [getenv('a'), getenv('a'), getenv('a')];
$b = $a;
unset($b); // add to GC buffer

array_splice($a, 1, 1);

var_dump($a);

?>
--EXPECT--
array(2) {
[0]=>
string(1) "1"
[1]=>
string(1) "1"
}
3 changes: 3 additions & 0 deletions Zend/zend_hash.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1962,6 +1962,9 @@ ZEND_API void ZEND_FASTCALL zend_hash_destroy(HashTable *ht)
{
IS_CONSISTENT(ht);
HT_ASSERT(ht, GC_REFCOUNT(ht) <= 1);
ZEND_ASSERT(((GC_TYPE(ht) == IS_ARRAY && !(GC_TYPE_INFO(ht) & GC_INFO_MASK))

@iliaal iliaal Oct 27, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

php_splice() also calls zend_hash_destroy(in_hash) while replacing bucket storage, but keeps the GC information intact. A GC-buffered array can have refcount 1, so array_splice()'s parameter separation can leave it unchanged and this new assertion rejects a valid caller state.

Please apply the same GC_TYPE_INFO save/reset/restore used for array_unshift() around that call in php_splice(), and add regression coverage for a GC-buffered array. Keep the HashTable allocation alive; switching to zend_array_destroy() would free it.

@arnaud-lb arnaud-lb Oct 27, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for spotting this. Should be fixed now.

|| GC_TYPE(ht) == IS_NULL /* destroyed by GC */)
&& "HashTables exposed to userland should be destroyed with zend_array_destroy()");

if (ht->nNumUsed) {
if (HT_IS_PACKED(ht)) {
Expand Down
7 changes: 7 additions & 0 deletions ext/standard/array.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3456,7 +3456,11 @@ static void php_splice(HashTable *in_hash, zend_long offset, zend_long length, H
return;
}

uint32_t type_info = GC_TYPE_INFO(in_hash);
GC_TYPE_INFO(in_hash) = GC_ARRAY;

zend_hash_destroy(in_hash);
GC_TYPE_INFO(in_hash) = type_info;

HT_FLAGS(in_hash) = HT_FLAGS(&out_hash);
in_hash->nTableSize = out_hash.nTableSize;
Expand Down Expand Up @@ -3740,8 +3744,11 @@ PHP_FUNCTION(array_unshift)

/* replace HashTable data */
Z_ARRVAL_P(stack)->pDestructor = NULL;
uint32_t type_info = GC_TYPE_INFO(Z_ARRVAL_P(stack));
GC_TYPE_INFO(Z_ARRVAL_P(stack)) = GC_ARRAY;
zend_hash_destroy(Z_ARRVAL_P(stack));

GC_TYPE_INFO(Z_ARRVAL_P(stack)) = type_info;
HT_FLAGS(Z_ARRVAL_P(stack)) = HT_FLAGS(&new_hash);
Z_ARRVAL_P(stack)->nTableSize = new_hash.nTableSize;
Z_ARRVAL_P(stack)->nTableMask = new_hash.nTableMask;
Expand Down
Loading