Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:75921
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: pass (pb1.pair.com: domain gmail.com designates 209.85.192.48 as permitted sender)
MIME-Version: 1.0
In-Reply-To: <CAMUwpuRbZT+8KU_0HLMkbCzkv4Xg6gS7_syHWTh1HNYWBWpGfw@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>
	<CAMUwpuRbZT+8KU_0HLMkbCzkv4Xg6gS7_syHWTh1HNYWBWpGfw@mail.gmail.com>
Date: Wed, 23 Jul 2014 07:58:15 +0200
Message-ID: <CAH-PCH42EhQBL6fq5eQPWrR_DxvUbWACEyY_C0qHTKGYY67dzA@mail.gmail.com>
To: Julien Pauli <jpauli@php.net>
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: multipart/alternative; boundary=001a11c23202aeb9c304fed606da
Subject: Re: [PHP-DEV] Problems with the fix for the BC break introduced in
 5.4.29 and 5.5.13
From: tyra3l@gmail.com (Ferenc Kovacs)

--001a11c23202aeb9c304fed606da
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable

On Tue, Jul 22, 2014 at 6:20 PM, Julien Pauli <jpauli@php.net> wrote:

> 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 class=
es
> >>> having custom object storage (eg. create_object) shouldn't assume tha=
t
> >>> their __construct will be called, but either do the initialization in
> the
> >>> create_object hook or validate in the other methods that the object w=
as
> >>> 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 tri=
ck
> >>> 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 initi=
al
> >>> 5.6.0 release (ofc it would be wonderful if people could lend me a ha=
nd
> >>> for
> >>> fixing as much as we can before the release), but we have to fix thos=
e
> >>> anyways.
> >>
> >>
> >> This sounds reasonable to me. newInstanceWithoutConstructor does not
> have
> >> the same remote exploitation concerns as serialize, so allowing crashe=
s
> 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 previous=
ly
> > 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() fr=
om
> > 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
>

thanks, I will keep that in mind when we start moving the initialization
from the constructors into the create_object functions.
I've also went ahead and created a pull request for the proposed changes:
https://github.com/php/php-src/pull/733
as you can see I've taken Nikita's advice and internal classes with final
constructors are still not allowed to be instantiated.

--=20
Ferenc Kov=C3=A1cs
@Tyr43l - http://tyrael.hu

--001a11c23202aeb9c304fed606da--