Everything
php
Learn Community Releases The PHP Project
Home » Project » Newsgroups » php.internals » [patch] Zend/zend_objects_API.c - bug #29980 (segfault while executing __destruct()) »

[patch] Zend/zend_objects_API.c - bug #29980 (segfault while executing __destruct())

php.internals

Antony Dovgal

21 years ago
And the last one, the most questionable patch. ATM ZE2 calls destructor at the end of the request and no matter is there were a fatal error (which should probably stop executing the script). In some cases it leads to nasty segfaults (me and report's author can reproduce it, but others can't. weird..). Some persons (hello, Andrey =)) think that this could be a useful feature, but for me it's just an inconsistency. IMO destructors should not be called after fatal errors, because they can cause even more harm.
-- Wbr, Antony Dovgal aka tony2001 tony2001@phpclub.net || antony@dovgal.com

Andi Gutmans

21 years ago
I think this makes sense because PHP could be in an unstable state. Think EG(exit_status) covers all possible situations? No time to check now how it differs from CG(unclean_shutdown) which is most often used. Andi At 09:57 AM 9/10/2004 +0400, Antony Dovgal wrote:

Antony Dovgal

21 years ago
On Thu, 09 Sep 2004 23:15:08 -0700 Andi Gutmans <andi@zend.com> wrote:
> I think this makes sense because PHP could be in an unstable state. > Think EG(exit_status) covers all possible situations? No time to check > now how it differs from CG(unclean_shutdown) which is most often used.
Well, as I can see zend_error() doesn't change CG(unclean_shutdown), but uses EG(exit_status) instead. And there is a side-effect: using exit_status we allow user to disable destructors calling exit($non_zero_status); Probably, this patch needs some more investigation.
-- Wbr, Antony Dovgal aka tony2001 tony2001@phpclub.net || antony@dovgal.com

Andrey Hristov

21 years ago
Well, if the engine is unstable state why the registered shutdown function is being executed then ? :) Andrey Andi Gutmans wrote:

Antony Dovgal

21 years ago
On Fri, 10 Sep 2004 10:03:48 +0200 Andrey Hristov <php@hristov.com> wrote:
> Well, if the engine is unstable state why the registered shutdown > function is being > executed then ? :)
good question, though =)
-- Wbr, Antony Dovgal aka tony2001 tony2001@phpclub.net || antony@dovgal.com

Andi Gutmans

21 years ago
At 10:03 AM 9/10/2004 +0200, Andrey Hristov wrote:
> Well, if the engine is unstable state why the registered shutdown > function is being >executed then ? :)
Right. I think it might be a problem to do so during an E_ERROR, most definitely in an E_CORE_ERROR.

Curt Zirzow

21 years ago
* Thus wrote Antony Dovgal:
> And the last one, the most questionable patch. > > ATM ZE2 calls destructor at the end of the request and no matter > is there were a fatal error (which should probably stop executing > the script). In some cases it leads to nasty segfaults (me and > report's author can reproduce it, but others can't. weird..).
I'm not sure if this has anything to do with it but the destructor is being called before the constructor actually finishes, I would think the state of the object itself isn't stable. I can stop the segfault happening if I cause the fatal error after the object is created.
> > Some persons (hello, Andrey =)) think that this could be a useful > feature, but for me it's just an inconsistency. IMO destructors > should not be called after fatal errors, because they can cause > even more harm.
Might want to add me to that list :) A use I can see is if the object manages a buffer of some sort and the destructor ensures that it is flushed, a bypass of the destructor would cause the buffer to get lost. Curt
-- First, let me assure you that this is not one of those shady pyramid schemes you've been hearing about. No, sir. Our model is the trapezoid!

Antony Dovgal

21 years ago
On Fri, 10 Sep 2004 16:23:41 +0000 Curt Zirzow <curt@php.net> wrote:
> Might want to add me to that list :) A use I can see is if the > object manages a buffer of some sort and the destructor ensures > that it is flushed, a bypass of the destructor would cause the > buffer to get lost.
Yup. Because it was a *FATAL* error. http://www.php.net/manual/en/ref.errorfunc.php#e-error
-- E_ERROR (integer) - Fatal run-time errors. These indicate errors that can not be recovered from, such as a memory allocation problem. Execution of the script is halted. -- I'm not a person to make this decision, but I think that fatal error should really halt execution as the documentation says. -- Wbr, Antony Dovgal aka tony2001 tony2001@phpclub.net || antony@dovgal.com

Andrey Hristov

