[GIT-PULLS] [php-src] PR #22697: Add destructor-hazard sections to defer reentrant destructors
[email protected] (iliaal)
| Newsgroups | php.git-pulls |
|---|---|
| Message-ID | <[email protected]> |
Pull Request: https://github.com/php/php-src/pull/22697 Author: 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/php-src#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.