Calling `unserialize()` inside `Serializable::unserialize()` allows the object to be manipulated before it has been restored.

[chongwick]

Description

The following code:

<?php
$v_116868 = 1;
$v_116869 = (int)($v_116868);
$v_116870 = error_reporting($v_116869,);
$v_116871 = '---------------- create, serialize, and unserialize a Point -------------------\n';
$v_116872 = 2;
$v_116897 = $value1;
$v_116898 = new BcMath\Number($v_116897,);
$v_116900 = $value2;
$v_116902 = (int)($v_116900);
$v_116903 = $v_116898 * $v_116902;
class Point implements Serializable
{
        private static $nextId = 1;

        private $x;
        private $y;
        private $id;

        public function __construct($x = 0, $y = 0)
        {
                $this->x = $x;
                $this->y = $y;
                $this->id = self::$nextId++;

                echo "\nInside " . __METHOD__ . ", $this\n\n";
        }

        public function __toString()
        {
                return 'ID:' . $this->id . '(' . $this->x . ',' . $this->y . ')';
        }

        public function serialize()
        {
                echo "\nInside " . __METHOD__ . ", $this\n\n";

                return serialize(array('y' => $this->y, 'x' => $this->x));
        }

    public function unserialize($data)
    {
                $data = unserialize($data);
                $this->x = $data['x'];
                $this->y = $data['y'];
                $this->id = self::$nextId++;

                echo "\nInside " . __METHOD__ . ", $this\n\n";
    }
}
$v_116874 = new Point($v_116872,$v_116903,);
$v_116875 = "Point $p = $v_116874\n";
$v_116876 = serialize($v_116874,);
$v_116877 = var_dump($v_116876,);
$v_116878 = '------\n';
$v_116879 = unserialize($v_116876,);

Resulted in this output:

Inside Point::__construct, ID:1(2,0)


Inside Point::serialize, ID:1(2,0)

string(88) "C:5:"Point":71:{a:2:{s:1:"y";O:13:"BcMath\Number":1:{s:5:"value";s:1:"0";}s:1:"x";i:2;}}"
/home/w023dtc/nightly_php/php-src/ext/bcmath/libbcmath/src/num2str.c:43:21: runtime error: member access within null pointer of type 'struct bc_struct'
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior /home/w023dtc/nightly_php/php-src/ext/bcmath/libbcmath/src/num2str.c:43:21

PHP Version

nightly

Operating System

ubuntu 20.04

[DanielEScherzer]

Minimal example to get a segfault on 3v4l:

<?php

class Point implements Serializable {
    public $y;

    public function __construct($y) { $this->y = $y; }

    public function serialize() {
        return serialize($this->y);
    }

    public function unserialize($data) {
        echo unserialize($data);
    }
}
$point = new Point(new BcMath\Number(0));
$ser = serialize($point);
var_dump($ser);
unserialize($ser);

produces

Deprecated: Point implements the Serializable interface, which is deprecated. Implement __serialize() and __unserialize() instead (or in addition, if support for old PHP versions is necessary) in /in/V52Vk on line 3
string(62) "C:5:"Point":45:{O:13:"BcMath\Number":1:{s:5:"value";s:1:"0";}}"

Process exited with code 139.

https://3v4l.org/V52Vk/rfc#vgit.master

[SakiTakamachi]

I haven’t looked into it in detail yet, but this is not a BCMath issue.
Even when I try with DateTime, the result still comes out wrong.

Most likely, there’s a code path where the object is only initialized but no values are ever set.

The error with BCMath occurs because it tries to access null when no value has been set, but that is not the essence of the problem.

https://3v4l.org/OKcNu

[SakiTakamachi]

In the Point::unserialize() method, calling the unserialize() function, and the increased nesting depth appears to prevent the __unserialize() methods of classes like Number and DateTime from being called correctly.

[SakiTakamachi]

Ah, I see.
Since __unserialize() is called only when the entire chain of unserialize() has finished, loading the object before that will break it.
The magic methods are properly controlled in order, but with Serializable, the order can sometimes break.

https://3v4l.org/R5Og9#v8.4.12

[SakiTakamachi]

@bukka
Are you familiar with this?
I tried various approaches, but it seems difficult to fix this in a simple way.
Do you think it would be better to fix it, or to raise an error in specific cases?

[bukka]

I will need to check and think about it but currently busy with other stuff so it will be few weeks...

[bukka]

I just checked it a bit and can see the issue (debugging is a bit pain though :) ). I can't really think about a good solution either and don't have really much extra time to spend more time on it this month. Maybe @iluuu1994 or @nielsdos have ideas.

[nielsdos]

The first thing that came to mind is protecting the call in zend_user_unserialize with incrementing the BG(serialize_lock). And yes, that fixes the issue because then the object instantiation isn't delayed until the outer unserialize finishes. However, that causes issues because apparently some state of the outer unserialize also needs to be applied to the inner ones (see e.g. ext/standard/tests/serialize/bug72785.phpt). So it's not a super easy fix.