Disabling the GC during shutdown

Johannes,

On Wed, Jun 19, 2013 at 2:43 PM, Johannes Schlüter
johannes@schlueters.dewrote:

1. Technically, all we need to do is force GC_G(gc_enabled) = 0 in
   shutdown. But we could also use zend_alter_ini_entry which has the same
   effect. The question comes is there any reason to go through the overhead
   of altering the ini entry instead of the global directly? We do access
   the
   global directly in other areas (but it's typically only set via ini)...

If possible try to prevent users from shooting their feet by reenabling
via ini_set() or gc_collect_cycles() or such.

Yeah, for that very reason I pushed it after the destructor functions. It
could technically go down a step further below the output buffer as well,
but I don't thing that's too necessary...

Additionally, I have switched it to use the ini behavior setting...

Additionally, considering that this does solve a segfault, is it worth
nominating this for 5.3? Or is it too risky (or something else I'm
missing)...

See my previous mail to the list ...

That's why I actually called out 5.3 specifically. I'm not sold either way,
but I can see why some would want to get this into the last release... And
I could see why not...

Thanks,

Anthony

All,

We were discussing a range of bugs today with the garbage collector. For
example: https://bugs.php.net/bug.php?id=64827

After quite a bit of digging, it appears what's happening is that the
garbage collector is running during the shutdown of PHP. So the destructors
are fired, and the variables are being destroyed when the GC run happens.
This means that the GC, while walking the variable tree runs into a
partially destructed object (where an entry of the hash table has already
been freed). This causes a segfault, and fun ensues.
Hey:

Sorry,  but I don't this this explain is right.

if there is more than one refcount to a zval, then it should never be freed

and if a zval is freed, then it must also be removed from the gc roots.

according to your explain,  the gc segfault while walking through

a hashtable of a object.

but that doesn't make any sense, since if it segfault in walking,

then it should also segfault when trying to free the hash table later
while dtor the object.

disable GC in shutdown is okey for me. but that is just try to

cover the bug somewhere in the refcount handler.. not the right fix.

thanks

And actually, I prefer not doing this(disable gc in shutdown) for now,
because maybe someday we can get a simple reproduce script(as we all
have see, it is very rare).

then the segfault will be fixed, in the right way.

thanks

Under normal conditions (not during shutdown), this does not appear to be
an issue, because the zval is destructed prior to the object destruction.
This means that there should never be a case where the GC hits a partially
freed object during normal execution.

From what I can see, there are two possible fixes. The first would be to
change how object destruction works in the first place, to tie the variable
into the destruction process (basically refactor the object delref API to
also accept the current zval). That way the part of the code that makes the
decision to nuke the object can nuke the zval first (and hence prevent this
condition). However, this is a major API change and would effect a lot of
extensions that are using or tieing into this hook.

The other option would be to simply disable the GC on shutdown. Considering
all of the variables are going to be thrown away anyway, having the GC run
during shutdown seems a bit wasteful anyway. So if we can kill two birds
with one stone, why not?

I've prepared a basic patch:
https://github.com/ircmaxell/php-src/compare/gc_deactivate_on_shutdown

I did confirm with odoucet (one of the original reporters) that this does
clear up his issue: https://gist.github.com/odoucet/5796378 (along with
trying a bunch of other things).

There are a few out standing questions though:

1. Technically, all we need to do is force GC_G(gc_enabled) = 0 in
   shutdown. But we could also use zend_alter_ini_entry which has the same
   effect. The question comes is there any reason to go through the overhead
   of altering the ini entry instead of the global directly? We do access the
   global directly in other areas (but it's typically only set via ini)...

2. I put it in php_request_shutdown() after deactivate_ticks, but before it
   calls shutdown functions. I could see it being moved after the shutdown
   function call, but I'm not sure if it's worth it (either way). thoughts?

3. Can anyone think of a reason we'd want the GC enabled during the request
   shutdown? I can't think of any...

Additionally, considering that this does solve a segfault, is it worth
nominating this for 5.3? Or is it too risky (or something else I'm
missing)...

Thanks,

Anthony

--
Laruence Xinchen Hui
http://www.laruence.com/

--
Laruence Xinchen Hui
http://www.laruence.com/

Laruence,


Sorry, but I don't this this explain is right.
>
> if there is more than one refcount to a zval, then it should never be
> freed
>
> and if a zval is freed, then it must also be removed from the gc roots.
>

The point here is that the GC is run *while* the zval is being freed.

Check out the backtrace here: https://bugs.php.net/bug.php?id=64827 ,
specifically zval pointer 0x272afb8

It appears 4 times recursively being passed into
zend_objects_free_object_storage before the GC is triggered and it
segfaults.


> according to your explain, the gc segfault while walking through
> a hashtable of a object.
>

Yes, that is what's happening here. zval_mark_grey() is trying to walk
through the object's hash table, but the first bucket is already freed, so
when it tries to access it bad things happen.


> but that doesn't make any sense, since if it segfault in walking,
> then it should also segfault when trying to free the hash table later
> while dtor the object.
>

That's the point. The dtor is already happening on that object when the GC
tries to run over it again.


> disable GC in shutdown is okey for me. but that is just try to
> cover the bug somewhere in the refcount handler.. not the right fix.
>

Looking back through, we still have the problem where we can't null out the
zval before destructing the object (like we do with arrays) to prevent
this. That's why I suggested one alternative would be to modify
zend_objects_store_del_ref_by_handle_ex to also accept the zval, so it can
be nulled if the object is going to be freed.

However, we could set the object's bucket to invalid in the delref handler
before we call free_storage. And then modify the GC to check for a valid
bucket... I'll try that patch today to see if it solves the original issue
(as it shouldn't result in an API change either)...

Thanks for the thoughts...

Anthony