FAQ
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=c2acdbdd3deb6787329bf0aca8ab0c04ace2a50c

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

Search Discussions

  • Anatol Belski at May 5, 2014 at 9:34 pm
    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
  • Stas Malyshev at May 6, 2014 at 8:26 am
    Hi!
    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.
    I'm not sure I understand this point. The manual says __sleep/__wakeup
    won't be called, but that doesn't seem to me a problem here, the problem
    here seems to be that unserialize throws a warning, not that __wakeup is
    not called.
    The patches fixes it globally preventing incomplete objects due to the
    incorrect callback invocation. A fix for that particular ticket could be
    This also breaks code that worked before, and does it in stable
    versions. I do not think it is an acceptable solution for stable
    versions. Can't we make the fix in a way that does not have BC breakage
    for stable versions?
    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 -
    Only if that code is going to be run on 5.6.0 - which it won't be for
    some time because nobody runs .0 versions in production next day they
    are released. But even with 5.6, we are trying to minimize amount of
    code we're breaking in minor releases, and here we're breaking phpunit,
    which is one of the most used PHP tools. So I think we need to look for
    a better solution. CCing also Sebastian, maybe he could explain more
    about this hack and how it can be made to work more smoothly maybe.
    --
    Stanislav Malyshev, Software Architect
    SugarCRM: http://www.sugarcrm.com/
    (408)454-6900 ext. 227
  • Anatol Belski at May 6, 2014 at 9:02 am
    Hi Stas,
    On Tue, May 6, 2014 10:26, Stas Malyshev wrote:
    Hi!

    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.
    I'm not sure I understand this point. The manual says __sleep/__wakeup
    won't be called, but that doesn't seem to me a problem here, the problem
    here seems to be that unserialize throws a warning, not that __wakeup is
    not called.
    Please take the case from #67072

    - SplFileObject implements Serializable interface and serialization is
    disabled by setting the callbacks to NULL or to the special Zend callback

    - imagine the serialization were enabled on it, so the string would look like
    'C:13:"SplFileObject":.......'

    - when unserializing, the ->unserialize() method implementation should be
    called (while it's disabled)

    - the users wants to do the trick and constructs this
    'O:13:"SplFileObject":1:{s:9:"*filename";s:15:"/home/flag/flag";}'

    - the var_unserializer code thinks it's an object with possible __wakeup,
    so no ->unserialize method is called, and no __wakeup as it's not
    implemented. If it would call ->unserialize(), it would lead to a similar
    error triggered from zend_class_unserialize_deny

    Result - there's a C struct with unititialized members which would lead to
    crashes when accessed. That will affect any internal class implementing
    Serializable. The warning is just an indication that a class implementing
    Serializable was passed with O: format and not with C:. That class per
    definition shouldn't even have __wakeup as it implements Serializable,
    while it's not disallowed.
    The patches fixes it globally preventing incomplete objects due to the
    incorrect callback invocation. A fix for that particular ticket could be
    This also breaks code that worked before, and does it in stable
    versions. I do not think it is an acceptable solution for stable versions.
    Can't we make the fix in a way that does not have BC breakage
    for stable versions?
    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 -
    Only if that code is going to be run on 5.6.0 - which it won't be for
    some time because nobody runs .0 versions in production next day they are
    released. But even with 5.6, we are trying to minimize amount of code
    we're breaking in minor releases, and here we're breaking phpunit, which
    is one of the most used PHP tools. So I think we need to look for a better
    solution. CCing also Sebastian, maybe he could explain more about this
    hack and how it can be made to work more smoothly maybe. --
    Yeah, Sebastian knows that at best. Generally, if no userspace fix is
    possible, one could still revert and put __wakeup() to SplFileObject
    class, while as mentioned it's discards the doc and brings possible
    vulnerability back. At least 5.6.0 should have that fix IMHO, or master.

    Cheers

    Anatol

Related Discussions

Discussion Navigation
viewthread | post
Discussion Overview
groupphp-internals @
categoriesphp
postedMay 5, '14 at 8:21p
activeMay 6, '14 at 9:02a
posts4
users2
websitephp.net

2 users in discussion

Stas Malyshev: 2 posts Anatol Belski: 2 posts

People

Translate

site design / logo © 2022 Grokbase