Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:88025
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: pass (pb1.pair.com: domain gmail.com designates 209.85.160.179 as permitted sender)
MIME-Version: 1.0
Sender: yohgaki@gmail.com
In-Reply-To: <55E7D2E5.50104@gmail.com>
References: <55E610C1.1000203@gmail.com> <CAGa2bXa41Jsh4vfNF+r3Mj2mLqPjT-UcgNgGzQ2U-d6MhwoMNw@mail.gmail.com>
 <CAGa2bXZtigz0dPe67HQJQnfjfnSNbnXP6m9Ut80BtXwF-8FUnA@mail.gmail.com> <55E7D2E5.50104@gmail.com>
Date: Thu, 3 Sep 2015 17:20:36 +0900
Message-ID: <CAGa2bXYiEZMRR6uB+vwUsScru1ft8AG3xuX2p9e_9CqGuFqoHQ@mail.gmail.com>
To: Stanislav Malyshev <smalyshev@gmail.com>
Cc: PHP Internals <internals@lists.php.net>
Content-Type: text/plain; charset=UTF-8
Subject: Re: [PHP-DEV] Heads up: merging security patches to 7
From: yohgaki@ohgaki.net (Yasuo Ohgaki)

Hi Stas,

On Thu, Sep 3, 2015 at 1:56 PM, Stanislav Malyshev <smalyshev@gmail.com> wrote:
>> I see number of var_push_dtor() to fix unserialization.
>> var_push_dtor() or var_push_dtor_no_addref() is required always when
>> php_var_unserialize() is failed.
>> Am I correct?
>
> Not necessarily. Basically, what happens is that when you do
> php_var_unserialize() the value you unserialize gets a slot in the
> reference table. So if you are destroying it and some serialization part
> later tries to access it - boom! That's where var_push_* comes in. The
> first one adds refcount so even if everybody drops references to it the
> value still survives. The second is used when we know no code uses this
> value, so no need to add refcount - only thing that should keep it alive
> is that it's still in the refs table (except for the case where it is
> taken from the table by r: or R: - then its refcount is bumped).
>
> Now the failure is tricky case - generally after the failure we need to
> bail out ASAP, but that failure may not be the final failure -
> serializes can be nested. So we may want to keep the values around even
> if our unserialize fails, because encompassing serialize may still
> reference that variable, since we may have already put it in the refs
> table. We could make serialize failure a fatal error, but that'd be
> pretty big BC break (exception wouldn't work, we'd need full blown fatal
> E_ERROR).
>
> In general, the idea is that anything that was put in reference table
> (namely, anything that passed through php_var_unserialize()) should be
> kept alive until the end of unserialize() call. Which means it should be
> put to dtor list via var_push_dtor() or var_push_dtor_no_addref().
>
> Now, that all was true for PHP 5. For PHP 7, the matter are more
> complicated as the lists now keep zvals and not zval *, and I'm not 100%
> sure yet what that means. I need to look into it and figure out.

Thank you for the detailed explanation!
It's very helpful and it seems I'm better to wait for unserialize() patch.
I'll finish other parts first.

Regards,

--
Yasuo Ohgaki
yohgaki@ohgaki.net