Newsgroups: php.internals
Path: news.php.net
Xref: news.php.net php.internals:74947
Mailing-List: contact internals-help@lists.php.net; run by ezmlm
Received-SPF: pass (pb1.pair.com: domain gmail.com designates 209.85.216.45 as permitted sender)
MIME-Version: 1.0
In-Reply-To: <CAOhuFvYuVhe1BDYQSfnftaTMNVeOZnXL482Sx3r508jhSyVsfg@mail.gmail.com>
References: <53886F73.70402@php.net>
	<CAEPJdunEaCHaBvzQ+AJiO5tLGW2NhW53fot1hztNXkV+sUDNSQ@mail.gmail.com>
	<1401810913.2282.81.camel@guybrush>
	<538DFEB1.7030207@fedoraproject.org>
	<CADyq6sKU0Hsm4tdO=tKuUDqgZHn6FEU40ROJ8yC=xu13tSAUxQ@mail.gmail.com>
	<CANzZr6k7=+eV69y7OSQuM5ThhNOEmrKLj-RUQy2o4qDdW=sTHQ@mail.gmail.com>
	<538EAD75.5020402@fedoraproject.org>
	<CAEPJdumPPUM6bkVa1ACXkREsOCpPGrOjGJeRKgAhho-H+JkE4g@mail.gmail.com>
	<CAMUwpuTZxoOgKHbJ5xMBfj-yYFQW3QAaBSVq+UYDrGkTX=YWYg@mail.gmail.com>
	<CAEPJdukpDboN-d=8TSpQZ6FHXeatj=RLhzAmRhNwWA8B+Ucx1A@mail.gmail.com>
	<CAEPJdukpcq6MikdkOfB=Qrw37M15NGK7MFPUTLjrLoHu5g=rfA@mail.gmail.com>
	<CAMUwpuReCN-QaN=wHLcgAb=Z0sLXNtHqkcGS63_bOboBNd37ew@mail.gmail.com>
	<CADyq6sJWVWH+74tppTX3Fun6YwZD=L=hMOOH5OGDUcXjKjrCiA@mail.gmail.com>
	<CAMUwpuRTd2i24Y8V+ONqnjyhZB3cfQG+C+9h1TFpWmoKVqdyLQ@mail.gmail.com>
	<CAH-PCH5TvCtL21J-GyUaJ99OQm+AZ9sd1dkj0i4=5FPUdTirBA@mail.gmail.com>
	<CADyq6sKFmpvTn5RwP1N1kcqZ2WCsp4TEmaNygi29JV76N7S6_w@mail.gmail.com>
	<539B0888.9040208@fedoraproject.org>
	<CADyq6sLTKNa6V-R0Y97fEzGLMLCaAys3S9Hu0Sijzb2isMnrFg@mail.gmail.com>
	<CAOhuFvYuVhe1BDYQSfnftaTMNVeOZnXL482Sx3r508jhSyVsfg@mail.gmail.com>
Date: Tue, 17 Jun 2014 15:49:28 +0200
Message-ID: <CAH-PCH5+oCzH4eunNckPufYWBw=40oiYEs3ACC9N_PN4z7h8YQ@mail.gmail.com>
To: Rafael Dohms <listas@rafaeldohms.com.br>
Cc: Marco Pivetta <ocramius@gmail.com>, Remi Collet <remi@fedoraproject.org>, 
	internals <internals@lists.php.net>
Content-Type: multipart/alternative; boundary=001a11c139ba981b7504fc086916
Subject: Re: [PHP-DEV] BC break in 5.4.29 and 5.5.13
From: tyra3l@gmail.com (Ferenc Kovacs)

--001a11c139ba981b7504fc086916
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable

Hi,

My replies are inline and the tl;dr is that I don't think that it is a good
idea to release the current "fix" to the problem as-is:


On Tue, Jun 17, 2014 at 11:01 AM, Rafael Dohms <listas@rafaeldohms.com.br>
wrote:

> On Fri, Jun 13, 2014 at 4:25 PM, Marco Pivetta <ocramius@gmail.com> wrote=
:
>
> > On 13 June 2014 16:19, Remi Collet <remi@fedoraproject.org> wrote:
> >
> > > See my proposal
> > > http://marc.info/?l=3Dphp-internals&m=3D140257374715698&w=3D2
> > >
> > > Not designed as a fix, but rather a small improvement to make this ha=
ck
> > > possible for Internal and Serializable classes
> > >
> >
> > Thanks Remi, that looks indeed like what we'd want :-)
> >
> > Marco Pivetta
> >
> > http://twitter.com/Ocramius
> >
> > http://ocramius.github.com/
> >
>
>
> Its a BC break people.
> It does not matter if we think A or B are correct, a point release CANNOT
> break BC.
>

generally BC breaks are not allowed in micro versions, but this doesn't
mean that we can't change existing behavior in micro versions.
currently there are 3 cases where we have precedence changing behavior in
micro versions:

   - bugfix
   - changing (explicitly) undefined behavior
   - introducing a new class/function/method/constant (I don't like this,
   but it seems that it is the consensus seems to be that this is okay).

In this particular case the BC was caused by a bugfix (
https://bugs.php.net/bug.php?id=3D67072) and changed the unserialize behavi=
or
regarding of the usage of the Serializable interface to prevent such issues
while keeping the implementation in-line with the docs (see
http://markmail.org/message/u6r6rzj6tzejlaaf for the details).

Even though that we noticed that it would affect some userland code and
Stas argued for not introducing the BC break, Anatol's argument seemed to
be reasonable enough to keep the change, and nobody else complained.

After the release, we get a some more complaints and Anatol decided to make
a compromise and change the fix to allow userland classes to be
instanitated via this trick, but still preventing the constructorless
instanitation of the internal classes via the crafted unserialize string
method.
While this will solve like 99.X% of the cases for the userland, this still
a BC break, as previously it was possible to instaniate internal classes
implementing the Serializable interface with the unserialize trick without
calling the constructor and we don't allow that anymore, and on a second
look, this also brings back the original problem this change was trying to
prevent but with a bit more effort:

[tyrael@ferencs-mbp-135 php-src.git (PHP-5.5 =E2=9C=97)]$ ./sapi/cli/php -v
PHP 5.5.15-dev (cli) (built: Jun 17 2014 15:33:34) (DEBUG)
Copyright (c) 1997-2014 The PHP Group
Zend Engine v2.5.0, Copyright (c) 1998-2014 Zend Technologies
[tyrael@ferencs-mbp-135 php-src.git (PHP-5.5 =E2=9C=97)]$ ./sapi/cli/php -r=
 'class
Foo extends SplFileObject{};$foo =3D
unserialize("O:".strlen("Foo").":\"Foo\":0:{}");echo $foo;'
[1]    72973 segmentation fault  ./sapi/cli/php -r

So the current "fix" still breaks BC compared to the original buggy
behavior, but still not completely fixes the problem...

I think that the original change was warranted here and even though that we
introduced a BC break, we shouldn't allow instanitation of
incomplete/unstable objects because that opens up more serious problems for
the users, and I'm somehow glad that we didn't rushed with the release of
this "fix", as I'm not entirely convinced, that this is the proper way for
handling this issue.


> It has been 18 days without a 5.5.14 release or a rollback of 5.5.13.
> This is highly unacceptable and irresponsible of a core language.
>
> I cannot understand why we are discussing this and leaving userland broke=
n,
> this affect a lot more the phpunit/doctrine.
> Rollback the release, or release a 5.5.14 release that fixes it, THEN you
> discuss what the proper thing should be and worry about it in 5.5.15.
>

I think that this is a bit blown out of proportion, even thought that this
change affected some really popular tools, but this was still caused by a
warranted fix in the internal unserialization behavior which was documented
to work the way as Anatol changed to behave in the first place.
I think it is a pretty good thing that we haven't rushed out another
release without making sure we have a fix we are satisfied with.

--=20
Ferenc Kov=C3=A1cs
@Tyr43l - http://tyrael.hu

--001a11c139ba981b7504fc086916--