Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:67743
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: pass (pb1.pair.com: domain gmail.com designates 209.85.217.181 as permitted sender)
MIME-Version: 1.0
Sender: laruence@gmail.com
In-Reply-To: <CAAyV7nEx32ni3J0dOQprJCXd=4D06XwS0-Ro811OtqmPeEyYJg@mail.gmail.com>
References: <CAAyV7nEx32ni3J0dOQprJCXd=4D06XwS0-Ro811OtqmPeEyYJg@mail.gmail.com>
Date: Thu, 20 Jun 2013 18:12:33 +0800
Message-ID: <CABBJUpcz3g6sWZR_K+p6fe0_nP7YZqW1XgZ54ktgA1KgD6xchg@mail.gmail.com>
To: Anthony Ferrara <ircmaxell@gmail.com>
Cc: "internals@lists.php.net" <internals@lists.php.net>
Content-Type: text/plain; charset=UTF-8
Subject: Re: [PHP-DEV] Disabling the GC during shutdown
From: laruence@php.net (Laruence)

On Thu, Jun 20, 2013 at 1:46 AM, Anthony Ferrara <ircmaxell@gmail.com> wrote:
> 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


>
> 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/