Skip to content

Fix GH-12265: Cloning an object breaks serialization recursion #12287

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: PHP-8.1
Choose a base branch
from
Open

Fix GH-12265: Cloning an object breaks serialization recursion #12287

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0a55764
Fix GH-12265: Cloning an object breaks serialization recursion
nielsdos Sep 23, 2023
60f05cf
__sleep too
nielsdos Sep 25, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 47 additions & 0 deletions ext/standard/tests/serialize/gh12265.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
--TEST--
GH-12265 (Cloning an object breaks serialization recursion)
--FILE--
<?php

class A {
public function __construct(public B $x) {
}
}

class B {
public A $a;

public function __serialize()
{
return ['a' => new A($this)];
}
}

class C {
public B $b;

public function __construct() {
$this->b = new B;
}
}

$b = new B();
$sb = serialize($b);
$stb = serialize(new B);

printf("serialized original: %s\n", $sb);
printf("serialized temp : %s\n", $stb);

$c = new C;
$sc = serialize($c);
$stc = serialize(new C);

printf("serialized original: %s\n", $sc);
printf("serialized temp : %s\n", $stc);

?>
--EXPECT--
serialized original: O:1:"B":1:{s:1:"a";O:1:"A":1:{s:1:"x";r:1;}}
serialized temp : O:1:"B":1:{s:1:"a";O:1:"A":1:{s:1:"x";r:1;}}
serialized original: O:1:"C":1:{s:1:"b";O:1:"B":1:{s:1:"a";O:1:"A":1:{s:1:"x";r:2;}}}
serialized temp : O:1:"C":1:{s:1:"b";O:1:"B":1:{s:1:"a";O:1:"A":1:{s:1:"x";r:2;}}}
4 changes: 3 additions & 1 deletion ext/standard/var.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -671,7 +671,9 @@ static inline zend_long php_add_var_hash(php_serialize_data_t data, zval *var, b
return 0;
} else if (!in_rcn_array
&& Z_REFCOUNT_P(var) == 1
&& (Z_OBJ_P(var)->properties == NULL || GC_REFCOUNT(Z_OBJ_P(var)->properties) == 1)) {
&& (Z_OBJ_P(var)->properties == NULL || GC_REFCOUNT(Z_OBJ_P(var)->properties) == 1)
/* __serialize may arbitrarily increase the refcount */
&& Z_OBJCE_P(var)->__serialize == NULL) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't this also a possible problem for __sleep()?

Copy link
Member Author

@nielsdos nielsdos Sep 25, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed, added a check for this and a test. Although the check is unfortunately more expensive than the one for __serialize, so we maybe need a different solution...
For example by calling var_add_hash before php_var_serialize_call_sleep when it hasn't been called before (or early-returned before).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So another follow-up question is, what happens if Serializable is used without __serialize() (other than a deprecation notice)?

We really should think about deprecating __sleep()/__wakeup() as it seems to me the functionality is covered by the new serialize magic methods.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So another follow-up question is, what happens if Serializable is used without __serialize() (other than a deprecation notice)?

I don't think this is a problem for this bug? But I could be wrong or misunderstanding the question.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIRC If Serializable is implemented, serialize() function will call the serialize() method of the object, similar to __sleep()/__serialize()

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That serialize methods is supposed to returns the string that will be embedded in serialize's output. As the bug manifests in the built-in reference tracking when it performs the default serialization, I don't think we'll have that issue in that case?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah so that's how it works, yeah should be fine then

return 0;
}

Expand Down