Fix GH-10026: Garbage collection stops after exception in SoapClient

[nielsdos]

Closes GH-10026

In some places where zend_bailout() is used, PHP recovers and normal execution of the script can continue. This is problematic because zend_bailout() protects the GC; causing garbage not to be collected in the future. This PR introduces a new variant of zend_bailout(): zend_bailout_without_gc_protect(). We can use it in places where we know we can recover and GC must still be enabled.

[iluuu1994]

This sounds sensible to me. TBH I don't know why the gc_protect() is necessary. I'm not sure if it's just there to improve performance on fatal shutdowns or to protect from some kind of memory corruption, tests pass without it.

@dstogov Does this look ok to you?

[iluuu1994]

Maybe we could pick a more generic name, like _zend_nonfatal_bailout to make it more clear which is to be used.

[dstogov]

I don't have a strong opinion about this PR yet.

Another possible approach is to remember and restore the original value of gc_protected() in the "safe" catch places.

Note that zend_bailuot() also sets CG(unclean_shutdown) that may prevent automatic GC and "safe" destruction of global symbol table. We may also keep it in "safe" places.

BTW I can't be sure when zend_bailout() and zend_catch are safe. It's possible to prove safety only if we know both source and destination of longjmp().

I don't like zend_bailout_without_gc_protect() name. I would prefer zend_safe/clean_bailout or zend_bailout_safe/clean.

[iluuu1994]

@dstogov Can you explain why gc_protect(1); is there in the first place? Is it to avoid unnecessary additions to the GC buffer that will never be cleaned up? We can document this in a comment. This will make it easier to understand whether using the new method that doesn't protect the GC is safe to use.

[dstogov]

Note that gc_bailout() wasn't designed to be caught and continue normal execution

• CG(unclean_shutdown) - disabled GC run at shut-down
• gc_protect() - disabled future GC root collection

[arnaud-lb]

Note that [zend]_bailout() wasn't designed to be caught and continue normal execution

We could try to stop doing this in master.

From my understanding, SOAP leverages zend_catch to convert fatal errors into exceptions. We could do it the other way around.

Unfortunately, SOAP does that for all errors, not only SOAP ones, and it will not be possible to stop using zend_catch for these. Maybe we should stop catching these, as this will result in crashes or memory leaks. Also, fatal errors are less frequent now that many have been replaced by exceptions.

[dstogov]

Note that [zend]_bailout() wasn't designed to be caught and continue normal execution

We could try to stop doing this in master.

From my understanding, SOAP leverages zend_catch to convert fatal errors into exceptions. We could do it the other way around.

It would be great to have the other way. I didn't have a better idea.

Unfortunately, SOAP does that for all errors, not only SOAP ones

This is done on purpose. SOAP server talks to SOAP clients and in case of PHP fatal errors, it should respond in a way that might be understand by clients.

, and it will not be possible to stop using zend_catch for these. Maybe we should stop catching these, as this will result in crashes or memory leaks. Also, fatal errors are less frequent now that many have been replaced by exceptions.

Fatal errors in SOAP client may be replaced by exceptions, but this will require manual error recovery along the C call stack (releasing all the allocated data).

[arnaud-lb]

This is done on purpose. SOAP server talks to SOAP clients and in case of PHP fatal errors, it should respond in a way that might be understand by clients.

I see, this didn't occur to me. This appears to be done in a safe way in SOAPServer, though, as this always bailouts in the end. soap_error_handler responds to the client, and then bailouts.

Fatal errors in SOAP client may be replaced by exceptions, but this will require manual error recovery along the C call stack (releasing all the allocated data).

👍