[PHP-INTERNALS] BC break in 5.4.29 and 5.5.13

Fixed in doctrine

https://github.com/doctrine/doctrine2/commit/93c276d059b40b0783ba9a24549a8b135e257693#diff-093aaad4da679d0374260757bf7bafca
Fix in PHPUnit

https://github.com/sebastianbergmann/phpunit-mock-objects/commit/1c68f1338f1940deb8265428bb2a7cbc5bc074b5#diff-64dbbc1c21f9be2d92e2b715617ffe34

https://github.com/sebastianbergmann/phpunit-mock-objects/commit/1c68f1338f1940deb8265428bb2a7cbc5bc074b5#diff-64dbbc1c21f9be2d92e2b715617ffe34

Hi Remi,
thanks for the reproduce case. As discussed on the IRC, I'll be working on
a solution to minimize the BC break in 5.4 and 5.5. IMHO 5.6 is fine with
that "as is", as it's correct with the current specifications of the
Serializable and fixes wide range crash vulnerability.

Regards

Anatol

I've invented a patch fixing BC regarding this issue. It passes the
userspace classes but blocks the internal classes. While it's a must for
the stable 5.4 and 5.5, I still doubt we should put it into 5.6++. Even it
can work with userspace classes, it's highly inconsistent. For example - a
userclass implementing Serializable but throwing an exception in
unserialize() will behave wrong when instantiated that way.

Basically before I apply this fix - IMHO, it should go into 5.4 and 5.5
only, 5.6 should have clean behaviour. From the userspace perspective -
doctrine and phpunit have already fixed this using
ReflectionClass::newInstanceWithoutConstructor(), so that is fine with the
future. The fix should only go for legacy. So, what you say?

Regards

Anatol

Would it be possible to do a quick releases of 5.4 and 5.5 that fixes the
BC break (it means 5.4.30 and 5.5.14)? I had to spent some time this
morning updating one project just to update Doctrine.

The thing is that not everyone knows about the issue and not everyone is
using "dev-master" in the composer...

Also updating composer is for some project with lots of depencies really
painful (especially if you don't have time to do it often... :) ). I don't
think that requiring it for php bug release is a good idea. It would be
great to do a new release as soon as possible and prevent any such issue.

Thanks

Jakub

such an issue btw. is a case where I can imagine a php.net frontpage
news entry by the RMs :)

johannes

Tested. Work fine.
Thanks.
Agree.


Remi.

@Julien: So what is the strategy now? This should be reverted ASAP.

After this my question, besides the PHPT, aren't any tests against php
libraries run before releases?
Especially this one would have broken PHPUnit already in any test using
mocks.

We were aware of the BC break before release (see
http://marc.info/?l=php-internals&m=139932135130155&w=2), but it looks like
the issue was forgotten after some initial discussion.

Nikita

We send out RCs two weeks before a release, but receive barely any
feedback. I'd love if external projects would verify those and raise a
red flag ...

johannes

We don't have the resources to test against RCs, and we don't have any CI
environment on our own except for travis-ci (which is what most projects
currently use).

Maybe push a `php-nightly` binary to the Travis folks instead? We'd gladly
test against that, and even if people won't report immediately, the travis
API is simple enough to allow mining on the builds to discover problems
eagerly.

Marco Pivetta

http://twitter.com/Ocramius

http://ocramius.github.com/

Nightly testing (or push testing) I see on our side. The issue with
testing applications is that we need the time from the individual
project maintainers. Not knowing the application it often is quite hard
to dig down what in the underlying systems broke. Project maintainers
typically an pinpoint this faster.

Speaking about resources: The core PHP team is very small, way to small
to keep up with a relevant amount of projects all moving.

johannes

Pushing a "nightly" package to Travis would solve this issue almost
entirely.

There's an already working test suite by the HHVM folks at
https://github.com/facebook/hhvm/blob/1ecb4443d95c2e2659028f148083dd5c85f2099b/hphp/test/frameworks/frameworks.yaml

In the eventuality in which PHP-SRC retains control over the CI environment
that runs the framework tests, this suite could be reused. The problem is
that it actually takes days to run all those tests, and I don't know if the
hardware is available.

Marco Pivetta

http://twitter.com/Ocramius

http://ocramius.github.com/

Perhaps just a small, frequently-used subset could be tested?

I agree we should run our tests against well known tested projects,
such as PHPUnit , Doctrine or other.

That's in a perfect world though.

Julien.P

I suggest we merge Anatol's patch to 5.5 so it's gonna be in 5.5.14 and up.

I'm not really fan to merge it to 5.6. The patch fixing the BC
introduces inconsistencies between internal and userland classes, and
I don't really like that.
Other thoughts perhaps ?


Julien

Hi Julien, Marco,
I finished setting up a similar CI environment for the project I work
on, so that the Revive Adserver test suite is also run with latest PHP
5.4, 5.5, 5.6, master, phpng (disabled atm) and int64 every night.

I've sent an email about it to php-qa just a few days ago.

I could set up some jobs for a few more open source projects if they are
not extremely resource hungry.


Cheers

PHPUnit was fairly easy:

https://revive.beccati.com/bamboo/browse/PHPUN-LATE-2

It currently fails on 5.4, 5.5, phpng from last night.


Cheers

Ow, I missed the mail to qa so.

That's a nice job, I'll use that frequently before releases to ehance
our quality.

Thank you for helping in this critical part :-)

Julien

And the bug is known by upstream project.

It is fixed in Doctrine and in PHPUnit.

Both project doesn't seems to think it is important as they don't
publish any new version with the fix included.


My 0,02€

Remi.

We didn't know of the bug/problem until it landed in the release

It is going to be released, but yes, we actually have the expectation a
patch release for 5.4 and 5.5 with a rollback of this breakage.
Even if we fix the bug ourselves, this is going to break apps that receive
a PHP upgrade and not an upgrade of the library dependencies.

Marco Pivetta

http://twitter.com/Ocramius

http://ocramius.github.com/

  The original Doctrine PR that fixed this issue was
https://github.com/doctrine/doctrine2/pull/1045, and Benjamin made some
revisions in
https://github.com/doctrine/doctrine2/commit/e577e7786796e9393df381f02221046dea6253a5
(note: there is a typo there, as he actually should be checking for 5.4.29
instead of 5.4.28). Based on the conversation above, it sounds like the
revert will end up in 5.4.30 and 5.5.14, but we should expect the change
for 5.6+.

In that case, Benjamin's commit needs to (a) fix the 5.4.x typo and (b) add
another condition to utilize Reflection for 5.6+.

Has PHPUnit already incorporated a work-around for this? At a glance, I
didn't see anything in the main repository or
https://github.com/sebastianbergmann/phpunit-mock-objects.

Why don't you simply use newInstanceWithoutConstructor() as soon as
possible (5.4+) ?

I rather see unzerialize('O:%d:"%s":0:{}') as a workaround for old
version, instead of the opposite...

The fix is not really reverted, only a workaround for this regression,
and only for user class (so the segfault is fixed, for internal class).

FYI, a user of my repo have confirm (real app.) than
- doctrine + php 5.4.29 is broken
(the reason why I raised this issue here last Friday, I was aware of
PHPUnit problem for a long time, but was thinking it is 5.6 only)
- doctrine + initial patch works with php 5.4.29
- doctrine works with php 5.4.29 + patch
https://github.com/sebastianbergmann/phpunit-mock-objects/commit/1c68f1338f1940deb8265428bb2a7cbc5bc074b5

https://github.com/sebastianbergmann/phpunit-mock-objects/commit/ba8784d88ff4cc52cf9424d7f7c631ccd867019c


Remi.

P.S. FYI, in Fedora:
- Doctrine package includes the initial patch, so works
- PHPUnit package includes the upstream patch, so works
- PHP 5.5.13 includes the patch, so even a manually installed Doctrine
or PHPUnit will work.

Remi,

I didn't have time to check the implications of
"newInstanceWithoutConstructor", i didn't want to merge that directly into
our stable branches. Plans to migrate to the function for php5.4++ exist
though.

greetings
Benjamin

newInstanceWithoutConstructor() has been introducted into PHP to
address exactly the
use-cases we are considering in this topic. The Doctrine one for example.

Julien

i know that, but its no reason for me to introduce this in a point bugfix
release, just to fix an upstream BC break, without testing it further.

any news?

Patch will be included in next 5.5.14RCs and 5.5.14.

Julien

What about 5.6.x? I'm getting failure reports from other folks that have
the beta installed...

Marco Pivetta

http://twitter.com/Ocramius

http://ocramius.github.com/

I'm myself against merging it to 5.6

But for consistency, and if we follow our rules, the patch should also
be applied to 5.6
Ferenc, what do you think about it ?

Julien

See related discussion on
https://github.com/sebastianbergmann/phpunit-mock-objects/commit/5f58fcb4f94a9802e0861c94760d0ce2190fb6d3#commitcomment-6591497

