Skip to content

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

Merged
arnaud-lb merged 2 commits into
php:masterfrom
arnaud-lb:hash-destroy-gc
Oct 27, 2026
Merged

arnaud-lb merged 2 commits into
php:masterfrom
arnaud-lb:hash-destroy-gc

Conversation

@arnaud-lb

@arnaud-lb arnaud-lb commented Oct 26, 2026 •

Copy link
Copy Markdown
Member

Related: f963484

MACOS failure unrelated

@iliaal iliaal left a comment

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.

Please cover the array_splice() storage-replacement path as well.

Comment thread Zend/zend_hash.c
{
IS_CONSISTENT(ht);
HT_ASSERT(ht, GC_REFCOUNT(ht) <= 1);
ZEND_ASSERT(((GC_TYPE(ht) == IS_ARRAY && !(GC_TYPE_INFO(ht) & GC_INFO_MASK))

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.

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.

@iliaal iliaal left a comment

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.

Thanks for the fix

@arnaud-lb
arnaud-lb merged commit 947c556 into php:master Oct 27, 2026
11 of 18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants