Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:75870
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: pass (pb1.pair.com: domain gmail.com designates 209.85.220.170 as permitted sender)
MIME-Version: 1.0
Sender: julienpauli@gmail.com
In-Reply-To: <CAH-PCH6uUVH1F8C9mn0q4mvKsyiG_VPZPKMuiepn+Cp0K0=9Ug@mail.gmail.com>
References: <CAH-PCH6xEXspisLOHtiTbViA4vDTk7q6LM3t_Bh9Dzr1cj_Nag@mail.gmail.com>
 <53A1C722.9060501@fedoraproject.org> <53A21137.6010705@sugarcrm.com>
 <CAMUwpuT_S2EE6cdJM95RzY4vmORGx3vJ9+gau8G3EZCMsNSydA@mail.gmail.com>
 <53A2A9BD.1070603@sugarcrm.com> <CAH-PCH5-vtCVvGvceMqt6rDXq4zo0Vb34cyDK3KdBUNfg_oLyg@mail.gmail.com>
 <CADyq6sKQbV+Avzfs3OAdGM9Hc85i18t0LSATD6Rk=HjqKOMgBg@mail.gmail.com>
 <53A3874E.20704@sugarcrm.com> <CAH-PCH6nSk0ir_TSjhCd0BVhiY5UsOsLKzqKhqfaU1G0ghi4Tg@mail.gmail.com>
 <53A62AFF.4080302@sugarcrm.com> <CADyq6s+N_mNc-n1g=S5LwWBZ9fkZ+hqRc3Ycu8M7EBd9Hc1ABw@mail.gmail.com>
 <53B10D59.4060206@sugarcrm.com> <F5324819-A85F-47BA-8FE5-27FBE87E1B89@gmail.com>
 <CAH-PCH6Tb-QbTaB3_qthpiGo4fcs0afViDhACmcur=vZ+=HnFg@mail.gmail.com>
 <CAF+90c9r18LYsfFOhTj9faqBy2w+MxKAk9WmVi3PZ0uzABwQdQ@mail.gmail.com> <CAH-PCH6uUVH1F8C9mn0q4mvKsyiG_VPZPKMuiepn+Cp0K0=9Ug@mail.gmail.com>
Date: Tue, 22 Jul 2014 18:20:45 +0200
Message-ID: <CAMUwpuRbZT+8KU_0HLMkbCzkv4Xg6gS7_syHWTh1HNYWBWpGfw@mail.gmail.com>
To: Ferenc Kovacs <tyra3l@gmail.com>
Cc: Nikita Popov <nikita.ppv@gmail.com>, Alexey Zakhlestin <indeyets@gmail.com>, 
	Stas Malyshev <smalyshev@sugarcrm.com>, Marco Pivetta <ocramius@gmail.com>, 
	Sebastian Bergmann <sebastian@php.net>, Remi Collet <remi@fedoraproject.org>, 
	PHP Internals <internals@lists.php.net>
Content-Type: text/plain; charset=UTF-8
Subject: Re: [PHP-DEV] Problems with the fix for the BC break introduced in
 5.4.29 and 5.5.13
From: jpauli@php.net (Julien Pauli)

On Tue, Jul 22, 2014 at 2:04 PM, Ferenc Kovacs <tyra3l@gmail.com> wrote:
>
>
>
> On Tue, Jul 22, 2014 at 1:48 PM, Nikita Popov <nikita.ppv@gmail.com> wrote:
>>
>> On Tue, Jul 22, 2014 at 12:27 PM, Ferenc Kovacs <tyra3l@gmail.com> wrote:
>>>
>>> sorry for the late reply.
>>> you are right and after looking into the implementation I think classes
>>> having custom object storage (eg. create_object) shouldn't assume that
>>> their __construct will be called, but either do the initialization in the
>>> create_object hook or validate in the other methods that the object was
>>> properly initialized.
>>> given that this lack of initialization problem is already present(you can
>>> extend such a class and have a __construct() in the subclass which
>>> doesn't
>>> call the parent constructor), and we want to keep the unserialize trick
>>> fixed (as that exposes this problems to the remote attacker when some
>>> code
>>> accepts arbitrary serialized data from the client) I see no reason to
>>> keep
>>> the limitation in the ReflectionClass::newInstanceWithoutConstructor()
>>> and
>>> allowing the instantiation of internal classes will provide a clean
>>> upgrade
>>> path to doctrine and co.
>>> ofc. we have to fix the internal classes misusing the constructor for
>>> proper initialization one by one, but that can happen after the initial
>>> 5.6.0 release (ofc it would be wonderful if people could lend me a hand
>>> for
>>> fixing as much as we can before the release), but we have to fix those
>>> anyways.
>>
>>
>> This sounds reasonable to me. newInstanceWithoutConstructor does not have
>> the same remote exploitation concerns as serialize, so allowing crashes here
>> that can also be caused by other means seems okay to me (especially if we're
>> planning to fix them lateron). Only additional restriction I'd add is to
>> disallow calling it on an internal + final class. For those the __construct
>> argument does not apply. For them the entity-extending-internal-class
>> usecase doesn't apply either, so that shouldn't be a problem.
>>
>> Nikita
>>
>
> Thanks for the prompt reply!
> I was considering mentioning the final constructors, but as we previously
> didn't checked that and from a quick look it seems that we are mostly using
> it as an easy/cheap way to make sure that the object is initialized properly
> (which could also happen when a subclass calls parent::__construct() from
> it's constructor) which isn't exactly the best usecase for final.
> But I don't really mind having that check.

I'm +1 also with the idea.

Just take care to have a clone_handler defined as well, as the default
clone handler doesn't call create_object.
http://lxr.php.net/xref/PHP_5_5/Zend/zend_objects.c#218

Julien