php.net |  support |  documentation |  report a bug |  advanced search |  search howto |  statistics |  random bug |  login

go to bug id or search bugs for

Doc Bug #68099 Unable to unserialize data after implementing Serializable
Submitted: 2014-09-25 16:23 UTC Modified: 2015-04-06 09:24 UTC
Votes:4
Avg. Score:5.0 ± 0.0
Reproduced:4 of 4 (100.0%)
Same Version:1 (25.0%)
Same OS:0 (0.0%)
From: manuel-php at mausz dot at Assigned: tyrael (profile)
Status: Wont fix Package: *General Issues
PHP Version: 5.6.1RC1 OS:
Private report: No CVE-ID: None
View Developer Edit
Welcome! If you don't have a Git account, you can't do anything here.
If you reported this bug, you can edit this bug over here.
(description)
Block user comment
Status: Assign to:
Package:
Bug Type:
Summary:
From: manuel-php at mausz dot at
New email:
PHP Version: OS:

 

 [2014-09-25 16:23 UTC] manuel-php at mausz dot at 
Description:
------------
Consider the following case:
* User implements class Foo which gets serialized and unserialized during daily use. Serialized data is stored somewhere.
* User changes class Foo to implement Serializable
* User still wants to transparently unserialize his old data

This worked in PHP5.5 and below. It's broken in PHP 5.6.0 and above.

If this is intended than this is a major BC break not mentioned in the upgrade guidelines.

Test script:
---------------
<?php

error_reporting(E_ALL);

# export ... simulates exporting old data where class Foo didn't implement Serializable
$mode = (isset($argv[1]) && $argv[1] === 'import') ? 'import' : 'export';

if ($mode === 'export')
{
  class Foo
  {
    protected $foo = null;

    public function __construct()
    {
      $this->foo = "something";
    }
  }

  $foo = new Foo();
  $data = serialize($foo);
  file_put_contents('/tmp/phpbug', $data);
}
else
{
  class Foo implements Serializable
  {
    protected $foo = null;

    public function __construct()
    {
      $this->foo = "something";
    }

    public function serialize()
    {
      echo __CLASS__ . "::serialize called\n";
      return $this->foo;
    }

    public function unserialize($data)
    {
      echo __CLASS__ . "::unserialize called\n";
      $this->foo = $data;
    }
  }

  $data = file_get_contents('/tmp/phpbug');
  $foo = unserialize($data);
  if ($foo instanceof Foo)
    echo "Thumbs up!\n";
  else
    echo "Unserialize FAILED\n";
}


Expected result:
----------------
# php5.5 serialize.php export; php5.5 serialize.php import
Thumbs up!

Actual result:
--------------
 ./php-src-PHP-5.6.1/sapi/cli/php serialize.php export ; ./php-src-PHP-5.6.1/sapi/cli/php serialize.php import

Warning: Erroneous data format for unserializing 'Foo' in /home/manuel/serialize.php on line 49

Notice: unserialize(): Error at offset 13 of 43 bytes in /home/manuel/serialize.php on line 49
Unserialize FAILED

Patches

Pull Requests

History

AllCommentsChangesGit/SVN commitsRelated reports
 [2014-09-25 20:31 UTC] manuel-php at mausz dot at 
Reverting c2acdbdd3deb6787329bf0aca8ab0c04ace2a50c fixes this issue
 [2014-10-15 16:25 UTC] tyrael@php.net
-Type: Bug +Type: Documentation Problem
 [2014-10-15 16:25 UTC] tyrael@php.net 
hi,

classes implementing the Serializable interface are using a different serialize format than used for other classes.
unfortunately when this new format was introduced, there were no checks implemented to enforce this behavior, so it was possible to unserialize an instance implementing the Serialize interface using the old format, which bypassed the call for the unserialize method declared for that class.
with 5.6 we implemented these checks to enforce that the unserialize calls can't be bypassed using the old serialize format.
while what you are doing in your example is pretty risky, as it is prone to result in an bogus instance(the only case where it would produce the correct result is that if your class' unserialize handler writes the same properties and does no other transformation on the data), I think you are still right that we should explicitly state this behavior change in the migration guide(currently we only mention the change in the unserialize manual: http://php.net/manual/en/function.unserialize.php ).
 [2014-10-15 16:50 UTC] manuel-php at mausz dot at 
Hi Ferenc,

please don't consider the test script to be a something that's in production.

In fact we stumbled across this while upgrading a webserver running horde webmail. Horde upgraded a bunch of classes to implement Serializable (https://github.com/horde/horde/commit/bc36d596a868602f525d2cc57e9de1155a23456e) about a year back. So any data that was serialized before this change is broken with PHP 5.6.

I consider this a major BC break for a minor PHP version, something that is hard to find and almost impossible to fix at the appplication level. You can catch the failed unserialize()-call but you can't unserialize/convert the data unless you implement the serialize format in PHP.

Also c2acdbdd3deb6787329bf0aca8ab0c04ace2a50c doesn't reintroduce #67072. I fail to see why this can't be reverted (or fixed correctly).
 [2014-10-15 18:32 UTC] tyrael@php.net 
I agree that this is a BC break, but the original behavior (that we allow the old serialize format to be used for classes using the new format) was never documented nor officially supported, but I would still vote to keep it for now if this behavior couldn't cause potential security problems,
https://bugs.php.net/bug.php?id=67072 was the initial bugreport which made us realize that this trick which allows the unserialize call to be bypassed for classes using the new serialize format(classes which are implementing Serialize) can be potentially used to cause segfaults or even more dangerous problems, and there are lots of php apps out there which allows unserialization of arbitrary so that plugging this hole was warranted.

if you have an idea how to fix this "properly", I'm all ears, we had some really lengthy discussion on the mailing list and couldn't come up with a better approach.
(for the record we introduced a less strict check for 5.4 and 5.5 where we only reject the unserialization of the Serializable classes with the old format where the class is or extending an internal class which can cause memory corruptions like the one described in the original bugreport).

if you still feel that this is too much of a BC impact or if you have a better suggestion, please bring this on the internals@ mailing list, where more people can join the discussion.
 [2014-10-17 09:24 UTC] tyrael@php.net
-Assigned To: +Assigned To: tyrael
 [2015-04-06 09:24 UTC] tyrael@php.net
-Status: Assigned +Status: Wont fix
 
PHP Copyright © 2001-2025 The PHP Group
All rights reserved. 		Last updated: Fri Apr 25 11:01:29 2025 UTC