Bug Fix: #2372

commercebyte · 2017-02-06T23:39:32Z

Corrupted class entries on shutdown when a destructor spawns another object
(C) 2017 CommerceByte Consulting

When zend_objects_store_call_destructors() is called from the shutdown sequence -
it's calling the dtor's for remaining objects one by one in sequence of object handles.
If the dtor spawns one or more objects, and the new objects happen to reuse the old handles -
their dtor's are not called in this cycle.
The dtor's are called later on, when zend_deactivete() kicks in, and the static property lists in the class entries are freed.
This causes "Undefined static property" errors, and/or SIGSEGV.

Solution:
zend_object_store.no_reuse field is added
Set to 0 on initialization, set to 1 on the shutdown sequence.
zend_objects_store_put(zend_object *) checks the no_reuse flag, and never reuses the old handle slots if set.
This way, the dtor's for newly spawned objects are guaranteed to be called in the zend_objects_store_call_destructors() loop.

Corrupted class entries on shutdown when a destructor spawns another object (C) 2017 CommerceByte Consulting When zend_objects_store_call_destructors() is called from the shutdown sequence - it's calling the dtor's for remaining objects one by one in sequence of object handles. If the dtor spawns one or more objects, and the new objects happen to reuse the old handles - their dtor's are not called in this cycle. The dtor's are called later on, when zend_deactivete() kicks in, and the static property lists in the class entries are freed. This causes "Undefined static property" errors, and/or SIGSEGV. Solution: zend_object_store.no_reuse field is added Set to 0 on initialization, set to 1 on the shutdown sequence. zend_objects_store_put(zend_object *) checks the no_reuse flag, and never reuses the old handle slots if set. This way, the dtor's for newly spawned objects are guaranteed to be called in the zend_objects_store_call_destructors() loop.

