Skip to content

Add destructor-hazard sections to defer reentrant destructors#22697

Open
iliaal wants to merge 1 commit into
php:masterfrom
iliaal:proto/dtor-hazard-pin
Open

Add destructor-hazard sections to defer reentrant destructors#22697
iliaal wants to merge 1 commit into
php:masterfrom
iliaal:proto/dtor-hazard-pin

Conversation

@iliaal

@iliaal iliaal commented Jul 11, 2026

Copy link
Copy Markdown
Contributor

This is the destructor slice of the reentrancy work tracked in GH-20001, building on the error-handler deferral in #22515. It adds ZEND_DTOR_HAZARD_BEGIN/END sections and a deferral gate in zend_objects_store_del: while a section is open, an object that reaches refcount zero and has a destructor is pinned and its destructor deferred until the section closes. Applied to three reentrancy use-after-frees where an internal C loop drops a refcount while holding a raw pointer a synchronous __destruct could free: concat (GH-20477), zend_hash_clean (GH-22061), and unserialize teardown (GH-21824).

How this differs from arnaud-lb#31: that prototype deferred every destructor through a single global EG(delayed_effects) queue, pinned the dying object with GC_SET_REFCOUNT(obj, 1), and flushed at the VM function-leave helper. It left three problems open: throwing destructors reordered against try/finally and catch (6 XFAILed tests), 67 tests needed a dummy call to create a flush point, and GC reconciliation between the RC=1 pin and cycle collection stayed unresolved. This mechanism is scoped instead. BEGIN/END bracket only the internal loops that hold the raw pointer, so no user code runs inside a window. That drops the try/finally reordering question, drops the timing change (flush is at section exit, not an arbitrary safepoint), uses GC_ADDREF instead of GC_SET_REFCOUNT so it reuses the existing store_del invariants, and closes the GC hole by having gc_possible_root_when_full skip synchronous collection while a section is open.

Performance: the store_del gate is one branch guarded by has_dtor, so destructor-less objects are untouched. Sections arm only on cold paths. Release callgrind over 2M concats shows +1.2 instructions per concat.

P.S. As a follow-up, this can retire hand-rolled per-site guards for the same class: SplFixedArray's cached_resize field exists only to defer a re-entrant setSize a destructor triggers during the element-free, so wrapping that phase in a hazard section would let the whole cached_resize machinery be removed. I will propose that separately once this lands.

An internal C operation that drops a refcount while holding a raw pointer
into a structure a synchronous __destruct could free is a use-after-free.
Add ZEND_DTOR_HAZARD_BEGIN/END sections and a deferral gate in
zend_objects_store_del: while a section is open, an object that reaches
refcount zero and has a destructor is pinned with GC_ADDREF and its
destructor deferred until the section closes. gc_possible_root_when_full
skips synchronous collection while a section is open. Apply the sections
to concat (phpGH-20477), zend_hash_clean (phpGH-22061), and unserialize
teardown (phpGH-21824).

Fixes phpGH-20477
Fixes phpGH-22061
Fixes phpGH-21824
@iluuu1994

Copy link
Copy Markdown
Member

This doesn't look like a general solution. Many paths in the VM currently have awkward handling for destructors. Basically, search for "garbage" in Zend/zend_vm_def.h. With Arnaud's solution, we can completely get rid of this handling and forget about this issue entirely. The same will go for error handlers.

@iliaal

iliaal commented Jul 11, 2026

Copy link
Copy Markdown
Contributor Author

For error handlers there is already a general solution in #22515 that afaik covers all known issues and cases with essentially 0 performance impact. This is 1st stab that while not strictly general provides isolated, essentially performance safe fix for 3 issues and simplification/unification path for one-off fixes in SPL.

Still lookin at broader fix, but this is an interim working solution

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