Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:75009
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: pass (pb1.pair.com: domain gmail.com designates 209.85.192.54 as permitted sender)
MIME-Version: 1.0
In-Reply-To: <53A2A9BD.1070603@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>
Date: Fri, 20 Jun 2014 02:30:36 +0200
Message-ID: <CAH-PCH5-vtCVvGvceMqt6rDXq4zo0Vb34cyDK3KdBUNfg_oLyg@mail.gmail.com>
To: Stas Malyshev <smalyshev@sugarcrm.com>
Cc: Julien Pauli <jpauli@php.net>, Remi Collet <remi@fedoraproject.org>, 
	PHP Internals <internals@lists.php.net>
Content-Type: multipart/alternative; boundary=089e0149caa62b661004fc399a84
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)

--089e0149caa62b661004fc399a84
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable

On Thu, Jun 19, 2014 at 11:13 AM, Stas Malyshev <smalyshev@sugarcrm.com>
wrote:

> Hi!
>
> > For me, it's a safe solution, however, it breaks BC, I think it's a
> > no-go for 5.4 and 5.5.
>
> It breaks BC in something that never was part of the official API, never
> was promised to work and works only by accident for internal classes.
> Each such object can segfault at smallest provocation, so keeping them
> is essentially requiring that we keep segfault compatibility. I don't
> think we should promise that.
>
> > - Revert the BC break in 5.5 and 5.4
> > - Keep the segfault, we've been living with it for ages
>
> That's not the reason not to fix segfaults. Virtually every bug we're
> fixing in the code we have been "living with for ages". That's not the
> reason to keep them now that we know it segfaults.
>
> > - Patch the manual to clearly show one should never try to unserialize
> > hand-made strings : we just do not support such behavior (thus, it
> > could lead to segfaults)
>
> Well, here we contradict ourselves then - if we do not support such
> behavior, why we are taking so much effort to enable it? Because
> declaring that changing something even in small part is inacceptable
> even if the price is known crashes in the application (and I wonder what
> security people can make of it - uninitialized objects might be way
> worse than just null pointer deref...) - that looks a lot like
> supporting to me. So in what meaning we don't support it then?
> --
> Stanislav Malyshev, Software Architect
> SugarCRM: http://www.sugarcrm.com/
> (408)454-6900 ext. 227
>
> --
> PHP Internals - PHP Runtime Development Mailing List
> To unsubscribe, visit: http://www.php.net/unsub.php
>
>
I've linked the discussion and provide a really compact summary (I think it
is too compact) at https://bugs.php.net/bug.php?id=3D67072
I'm a bit indecisive about the current situation, because I think that we
should fix the root cause of these problems, but I'm also wanna make sure
that we don't totally criple phpunit-mock-objects and doctrine with a micro
release.
I think it would be safe to prohibit the unserialization of classes
implementing the Serializable interface with the "O:" format, userland
should use the "C:" format for these classes, we don't have that many
classes outright denying the unserialization, and if a class validates the
incoming data in the unserialize method, then the userland should make sure
to provide appropriate data for the validation to pass. (Ofc. we should
also make sure that we don't require more data, than necessary, see
https://bugs.php.net/bug.php?id=3D67453&edit=3D1)

Another topic would be to that internal classes (not implementing
Serializable) can still be instantiated without the constructor call
through the unserialize method (using the "O:" format), and we can't
prohibit that, as it is/can be perfectly normal to unserialize an internal
class previously properly instantiated and serialize, but we don't have the
knowledge in the class to tell what is a properly serialized string, and
what isn't for that given class.
(Which means that allowing the internal classes to be instantiated via
ReflectionClass::newInstanceWithoutConstructor() doesn't really introduces
a new set of problems, it would just allow to shut yourself with reflection
instead of an obscure unserialize trick. (Not saying we should do this,
just mentioning that the problem already exists).
I think that it would be a nice if we could add more validation for the
internal classes __wakeup()/unserialize() methods for data which presence
is mandatory for the class to be able to work, which would ofc. bother the
life of the phpunit/doctrine users/devs, but it would only prevent the
mocking those classes which would be unstable/dangerous previously
when instantiated
without the constructor call.

--=20
Ferenc Kov=C3=A1cs
@Tyr43l - http://tyrael.hu

--089e0149caa62b661004fc399a84--