Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:74972
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: pass (pb1.pair.com: domain gmail.com designates 209.85.216.179 as permitted sender)
MIME-Version: 1.0
In-Reply-To: <53A0C6DD.50705@sugarcrm.com>
References: <CAH-PCH6xEXspisLOHtiTbViA4vDTk7q6LM3t_Bh9Dzr1cj_Nag@mail.gmail.com>
	<53A0C6DD.50705@sugarcrm.com>
Date: Wed, 18 Jun 2014 10:03:02 +0200
Message-ID: <CAH-PCH4zN-p2YPzp8iwwBN-Xumcdk6FsqEJv+6EwZ4tgyarJgw@mail.gmail.com>
To: Stas Malyshev <smalyshev@sugarcrm.com>
Cc: Stanislav Malyshev <stas@php.net>, julien pauli <jpauli@php.net>, Anatoliy Belsky <ab@php.net>, 
	Remi Collet <remi@php.net>, PHP Internals <internals@lists.php.net>
Content-Type: multipart/alternative; boundary=001a11c16aca7f58a104fc17b023
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)

--001a11c16aca7f58a104fc17b023
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable

On Wed, Jun 18, 2014 at 12:53 AM, Stas Malyshev <smalyshev@sugarcrm.com>
wrote:

> Hi!
>
> > Could you check out my last mail about the unserialize stuff?:
> > http://news.php.net/php.internals/74947
>
> Reading this message, I gather the situation is as follows:
>
> 1. Original fix banned O: unserialization of classes that have custom
> serializers.
>

Yeah, with
https://github.com/php/php-src/commit/757da1eed5ffbc1c5a0ae07e82ae91f7e431f=
1a2
we introduced the Serializable interface, which provides a different method
to serialize/unserialize an object.
Since then, serialize/unserialize uses a different codepath for objects
implementing this interface: __sleep()/__wakeup() is ignored, the
unserialized format will use C: instead of O, and the format of the payload
will be also a bit different:
http://3v4l.org/B57Kk

Even though we documented this from the userland point of view:
http://www.php.net/Serializable

Classes that implement this interface no longer support __sleep() and
> __wakeup(). The method serialize is called whenever an instance needs to =
be
> serialized.
> ...
> The above example will output something similar to:
> string(38) "C:3:"obj":23:{s:15:"My private data";}"


So while we never produced "O:" for objects implementing Serializable, we
also never enforced/validated that, which made it possible to unserialize a
Serializable via bypassing it's unserialize() method, and this also allowed
to unserialize classes which explicitly prohibits that
(zend_class_unserialize_deny).

Userland devs could have used Reflection to check if the class they are
trying to mock/whatever was implementing Serializable, but that would have
required a Reflection call plus with the O: format you can always use the
O:$classNameLength:"$className":0:{}
for every class, while with the C: format, you have to make sure that your
string passes the unserialize() call of that specific class, which makes it
unfeasible for general purpose libraries like phpunit-mock-objects or
doctrine2 entity manager.


> 2. The following fix allowed this behavior for user classes.
>

yes


> 3. The second fix means that if the user class descends from internal
> class with serializer, we still have a problem.
>

correct


>
> My opinion is this:
>
> 1. Using unserialize() on anything that is not a result of serialize()
> is a hack. As such, there are no support guarantees that it would work
> in any particular manner, besides continuity of support for serialized
> format itself. In particular, we have no guarantees that string that did
> not come from serializer would behave in any particular way.
>

strongly agree


>
> 2. We should make reasonable effort to keep code that worked in version
> x.y.z working in x.y.z+1. However, "reasonable" is important here, if
> there's a behavior that is not documented and not wanted, we can break
> it. We don't have to and will try not to, but we can.
>

+1, we don't really document the php serialize format (only show some
examples), so I would consider it an internal implementation detail, and we
indeed mentioned in the docs that classes implementing Serializable will
have a different output, so I think that it isn't anything wrong with
actually starting to enforce the documented behavior and validate and
reject malformed serialized strings.


>
> 3. In light of this, I think we could do with current patch in 5.4 and
> 5.5 (one that permits userland classes) but we should plug it completely
> for 5.6. If somebody needs code that does whatever it did, it should be
> in Reflection or someplace else, serializer should do serializing. But I
> think for stable version it is a reasonable, if imperfect, compromise -
> it would allow most common use cases (userland classes) to work and most
> common crashes (internal classes) to not crash. Extending SplFileInfo
> looks a bit more exotic so I think we can live with it not being fixed
> till 5.6.
>
>
I also agree that it is more important to make sure that internal objects
are properly instantiated and not subject to random segfaults and probably
other problems, than to allow an undocumented hack to be used for easier
instantiation of objects.

Back then when Sebastian
proposed ReflectionClass::newInstanceWithoutConstructor() for 5.4, he
himself introduced the only userland class restriction, because as he
mentioned some internal classes would crash if instantiated without a
constructor:
http://marc.info/?l=3Dphp-internals&m=3D131426563418067&w=3D2
And it properly considers userland classes extending internal classes as
internal, and prohibits the instantiation for such classes:
http://3v4l.org/Zp4YS

I agree that this would be big hit for some of those tools/libs, but other
than fixing every internal class to be properly initialized without their
constructors (which would be a nice thing anyways) I don't see any other
way to fix the segfaults while keeping the ability to instantiate any class
(be that an internal class, or a class implementing Serialize) with ease.

--=20
Ferenc Kov=C3=A1cs
@Tyr43l - http://tyrael.hu

--001a11c16aca7f58a104fc17b023--