21 years ago
Curt Zirzow wrote:
> * Thus wrote Antony Dovgal: > >>And the last one, the most questionable patch. >> >>ATM ZE2 calls destructor at the end of the request and no matter >>is there were a fatal error (which should probably stop executing >>the script). In some cases it leads to nasty segfaults (me and >>report's author can reproduce it, but others can't. weird..). > > > I'm not sure if this has anything to do with it but the destructor > is being called before the constructor actually finishes, I would > think the state of the object itself isn't stable. I can stop the > segfault happening if I cause the fatal error after the object is > created. > > >>Some persons (hello, Andrey =)) think that this could be a useful >>feature, but for me it's just an inconsistency. IMO destructors >>should not be called after fatal errors, because they can cause >>even more harm. > > > Might want to add me to that list :) A use I can see is if the > object manages a buffer of some sort and the destructor ensures > that it is flushed, a bypass of the destructor would cause the > buffer to get lost. >
Well, the system is in unstable state and that means that a flush may fail but a try is better than nothing. Andrey

Stanislav Malyshev

21 years ago
AD>>ATM ZE2 calls destructor at the end of the request and no matter is AD>>there were a fatal error (which should probably stop executing the AD>>script). In some cases it leads to nasty segfaults (me and report's AD>>author can reproduce it, but others can't. weird..). Well, the cause of the faults is as follows: If we are in shutdown, and one of the destructors fails with fatal error, then other destructors for other objects are not called. Thus, their if they hold some objects, destructors for these objects will not be called. Later, when the storage is cleaned with zend_objects_store_free_object_storage(), engine will attempt to call dtors for objects that didn't have their dtors called before. However, on that stage engine is already unable to run PHP code (function and class tables are already cleaned, etc.) - so it crashes. What I would propose is to inhibit calling destructors after shutdown_destructors() was finished.
-- Stanislav Malyshev, Zend Products Engineer stas@zend.com http://www.zend.com/ +972-3-6139665 ext.115

Antony Dovgal

21 years ago
On Mon, 13 Sep 2004 14:02:43 +0300 (IDT) Stanislav Malyshev <stas@zend.com> wrote:
> AD>>ATM ZE2 calls destructor at the end of the request and no matter > AD>is>there were a fatal error (which should probably stop executing > AD>the>script). In some cases it leads to nasty segfaults (me and > AD>report's>author can reproduce it, but others can't. weird..). > > Well, the cause of the faults is as follows: > If we are in shutdown, and one of the destructors fails with fatal > error, then other destructors for other objects are not called. Thus, > their if they hold some objects, destructors for these objects will > not be called. Later, when the storage is cleaned with > zend_objects_store_free_object_storage(), engine will attempt to call > dtors for objects that didn't have their dtors called before. However, > on that stage engine is already unable to run PHP code (function and > class tables are already cleaned, etc.) - so it crashes.
Looks like you're right, but why others can't reproduce this segfault?
> What I would propose is to inhibit calling destructors after > shutdown_destructors() was finished.
Sounds nice: we should not call destructors after they were already called =) I could propose a simple solution: add a global flag, which will indicate that shutdown_destructors() was called, and do appropriate check in zend_objects_store_del_ref(). Comments/objections?
-- Wbr, Antony Dovgal aka tony2001 tony2001@phpclub.net || antony@dovgal.com

Jason Garber

21 years ago
Hello Antony, Maybe this was assumed, but wouldn't this be a per-request flag, rather than a global flag?
-- Best regards, Jason mailto:jason@ionzoft.com Tuesday, September 14, 2004, 10:18:29 AM, you wrote: AD> Sounds nice: we should not call destructors after they were already called =) AD> I could propose a simple solution: add a global flag, which will indicate AD> that shutdown_destructors() was called, and do appropriate check in AD> zend_objects_store_del_ref(). AD> Comments/objections?

Stanislav Malyshev

21 years ago
AD>>> class tables are already cleaned, etc.) - so it crashes. AD>> AD>>Looks like you're right, but why others can't reproduce this segfault? Only idea I have they didn't run it in debug mode. In the debug mode, destroyed memory is overwritten, in the release mode it isn't. So on debug, use-after-free cases drop dead instantly, while on release mode they may very well work in some cases. AD>>Sounds nice: we should not call destructors after they were AD>>already called =) The problem is that if one of the dtors fails, others aren't called. THis is OK (i.e., this is better than the alternatives), but it confuses the engine later on the way of shutdown. AD>>I could propose a simple solution: add a global flag, which will indicate AD>>that shutdown_destructors() was called, and do appropriate check in AD>>zend_objects_store_del_ref(). AD>>Comments/objections? I personally don't like the idea of having yet another hack flag. Moveover, the del_ref is not the only place one may call destructors. What I'd do is: if one of the dtors bails out, we catch it in call_dtors and then mark all the objects as "already destroyed" - so there's no way any dtor could be called after that from any place. This is a bit slower performance-wise, bnut I'm not sure performance is much of concern in situation where the code has failed in shutdown - I don't know any "fast failure" benchmarks ;) After all, it will be not much longer than the regular shutdown anyway - one pass over the store. The advantage of this approach is that it follows the existing protocol and not insterts another flag (read - branch in any function working along the protocol) which leads to potential breakage.
-- Stanislav Malyshev, Zend Products Engineer stas@zend.com http://www.zend.com/ +972-3-6139665 ext.115

