Serialize/deserialize loses some data

Description

Serialization and deserialization of slightly complex structure with one object placed inside multiple other objects causes data loss.
I have created a reproducer https://3v4l.org/2m3Jb

It is quite easy to avoid this bug. For example by removing the __sleep function. It took me quite a while to prepare a working reproducer, even slight changes stops triggering the problem.

<?php

assert(is_array($data->second->items[0]->children));

$data = unserialize(serialize($data));

assert(is_array($data->second->items[0]->children));

It could be similar problem to 12265 but 12265 only affects PHP >=8.1. This bug is probably present in even older releases, but I did not make the reproducer to run in such old versions.

PHP Version

All versions >= PHP 7.4, probably even older

Operating System

No response

More minimal reproduction: https://3v4l.org/2Kh52

<?php

class Item {
    public ?array $children = [];
    public ?Item $parent = null;

    public function __sleep() {
        return ["children", "parent"];
    }
}

$baseProduct = new Item();

$child = new Item();
$child->parent = $baseProduct;
$baseProduct->children = [ $child ];

$data = [clone $baseProduct, $baseProduct];

assert(is_array($data[1]->children));

$data = unserialize(serialize($data));

assert(is_array($data[1]->children));

echo 'OK';

Bisected to 90fea67

@iluuu1994

I understand the problem.
When you clone the $baseProduct object, it's only a shallow clone: that means the children array points to the same array as the original $baseProduct object.
So when we visited the arrays, we visit the children array twice: once reachable via children and once via children->parent->children. The recursion protection then triggers and puts a NULL instead of an array in the serialized output. However, objects and references are "deduplicated" using the r:<number>/R:<number> notation, so everything works fine if we remove recursion protection...
It makes me wonder why we have this array recursion protection at all. I can see it was originally added via f0cf877 but that's no longer necessary because reading $GLOBALS is now always a copy.

Making self-recursive arrays should only be possible via references as arrays separate themselves on write. Although I vaguely remember some edge case bug where we could put an array in itself without references.

This works for me and passes all tests:

diff --git a/ext/standard/var.c b/ext/standard/var.c
index 3d53ebf5137..745997b04fd 100644
--- a/ext/standard/var.c
+++ b/ext/standard/var.c
@@ -951,18 +951,11 @@ static void php_var_serialize_nested_data(smart_str *buf, zval *struc, HashTable
 			/* we should still add element even if it's not OK,
 			 * since we already wrote the length of the array before */
 			if (Z_TYPE_P(data) == IS_ARRAY) {
-				if (UNEXPECTED(Z_IS_RECURSIVE_P(data))
-					|| UNEXPECTED(Z_TYPE_P(struc) == IS_ARRAY && Z_ARR_P(data) == Z_ARR_P(struc))) {
+				if (UNEXPECTED(Z_TYPE_P(struc) == IS_ARRAY && Z_ARR_P(data) == Z_ARR_P(struc))) {
 					php_add_var_hash(var_hash, struc, in_rcn_array);
 					smart_str_appendl(buf, "N;", 2);
 				} else {
-					if (Z_REFCOUNTED_P(data)) {
-						Z_PROTECT_RECURSION_P(data);
-					}
 					php_var_serialize_intern(buf, data, var_hash, in_rcn_array, false);
-					if (Z_REFCOUNTED_P(data)) {
-						Z_UNPROTECT_RECURSION_P(data);
-					}
 				}
 			} else {
 				php_var_serialize_intern(buf, data, var_hash, in_rcn_array, false);

@iluuu1994 You worked on serialization before. Do you see why this array recursion protection would still be necessary?

@iluuu1994

I have done a bit checking and don't see a reason why it's there but would be good to get @iluuu1994 opinion as he knows more about the engine.

I also noticed that serialization doesn't actually have any guards that json and var_dump have to cover this:

<?php


class A {
    function __serialize() {
        global $o;
        return ['o' => serialize($o)];
    }
}

$o = new A;
echo serialize($o);

But it is probably not necessarily a big issue as it no longer segfaults (it is caught by the stack check) and such code is quite unlikely (probably less likely than in json).

Although I vaguely remember some edge case bug where we could put an array in itself without references.

Might be this one: GH-10085

We already check for stack overflows in php_var_serialize_intern(), so removing the recursion check might be ok. As Niels said, arrays should not be self-recursive without either a reference or object. While it may be possible to construct them in some hacky ways (e.g. GH-10085), it shouldn't be possible to do so in user-provided data.

Yes it was indeed that issue!
Indeed since bd724bd (8.3.13) we have an overflow check.
So I'll remove the check and target 8.3. Thanks for the input.

Merge branch 'PHP-8.3' into PHP-8.4

Merge branch 'PHP-8.4'

Fix GH-19701: Serialize/deserialize loses some data

Navigation Menu

Repository navigation

Serialize/deserialize loses some data #19701

Description

PHP Version

Operating System

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Serialize/deserialize loses some data #19701

Description

Description

PHP Version

Operating System

Activity

DanielEScherzer commented on Sep 4, 2025

ndossche commented on Sep 5, 2025

ndossche commented on Sep 6, 2025

bukka commented on Sep 8, 2025

iluuu1994 commented on Sep 8, 2025

ndossche commented on Sep 8, 2025

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions