Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:75024
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: pass (pb1.pair.com: domain gmail.com designates 209.85.216.172 as permitted sender)
MIME-Version: 1.0
In-Reply-To: <53A3874E.20704@sugarcrm.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>
Date: Fri, 20 Jun 2014 19:46:25 +0200
Message-ID: <CAH-PCH6nSk0ir_TSjhCd0BVhiY5UsOsLKzqKhqfaU1G0ghi4Tg@mail.gmail.com>
To: Stas Malyshev <smalyshev@sugarcrm.com>, Sebastian Bergmann <sebastian@php.net>, 
	Marco Pivetta <ocramius@gmail.com>
Cc: Julien Pauli <jpauli@php.net>, Remi Collet <remi@fedoraproject.org>, 
	PHP Internals <internals@lists.php.net>
Content-Type: multipart/alternative; boundary=001a11c2acc884569a04fc48123d
Subject: Re: [PHP-DEV] Re: Problems with the fix for the BC break introduced
 in 5.4.29 and 5.5.13
From: tyra3l@gmail.com (Ferenc Kovacs)

--001a11c2acc884569a04fc48123d
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable

On Fri, Jun 20, 2014 at 2:58 AM, Stas Malyshev <smalyshev@sugarcrm.com>
wrote:

> Hi!
>
> > Wouldn't it be much easier to just revert for 5.4 and 5.5 and then
> > patching `ReflectionClass#newInstanceWithoutConstructor()` to just
> > handle internal PHP classes as well?
>
> We can not just handle internal PHP classes without calling their ctor.
> That's the whole problem - for internal class, if ctor is not called,
> you can get segfault on anything you do with the class.
>
> > Is there a technical limitation for this?
> > Are there security problems related to the patch that broke
> > phpunit/doctrine? If not, can the patch simply be delayed and only
> > applied to 5.6?
>
> Right now we know this thing can produce segfaults. But running code on
> unititialized data may be worse than that.
>
> --
> Stanislav Malyshev, Software Architect
> SugarCRM: http://www.sugarcrm.com/
> (408)454-6900 ext. 227
>

I have a longer draft reply about the technical parts, but I wanted to send
a quick headsup:
The original SplFileObject segfault can be achived without any unserialize
trick, one can simply extend an internal class depending on it's
constructor for providing the required initial state, and simply not call
the parent::__construct() from the child:
http://3v4l.org/fqFC6

And I also think that we can't fix/remove the O:X:"Y":0:{} trick without
providing some usable alternatives for phpunit/doctrine(not providing it
would really hurt the adoption imo).
So at the moment I see two options:

   1. Revert back to the original behavior for 5.4 and 5.5, and for 5.6
   introduce the original clean fix (classes implementing Serializable can
   only be unserialized with the "C:" format) and remove the restriction fr=
om
   ReflectionClass::newInstanceWithoutConstructor about internal classes (a=
nd
   make sure that we put up a warning in the docs about the potential
   instability of internal classes instanitated this way.
   2. Do the same as suggested for 5.6 in 1) but also for 5.4 and 5.5.

I think the first one is better.
If we would keep the current status, it would mean for the userland project
that:

   - for 5.6
      - they should use ReflectionClass::newInstanceWithoutConstructor for
      non-internal classes(using clunky checks like in
      https://github.com/sebastianbergmann/phpunit-mock-objects/commit/4a33=
8e464a94d3e5a48ae28292e98e9b4e3ac898
      )
      - they should need to use the unserialize O:X:"Y":0:{} trick for
      internal(including non-internal classes extending internal
classes) classes
      not implementing Serializable
      - they should use unserialize C:X:"Y":0:{} for internal classes
      implementing Serializable and because there are classes denying the
      unserialization or requiring specific unserialize payload (which we c=
an't
      expect the userland to keep track of) they would have to check if the
      unserialize fails and throw and exception, making the
instanitation of some
      classes outright impossible.
   - for 5.4 and 5.5
      - they should use ReflectionClass::newInstanceWithoutConstructor for
      non-internal classes(using clunky checks like in
      https://github.com/sebastianbergmann/phpunit-mock-objects/commit/4a33=
8e464a94d3e5a48ae28292e98e9b4e3ac898
      )
      - they should need to use the unserialize O:X:"Y":0:{} trick for the
      internal classes not implementing Serializable and for
non-internal classes
      extending internal classes implementing Serializable
      - they should use unserialize C:X:"Y":0:{} for internal classes
      implementing Serializable and because there are classes denying the
      unserialization or requiring specific unserialize payload (which we c=
an't
      expect the userland to keep track of) they would have to check if the
      unserialize fails and throw and exception, making the
instanitation of some
      classes outright impossible.

(and both phpunit and doctrine 2 supports 5.3 where
ReflectionClass::newInstanceWithoutConstructor isn't available)

--=20
Ferenc Kov=C3=A1cs
@Tyr43l - http://tyrael.hu

--001a11c2acc884569a04fc48123d--