Antony Dovgal

21 years ago
On Thu, 16 Sep 2004 16:31:30 +0300 (IDT) Stanislav Malyshev <stas@zend.com> wrote:
> AD>>> class tables are already cleaned, etc.) - so it crashes. > AD>> > AD>>Looks like you're right, but why others can't reproduce this > AD>segfault? > > Only idea I have they didn't run it in debug mode. In the debug mode, > destroyed memory is overwritten, in the release mode it isn't. So on > debug, use-after-free cases drop dead instantly, while on release mode > they may very well work in some cases.
Ok, I got it.
> AD>>Sounds nice: we should not call destructors after they were > AD>>already called =) > > The problem is that if one of the dtors fails, others aren't called. > THis is OK (i.e., this is better than the alternatives), but it > confuses the engine later on the way of shutdown.
Surely I agree.
> AD>>I could propose a simple solution: add a global flag, which will > AD>indicate>that shutdown_destructors() was called, and do appropriate > AD>check in>zend_objects_store_del_ref(). > AD>>Comments/objections? > > I personally don't like the idea of having yet another hack flag. > Moveover, the del_ref is not the only place one may call destructors. > What I'd do is: if one of the dtors bails out, we catch it in > call_dtors and then mark all the objects as "already destroyed" - so > there's no way any dtor could be called after that from any place. > This is a bit slower performance-wise, bnut I'm not sure performance > is much of concern in situation where the code has failed in shutdown > - I don't know any "fast failure" benchmarks ;) After all, it will be > not much longer than the regular shutdown anyway - one pass over the > store. The advantage of this approach is that it follows the existing > protocol and not insterts another flag (read - branch in any function > working along the protocol) which leads to potential breakage.
Sounds quite nice. Will look at it later a bit.
-- Wbr, Antony Dovgal aka tony2001 tony2001@phpclub.net || antony@dovgal.com

Stanislav Malyshev

21 years ago
Attached is the patch I'm thinking of. If nobody objects, I think I'll apply it on Friday. AD>>Sounds quite nice. AD>>Will look at it later a bit. AD>> AD>>
-- Stanislav Malyshev, Zend Products Engineer stas@zend.com http://www.zend.com/ +972-3-6139665 ext.115

Antony Dovgal

21 years ago
On Thu, 16 Sep 2004 16:57:06 +0300 (IDT) Stanislav Malyshev <stas@zend.com> wrote:
> Attached is the patch I'm thinking of. If nobody objects, I think I'll > apply it on Friday.
Yup, that's exactly what I wanted to write in the evening =)
-- Wbr, Antony Dovgal aka tony2001 tony2001@phpclub.net || antony@dovgal.com

Andi Gutmans

21 years ago
Looks good to me. At 04:57 PM 9/16/2004 +0300, Stanislav Malyshev wrote:

Zeev Suraski

21 years ago
Looks good. Zeev At 16:57 16/09/2004, Stanislav Malyshev wrote:

Andi Gutmans

21 years ago
At 04:31 PM 9/16/2004 +0300, Stanislav Malyshev wrote:
>AD>>I could propose a simple solution: add a global flag, which will indicate >AD>>that shutdown_destructors() was called, and do appropriate check in >AD>>zend_objects_store_del_ref(). >AD>>Comments/objections? > >I personally don't like the idea of having yet another hack flag. >Moveover, the del_ref is not the only place one may call destructors. What >I'd do is: if one of the dtors bails out, we catch it in call_dtors and >then mark all the objects as "already destroyed" - so there's no way any >dtor could be called after that from any place. This is a bit slower >performance-wise, bnut I'm not sure performance is much of concern in >situation where the code has failed in shutdown - I don't know any "fast >failure" benchmarks ;) After all, it will be not much longer than the >regular shutdown anyway - one pass over the store. The advantage of this >approach is that it follows the existing protocol and not insterts another >flag (read - branch in any function working along the protocol) which >leads to potential breakage.
Yep, that's exactly what I thought. Andi

Andi Gutmans

21 years ago
At 06:18 PM 9/14/2004 +0400, Antony Dovgal wrote:
>Sounds nice: we should not call destructors after they were already called =) > >I could propose a simple solution: add a global flag, which will indicate >that shutdown_destructors() was called, and do appropriate check in >zend_objects_store_del_ref(). >Comments/objections?
I prefer not to add another if() to this method. I did my best to keep it as slim as possible. We could run over the object store and mark all destructors as already called. That might be the best solution and wouldn't slow down the general case of objects being destroyed during script execution, but if there are lots of global objects it would potentially slow down the shutdown. Andi

Andi Gutmans

21 years ago
At 02:02 PM 9/13/2004 +0300, Stanislav Malyshev wrote:

Andi Gutmans

21 years ago
Yes I agree. At 02:02 PM 9/13/2004 +0300, Stanislav Malyshev wrote: