Delaying destructors and GC

[arnaud-lb]

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:

• 3d2b175
• 6f38acf
• https://bugs.php.net/bug.php?id=71020
• https://bugs.php.net/bug.php?id=73423
• heap-buffer-overflow at zval_undefined_cv #10168
• Use after free in SplDoublyLinkedList #16464
• Use after free in SplFixedArray #16478
• Use after free in SplObjectStorage #16479
• UAF when unsetting ArrayObject element #16646
• zend_array_try_init() with dtor can cause engine UAF #17162
• Use-after-free with extract() and EXTR_REFS #18209
• GC Reference Counting Assertion Failure via Object Destruction and Invalid Assignment #19999
• Heap-use-after-free in zend_array_count at Zend/zend_hash.c #20477

#19787 delays GC runs, but not destructors.

[iluuu1994]

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.

[bwoebi]

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.

[arnaud-lb]

@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.

[iluuu1994]

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.

[arnaud-lb]

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.

[arnaud-lb]

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.

[bwoebi]

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.

[bwoebi]

@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.