Re: Re: com php-src: Improved the fix for bug #67072, thanks Nikita: ext/standard/tests/serialize/005.phpt ext/standard/tests/serialize/bug67072.phpt
ext/standard/var_unserializer.c ext/standard/var_unserializer.re
Hello,
On Mon, May 5, 2014 22:21, Stas Malyshev wrote:
> Hi!
>
>
>>> Commit: c2acdbdd3deb6787329bf0aca8ab0c04ace2a50c
>>> Author: Anatol Belski <ab@php.net> Fri, 18 Apr 2014
>>> 15:13:32
>>> +0200
>>> Parents: 6e1e98d7b833492594aea9cf416905b42f8ee0f4
>>> Branches: PHP-5.4 PHP-5.5 PHP-5.6 master
>>>
>>>
>>> Link:
>>> http://git.php.net/?p=php-src.git;a=commitdiff;h=c2acdbdd3deb6787329bf
>>> 0aca8ab0c04ace2a50c
>>>
>>>
>>> Log:
>>> Improved the fix for bug #67072, thanks Nikita
>>>
>>>
>>> Bugs:
>>> https://bugs.php.net/67072
>>>
>> Just ran into an issue running Symfony unit tests with PHPUnit. The
>> issue is this piece of code:
>>
>> // We have to use this dirty trick instead of
>> ReflectionClass::newInstanceWithoutConstructor()
>> // because of
>> https://github.com/sebastianbergmann/phpunit-mock-objects/issues/154
>> $object = unserialize(
>> sprintf('O:%d:"%s":0:{}', strlen($className), $className)
>> );
>>
>>
>> This ends up using the O: serialization type on a Serializable class,
>> thus causing a warning. Not sure what we should do about this.
>
> This looks like a BC break, we can not have that in 5.4 and 5.5. We'll
> have to fix or revert this. --
> 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
>
>
>
The initial bug is that an unserialization string misuses "C:" vs "O:"
format which would call an incorrect method "unserialize" vs. "__wakeup".
And that leads to a crash with that particular ticket, due to an
incompletely initialized object. Formally this fix just brings the
functionality inline with the documentation,
http://de1.php.net/serializable . It says
"Classes that implement this
interface no longer support __sleep() and __wakeup().". So that dirty
tricks should actually have been impossible, according to the doc. So per
fact, it breaks something which was already broken but users was relying
on it.
The patches fixes it globally preventing incomplete objects due to the
incorrect callback invocation. A fix for that particular ticket could be
to add __wakeup to SplFileObject and revert the patch to the unserialize
code. However that would open the rabbit hole again for any vulnerable
user constructed strings on any internal classes. Also, that would at most
shortly defer the issue because 5.6 is coming out in a few weeks, so the
bad user code will have to be fixed in the userspace soon. As conclusion -
IMHO it should stay fixed as it is, it's already documented in UPGRADING.
If you still think it's not worth it, so I can revert the global
unserializer fix and go with a local fix for the SplFileObject class. What
do you say, guys?
Cheers
Anatol
Thread (4 messages)