Hi,

I agree that 5.6 should have a clean behavior solving both issues(being
able to initialize any object without a calling the constructor but now
having crash issues with improperly initialized internal objects). but I
have yet to see one.
If we don't have any other alternatives(in a reasonable timeframe) I would
be ok having the same fix as in 5.4 and 5.5.

Hi there,

I wrote a small wrapper around this "hack" that we've been using for years
(as suggested by Sebastian) and I put it at
https://github.com/Ocramius/Instantiator.

It seems like PHP 5.6 is still failing though (5.6.0-beta1):
https://travis-ci.org/Ocramius/Instantiator/builds/27493715
Test fails also on my machine (5.6.0-beta3).

Is any eventual fix going to be merged before the next beta?


Marco Pivetta

http://twitter.com/Ocramius

http://ocramius.github.com/

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



Remi

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.

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 can only agree with you here.

I totally fail to understand why we did not fire a new release right
away after we knew a BC break was introduced. We fail here.
Not even that. We know it is a BC break. So unless there is a change
that does not break BC, we cannot do that in any 5.x, not even 5.6,
except if there is a real appealing reason to do so (security, crash
bugs, etc).

Cheers,

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:

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.

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.

It is definitively not OK. I cannot imagine any feature that cannot
x.y+1, which happens less than a year later.

Cheers,

I agree with you, however the facts shows differently:
http://php.net/ChangeLog-5.php#5.5.11
OPCache: Added function opcache_is_script_cached().

And even the releaseprocess rfc is written in a way, which implies that
introducing new features are not considered as BC breaks:

https://wiki.php.net/rfc/releaseprocess

x.y.z to x.y+1.z

Bugfixes

*New features*Extensions support can be ended (moved to pecl)

*Backward compatibility must be keptAPI compatibility must be kept
(userland)*
ABI and API can be broken (internals), src compatibility should be kept if
possible, while breakages are allowed

Hi!
I think we've been through this a number of times and most of the people
agree that waiting for the next release cycle (at least a year) for a
self-contained minor function or feature addition makes little sense.
People add functions because they have need for them now, not years
later when they're ready to make a major version upgrade. We're not
talking something huge like new syntax or language feature, we're
talking about this:
Why one should wait for a year or more to be able to check if the script
is cached? It makes little sense to me. Adding it to active branch does
not hurt anything, so I see no problem with it.

And where is the limit? Add a full extension is fine too? No, it is not.

It reintroduces the pain to add dozen minors version checks for php
version dependencies.

Hi!
Depends on the extension. Adding new extension to core may be not
warranted, because you can just as well put it on PECL. So it's not a
good example - you can't just put a new function or parameter on PECL.
You don't need version checks. You need functionality checks. You'd need
them anyway if you want to support multiple versions.

you need both, and it is painful.

Hi!
It may be, but denying functionality because it's hard to ensure forward
compatibility is not a good way to go. If you need these checks, you
need this functionality, so delaying it for a year won't do good.

But no new features in released branches.

Hi!
We have a release schedule. Which is widely known - in fact, we've just
announced 5.5.14 RC1 last Thursday. I agree BC break is bad but not bad
enough to break the release schedule. If you suffer from this particular
problem, just keep on 5.5.12 for 2 weeks. And please test the RCs, if
people would actually test the RCs we'd know about such problems
*before* the release, not *after*. That's what RCs are for.

And here Stas you get my huge +1.
I cant understand the stress about this bug. It seems like the earth
is gonna blow up.
Suddenly, people are not "stuck with" 5.3.x anymore (thing we still
ear about, nearly daily), but they do run exactly 5.5.13. Common, are you
kidding ?

Just stay in 5.5.12, keep calm as there really is no need to lose
anyone's temper.
It is a BC break, yes, and we all apologize about it, believe it. But
it won't blow the earth up in the few next seconds, will it ?

The only moments I know we've been releasing a new version in rush,
was about huge security holes that got disclosed just after a release.

However, as I can see the patch seems to still have problems in our
RC1, we could go for an RC2 if no consensus is found about it before
next tuesday (the day we usually tag).
What do you think about this ?
Like Ferenc said, rushing a patch is a high risk at introducing
another bug, or just not fixing the bug properly. And then a bad
loop's gonna start from that.
Let's take time to test our patches / solutions, to keep our level of
quality. If RC2 is needed, there will be an RC2 (and more again if
needed). I'am all in favor of improving our quality , which usually is
the opposite of rushing.

Julien Pauli