Serialize/deserialize loses some data

[cuchac]

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

[DanielEScherzer]

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';

[nielsdos]

Bisected to 90fea67

[nielsdos]

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?

[bukka]

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).

[iluuu1994]

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.

[nielsdos]

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.