Re: BC break in 5.4.29 and 5.5.13

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=php-internals&m=140257374715698&w=2 > > > > > > Not designed as a fix, but rather a small improvement to make this hack > > > 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=67072) and changed the unserialize behavior 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 ✗)]$ ./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 ✗)]$ ./sapi/cli/php -r 'class Foo extends SplFileObject{};$foo = 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 broken, > 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. -- Ferenc Kovács @Tyr43l - http://tyrael.hu