Skip to content

Skip uninitialized typed properties when serializing objects#5396

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

Closed
nicolas-grekas wants to merge 1 commit into from
Closed

Skip uninitialized typed properties when serializing objects #5396

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
1 change: 1 addition & 0 deletions NEWS
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -6,6 +6,7 @@ PHP NEWS
- Core:
. Fixed bug #78434 (Generator yields no items after valid() call). (Nikita)
. Fixed bug #79477 (casting object into array creates references). (Nikita)
. Fixed bug #79447 (Serializing uninitialized typed properties with __sleep should not throw). (nicolas-grekas)

- DOM:
. Fixed bug #78221 (DOMNode::normalize() doesn't remove empty text nodes).
Expand Down
38 changes: 13 additions & 25 deletions ext/standard/tests/serialize/sleep_uninitialized_typed_prop.phpt
View file
Open in desktop
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
--TEST--
Referencing an uninitialized typed property in __sleep() should result in Error
Referencing an uninitialized typed property in __sleep() should be skipped
--FILE--
<?php

Expand All@@ -18,39 +18,27 @@ class Test {
}

$t = new Test;
try {
serialize($t);
} catch (Error $e) {
echo $e->getMessage(), "\n";
}
var_dump(serialize($t));
var_dump(unserialize(serialize($t)) == $t);

$t->x = 1;
try {
serialize($t);
} catch (Error $e) {
echo $e->getMessage(), "\n";
}
var_dump(unserialize(serialize($t)) == $t);

$t->y = 2;
try {
serialize($t);
} catch (Error $e) {
echo $e->getMessage(), "\n";
}
var_dump(unserialize(serialize($t)) == $t);

$t->z = 3;
try {
var_dump(unserialize(serialize($t)));
} catch (Error $e) {
echo $e->getMessage(), "\n";
}
var_dump(unserialize(serialize($t)) == $t);

var_dump($t);
?>
--EXPECT--
Typed property Test::$x must not be accessed before initialization (in __sleep)
Typed property Test::$y must not be accessed before initialization (in __sleep)
Typed property Test::$z must not be accessed before initialization (in __sleep)
object(Test)#3 (3) {
string(15) "O:4:"Test":0:{}"
bool(true)
bool(true)
bool(true)
bool(true)
object(Test)#1 (3) {
["x"]=>
int(1)
["y":protected]=>
Expand Down
4 changes: 1 addition & 3 deletions ext/standard/var.c
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -784,9 +784,7 @@ static int php_var_serialize_try_add_sleep_prop(
if (Z_TYPE_P(val) == IS_UNDEF) {
zend_property_info *info = zend_get_typed_property_info_for_slot(Z_OBJ_P(struc), val);
if (info) {
zend_throw_error(NULL,
"Typed property %s::$%s must not be accessed before initialization (in __sleep)",
ZSTR_VAL(Z_OBJCE_P(struc)->name), ZSTR_VAL(error_name));
return SUCCESS;

@TysonAndreTysonAndreApr 16, 2020

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If this is going to return success, it might make some sense to add a placeholder such as IS_UNDEF zend_hash_add(ht, name, undef_zval) when building this, then check for IS_UNDEF when using the resulting table

Otherwise, __sleep will inconsistently fail to warn about return ['prop', 'prop']; when prop is an uninitialized typed property

@nicolas-grekasnicolas-grekasApr 16, 2020

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

How would that not break on unserialize(), since that's the point of this PR?

@nikicnikicApr 20, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I don't think trying to preserve that warning is worth the complications (using IS_UNDEF for this is not possible -- IS_UNDEF in hashtable means the element doesn't exist).

}
return FAILURE;
}
Expand Down