Add destructor-hazard sections to defer reentrant destructors

[iliaal]

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.

[iluuu1994]

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]

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

[iluuu1994]

I'm really struggling to see how GH-22515 is different form Arnaud's approach. He also implemented his solution through interrupts. Also note that this is intentionally in one PR specifically because we are planning to propose the behavioral change, namely the delaying of destructors and error handlers, as an RFC.

[iliaal]

Right, the transport is the same, a queue drained at vm_interrupt safepoints, and #22515's HANDLE_EXCEPTION drain is adapted from Arnaud's branch. The difference is the gate, not the mechanism.

#22515 defers only diagnostics raised at user-code oplines, the class where the VM holds raw pointers into live operands, and it excludes the call/declare/include opcodes; E_USER_ERROR and E_RECOVERABLE_ERROR never defer; anything raised from an internal function keeps the synchronous handler call. #31 queues any matching diagnostic raised with a live frame, internal functions included (E_NO_DELAY exists, but only its own replay sites set it). That wider gate is where the RFC-sized surface comes from: set_error_handler($delay), PromotedErrorException, promotion precedence.

Flush density differs the same way. #22515 keeps the function-entry interrupt check and flushes at the first safepoint after the raising opcode completes (back-edges, leave, post-call, exception handling, script end), and it postpones the flush past an in-flight call in both the interpreter and the JIT, which is what fixes the CALL-VM cleanup_unfinished_calls fault (gh18262) on Windows/macOS. #31 removes the entry check in all VM kinds and its ZEND_RETURN flush is #if 0'd, so a top-level raise can carry to script end; the 67 tests adding a (function() {})(); flush point and the 48 opting out with delay: false compensate for that. At tip it also pins opcache.jit=0 in four tests, and its five XFAILs are all throwing-destructor/try-finally; #22515 runs under function JIT, tracing JIT and the CALL VM with no pins and no XFAILs.

Both agree on the core semantic: the raising opcode always completes before any handler runs. #22515 is the subset with the smallest observable delta that still fixes the crash class: no new API, no new exception class, and its 75 test-expectation updates are interleave shifts or corrections where a handler used to see mid-opcode state. What's left for the RFC is exactly #31's remainder: internal-function raises, destructors, and the opt-out API. On destructors I agree the general mechanism is the goal and I'm working on it; this is an interim guard for three live UAFs, not a competing end state. If the RFC lands delay-everything, both fold into it.

[TimWolla]

Please no more new macros.

[iluuu1994]

This response looks like AI. If so, please put the relevant part in quotes and disclose your usage. I think that's something we should be able to expect from members. I don't want to live in a dystopian world where we can't even trust the words from our colleagues.

and #22515 HANDLE_EXCEPTION drain is adapted from Arnaud's branch

Then why not give attribution? Why not ask Arnaud to collaborate and share ideas to minimize backwards incompatibilities, rather than proposing effectively the same approach with his name fully erased?

[iluuu1994]

Fat fingers. Didn't intend to close.

[iliaal]

It literarily says that part from adopted from Arnauld's patch, how much more clear can it be? Even #22515 credits Arnauld's work, so I am not sure where the comment about missing attribution comment is coming from?

This is a narrow, error-handlers-only take on https://github.com/php/php-src/issues/20018. https://github.com/arnaud-lb/php-src/pull/31 is the broader reference

In fact in #22515 I specifically offered to Arnauld "If you'd rather land the full delayed-effects design in one go, I can close this or fold the crash fixes into #31."

So, I genuinely find your comment regarding attribution and co-operation well out place here.

Not to mention that THIS particular PR is an interim (or perhaps permanent) performance neutral fix for 3 issues. And little nothing from the other solutions, and in fact clearly outlines how it is more narrow and specific in scope.

I think I outlined in the previous post (formatted with the help of LLM) how the approach is different, while it has similarities, there are absolutely clear differences in terms of the solution and design choices.