…buggy behavior, when PHP returns "Undefined static property" error due to class entry corruption. With my fix for bug 74053, both tests return no errors now, I corrected the EXPECTF accordingly [Anybody please advice if I'm wrong?] Also created bug74053.phpt, for the code I mentioned in the bug description

nikic · 2017-02-07T15:47:04Z

Zend/zend_objects_API.h

@@ -45,6 +45,7 @@ typedef struct _zend_objects_store {
 	uint32_t top;
 	uint32_t size;
 	int free_list_head;
+	char no_reuse; /* to be set to true when shutting down, to avoid missing dtor call on objects spawned by another dtor */


This is an ABI break on 32-bit platforms, so this change could only go into master as-is.

commercebyte · 2017-02-07T15:50:50Z

My apologies - let's change to int. I'll push an update in a few minutes -

…

______________ Jim Zubov CommerceByte Consulting President (917) 336-6221 Skype: commercebyte Schedule an Appointment: https://calendly.com/commercebyte

On February 7, 2017 10:47:47 AM EST, Nikita Popov ***@***.***> wrote: nikic commented on this pull request. > @@ -45,6 +45,7 @@ typedef struct _zend_objects_store { uint32_t top; uint32_t size; int free_list_head; + char no_reuse; /* to be set to true when shutting down, to avoid missing dtor call on objects spawned by another dtor */ This is an ABI break on 32-bit platforms, so this change could only go into master as-is. -- You are receiving this because you authored the thread. Reply to this email directly or view it on GitHub: #2372 (review)

nikic · 2017-02-07T15:51:03Z

Apart from the ABI issue, this approach looks fine to me.

@dstogov What do you think about this change?

nikic · 2017-02-07T15:53:25Z

My apologies - let's change to int.
I'll push an update in a few minutes

Changing it to int won't help here: The issue is not the used type, the issue is that on 32-bit this change increases the size of the zend_object_store struct, which is embedded in the executor globals. This will shift all globals coming after it down by 4 bytes, breaking any shared objects using those members.

commercebyte · 2017-02-07T16:00:01Z

Then I can suggest to move no_reuse out of zend_objects_store, and make it a separate global var. Looks like objects store is used only as a single global instance, so this approach should work. Any thoughts?

…

______________ Jim Zubov CommerceByte Consulting President (917) 336-6221 Skype: commercebyte Schedule an Appointment: https://calendly.com/commercebyte

On February 7, 2017 10:53:53 AM EST, Nikita Popov ***@***.***> wrote: > My apologies - let's change to int. > I'll push an update in a few minutes Changing it to int won't help here: The issue is not the used type, the issue is that on 32-bit this change increases the size of the zend_object_store struct, which is embedded in the executor globals. This will shift all globals coming after it down by 4 bytes, breaking any shared objects using those members. -- You are receiving this because you authored the thread. Reply to this email directly or view it on GitHub: #2372 (comment)

nikic · 2017-02-08T00:05:57Z

Then I can suggest to move no_reuse out of zend_objects_store, and make it a separate global var. Looks like objects store is used only as a single global instance, so this approach should work. Any thoughts?

Yes, this should work, if you declare the variable as ZEND_TLS (the object store is per-thread).

…object_store_no_reuse, to avoid alignment issues

commercebyte · 2017-02-08T02:27:52Z

Redefined as a global. Just a thought - it might've been better to have a public global execute state var - to be able to distinguish whether it's a normal execution or a shutdown. I don't see anything like that in the code.

…

______________ Jim Zubov CommerceByte Consulting President (917) 336-6221 Skype: commercebyte Schedule an Appointment: https://calendly.com/commercebyte

On February 7, 2017 7:06:21 PM EST, Nikita Popov ***@***.***> wrote: > Then I can suggest to move no_reuse out of zend_objects_store, and make it a separate global var. Looks like objects store is used only as a single global instance, so this approach should work. Any thoughts? Yes, this should work, if you declare the variable as `ZEND_TLS` (the object store is per-thread). -- You are receiving this because you authored the thread. Reply to this email directly or view it on GitHub: #2372 (comment)

dstogov · 2017-02-08T07:38:24Z

This fixes the issue, but looks like a hack. Let me think about a different solution.

…

________________________________ From: Nikita Popov <notifications@github.com> Sent: Tuesday, February 7, 2017 6:51:28 PM To: php/php-src Cc: Dmitry Stogov; Mention Subject: Re: [php/php-src] Bug Fix: (#2372) Apart from the ABI issue, this approach looks fine to me. @dstogov<https://github.com/dstogov> What do you think about this change? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub<#2372 (comment)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/ACZM0sKcNUNWv7ogRjYmFolUcG4hbagmks5raJMAgaJpZM4L43PK>.

commercebyte · 2017-02-08T12:34:09Z

I agree, looks somewhat like a hack. See my latest suggestion on the thread for another option - an execution state flag.

…

______________ Jim Zubov CommerceByte Consulting President (917) 336-6221 Skype: commercebyte Schedule an Appointment: https://calendly.com/commercebyte

On February 8, 2017 2:38:48 AM EST, Dmitry Stogov ***@***.***> wrote: This fixes the issue, but looks like a hack. Let me think about a different solution. ________________________________ From: Nikita Popov ***@***.***> Sent: Tuesday, February 7, 2017 6:51:28 PM To: php/php-src Cc: Dmitry Stogov; Mention Subject: Re: [php/php-src] Bug Fix: (#2372) Apart from the ABI issue, this approach looks fine to me. @dstogov<https://github.com/dstogov> What do you think about this change? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub<#2372 (comment)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/ACZM0sKcNUNWv7ogRjYmFolUcG4hbagmks5raJMAgaJpZM4L43PK>. -- You are receiving this because you authored the thread. Reply to this email directly or view it on GitHub: #2372 (comment)

dstogov · 2017-02-09T08:10:27Z

Right. Currently we have only EG(active), but we may easily add another 8-bit EG(state) or/and EG(flags).

EG_FLAGS_IN_SHUTDOWN - is set when PHP is in shutdown state

krakjoe added the Feature label Feb 7, 2017

krakjoe requested a review from nikic February 7, 2017 05:48

nikic reviewed Feb 7, 2017

View reviewed changes

Alignment fix, as per @nikic

a6acffe

newly added zend_object_store.no_reuse is redefined as a global zend_…

f9afc6a

…object_store_no_reuse, to avoid alignment issues

Added EG(flags) - executor global flags

1b1399c

EG_FLAGS_IN_SHUTDOWN - is set when PHP is in shutdown state

php-pulls merged commit 1b1399c into php:master Feb 10, 2017

bwoebi mentioned this pull request Apr 3, 2018

Fatal error during shutdown amphp/amp#212

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Bug Fix: #2372

Bug Fix: #2372

Uh oh!

commercebyte commented Feb 6, 2017

Uh oh!

nikic Feb 7, 2017

Uh oh!

commercebyte commented Feb 7, 2017 via email

Uh oh!

nikic commented Feb 7, 2017

Uh oh!

nikic commented Feb 7, 2017

Uh oh!

commercebyte commented Feb 7, 2017 via email

Uh oh!

nikic commented Feb 8, 2017

Uh oh!

commercebyte commented Feb 8, 2017 via email

Uh oh!

dstogov commented Feb 8, 2017 via email

Uh oh!

commercebyte commented Feb 8, 2017 via email

Uh oh!

dstogov commented Feb 9, 2017

Uh oh!

Uh oh!

Bug Fix: #2372

Bug Fix: #2372

Uh oh!

Conversation

commercebyte commented Feb 6, 2017

Uh oh!

nikic Feb 7, 2017

Choose a reason for hiding this comment

Uh oh!

commercebyte commented Feb 7, 2017 via email

Uh oh!

nikic commented Feb 7, 2017

Uh oh!

nikic commented Feb 7, 2017

Uh oh!

commercebyte commented Feb 7, 2017 via email

Uh oh!

nikic commented Feb 8, 2017

Uh oh!

commercebyte commented Feb 8, 2017 via email

Uh oh!

dstogov commented Feb 8, 2017 via email

Uh oh!

commercebyte commented Feb 8, 2017 via email

Uh oh!

dstogov commented Feb 9, 2017

Uh oh!

Uh oh!