Delaying destructors and GC

Description

Due to destructors and the cycle collector, zval_ptr_dtor() may have more effects than just freeing the given zval. This is the cause of bugs and workarounds.

These bugs and workarounds could be avoided by delaying destructors and GC runs until a safepoint is reached. If we use vm interrupts as safepoints, this would delay destructors until the next branch/loop or function call (this should be defined more precisely), which seems reasonable.

I'm creating this ticket to aggregate issues that would not exist with delayed destructors and GC runs:

#19787 delays GC runs, but not destructors.

Closely related issues are set_error_handler() (attempted fix at GH-12805), and __toString() (attempted fix at GH-15961). And also #15961 (comment) apparently, which I forgot about.

It's worth noting that these are all very artificial issues. I believe we have yet to encounter a non-fuzzer report for one of them. To quote @nielsdos:

To be clear, I like this idea and perhaps we should commit this; but I'm starting to doubt adding all these tiny things all over the place for specific cases is the way to go. One one hand we should fix as many as possible of these, on the other hand it's all subtle stuff and complexity increases.

I mostly share this sentiment now. We have fixed many of these issues, and yet more and more pop up, especially as more people are fuzzing than ever. The fix usually negatively affects performance, simplicity/readability, or both. I feel like, even if we can't solve this in a general way, we should not commit further fixes regardless. For fuzzers, it should be pretty simple to add set_error_handler(), __destruct() and __toString() to the ignore list.

@iluuu1994

vm_interrupt is unnecessarily late, but maybe an approach like #12805 (basically updating the EX(opline) temporarily to jump to a dedicated freeing opcode) could work easily (collect objects onto a persistent bare NULL-terminated array).
Then invoke the object destructor call at the first possible safe-point. This should be effectively nearly invisible. (EDIT: Ilija was just too fast to write :-D)

@iluuu1994 I see you mention set_error_handler - is there any actual reason you did not pursue that PR anymore (JIT probably)? Having these foundations probably makes it easier to implement the solution for destructors too.

And __toString() should be burned with fire. At least that one has the problem that it requires an immediately available return value, which is quite problematic and not sure if it can be fixed at all in a general way.

@iluuu1994

@iluuu1994 I was about to create a similar issue for set_error_handler(), referring to earlier work :) Edit: #20018

To be clear, I like this idea and perhaps we should commit this; but I'm starting to doubt adding all these tiny things all over the place for specific cases is the way to go. One one hand we should fix as many as possible of these, on the other hand it's all subtle stuff and complexity increases.

I mostly share this sentiment now. We have fixed many of these issues, and yet more and more pop up, especially as more people are fuzzing than ever. The fix usually negatively affects performance, simplicity/readability, or both. I feel like, even if we can't solve this in a general way, we should not commit further fixes regardless. For fuzzers, it should be pretty simple to add set_error_handler(), __destruct() and __toString() to the ignore list.

I agree with the observations, but I don't think we should start ignoring these. This would be equivalent to giving up on considering PHP a memory safe language. And at least some of the reports above are non-fuzzer ones.

But I think that we can fix this in a more general way, hence this issue.

Okay. I'm happy to re-investigate general solutions. That would allow us to revert many of the earlier fixes.

To be clear, I'm not against fixing real issues that are reported by real people, just the ones that are unlikely to occur in real code.

@iluuu1994

@iluuu1994 I see you mention set_error_handler - is there any actual reason you did not pursue that PR anymore (JIT probably)? Having these foundations probably makes it easier to implement the solution for destructors too.

@bwoebi Yes, it was primarily Dmitry's comment that suggested this solution wouldn't work for the JIT. It seems like it may be possible to change exception handling to check for EG(exception) rather than EX(opline) == &EG(exception_op). Though I suspect it was implemented this way for performance reasons.

And __toString() should be burned with fire. At least that one has the problem that it requires an immediately available return value, which is quite problematic and not sure if it can be fixed at all in a general way.

That's very unlikely to happen. The question then becomes: Are we contempt solving only 3 of the 4 cases, even if that means PHP remains a "memory unsafe" language, even though I don't agree with this premise to begin with.

The __toString() issue seems to have a narrower impact than the others, and may be fixable with a less general solution. For ASSIGN_OBJ_OP, ASSIGN_OBJ_REF I think it can be fixed by introducing a new write_property_op handler: #15961 (comment). For ASSIGN_DIM_OP it should be fixable with a ADDREF/DELREF.

I will attempt a fix for the toString issue.

If #12805 is ultimately not possible, I will also attempt a vm_interrupt-based approach for the other issues.

EX(opline) == &EG(exception_op)

There's no requirement to actually check against ZEND_HANDLE_EXCEPTION specifically. You could also introduce a ZEND_HANDLE_OUT_OF_ORDER opcode which you'd put into EG(exception_op) , which then actually dispatches to ZEND_HANDLE_EXCEPTION or ZEND_HANDLE_DELAYED_ERROR (or possibly ZEND_HANDLE_OBJECT_DESTRUCTORS) according to a global flag, keeping the happy path at the same cost than it's today in JIT.

@iluuu1994

@iluuu1994 I know that we're not going to remove __toString(). Sadly.
@arnaud-lb But indeed, the scope of __toString() is much more limited, and called at much fewer critical places. This is solvable without a generic solution. Sounds good to me.

My attempt at fixing the __toString() issues:

Fix GH-15938: Hold onto the container after a get_property_ptr_ptr() call #20246
Fix GH-20318, GH-20316: Handle concurrent mutations during reference assignment #20254
Fix GH-20319: Prevent ASSIGN_OP / ASSIGN_DIM_OP lvalue from being invalidated during assignment #20326

A few more cases for this list. Each of these setters releases the old callback or value before it stores the new one, so a destructor that sets it again frees memory that is still in use. All give a heap-use-after-free on a debug ASan build of PHP-8.4, and master has the same code.

<?php
$ch = curl_init();
class Cb {
    public function __invoke() { return 0; }
    public function __destruct() {
        global $ch;
        curl_setopt($ch, CURLOPT_WRITEFUNCTION, fn() => 0);
    }
}
curl_setopt($ch, CURLOPT_WRITEFUNCTION, new Cb);
curl_setopt($ch, CURLOPT_WRITEFUNCTION, null);

The same pattern crashes in:

curl_setopt() with any callback option, and with CURLOPT_PRIVATE
curl_reset(), when a callback's destructor sets its option again
curl_multi_setopt() with CURLMOPT_PUSHFUNCTION
SQLite3::setAuthorizer()
libxml_set_external_entity_loader() (ext/libxml: bailout during external entity loader destruction leaves stale FCC and can cause use-after-free #24763, Fix #24763: Clear libxml entity loader before destroying FCC #24764)
xml_set_*_handler() and xml_set_object()
Pdo\Pgsql::setNoticeCallback()

Navigation Menu

Repository navigation

Delaying destructors and GC #20001

Description

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Delaying destructors and GC #20001

Description

Description

Activity

iluuu1994 commented on Sep 30, 2025

bwoebi commented on Sep 30, 2025

arnaud-lb commented on Sep 30, 2025

iluuu1994 commented on Sep 30, 2025

iluuu1994 commented on Oct 1, 2025

arnaud-lb commented on Oct 2, 2025

arnaud-lb commented on Oct 2, 2025

bwoebi commented on Oct 2, 2025

bwoebi commented on Oct 2, 2025

arnaud-lb commented on Oct 29, 2025

binaryfire commented on Oct 28, 2026

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions