Skip to content

Handle indirect zvals in SplFixedArray::__serialize #10925

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
wants to merge 2 commits into from
Closed

Handle indirect zvals in SplFixedArray::__serialize #10925

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
67eb210
Handle indirect zvals in SplFixedArray::__serialize
nielsdos Mar 24, 2023
fb8a469
Fix an additional problem
nielsdos Mar 27, 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
24 changes: 11 additions & 13 deletions ext/spl/spl_fixedarray.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -597,8 +597,8 @@ PHP_METHOD(SplFixedArray, __serialize)
RETURN_THROWS();
}

uint32_t num_properties =
intern->std.properties ? zend_hash_num_elements(intern->std.properties) : 0;
HashTable *ht = zend_std_get_properties(&intern->std);
uint32_t num_properties = zend_hash_num_elements(ht);
array_init_size(return_value, intern->array.size + num_properties);

/* elements */
Expand All @@ -609,17 +609,15 @@ PHP_METHOD(SplFixedArray, __serialize)
}

/* members */
if (intern->std.properties) {
ZEND_HASH_FOREACH_STR_KEY_VAL(intern->std.properties, key, current) {
/* The properties hash table can also contain the array elements if the properties table was already rebuilt.
* In this case we'd have a NULL key. We can't simply use the properties table in all cases because it's
* potentially out of sync (missing elements, or containing removed elements) and might need a rebuild. */
if (key != NULL) {
zend_hash_add_new(Z_ARRVAL_P(return_value), key, current);
Z_TRY_ADDREF_P(current);
}
} ZEND_HASH_FOREACH_END();
}
ZEND_HASH_FOREACH_STR_KEY_VAL_IND(ht, key, current) {
/* If the properties table was already rebuild, it will also contain the
* array elements. The array elements are already added in the above loop.
* We can detect array elements by the fact that their key == NULL. */
if (key != NULL) {
Copy link
Member

Choose a reason for hiding this comment

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

Can you please describe me when the key is null? I wasn't be able to understand it 100% based on the comment above

Copy link
Member Author

Choose a reason for hiding this comment

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

I see I removed the comment, I need to place that back.

In any case: if var_dump (or something else that rebuilds the properties table) has been called before serialize(), then the properties table will include entries for the array elements. In those cases the key == NULL, and h == the index of the element.

For example:

<?php
$test = new SplFixedArray(2);
var_dump($test);
var_dump(serialize($test)); // If you add a printf to the foreach loop you'll see that the entries for array index 0 and 1 are also printed. But this does not happen if you remove the preceding var_dump, because then the properties table hasn't been rebuilt yet.

Copy link
Member Author

Choose a reason for hiding this comment

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

Also the difference between this version of the patch and the original is the following:
In the original patch I only added support for the INDIRECT zvals by appending _IND.
Shortly after merging I noticed a problem, so I reverted the merge and re-opened this PR.
The problem I noticed is that the code was using the wrong HashTable, their entries could still be stale (see the most recent commit which adds a testcase for this).

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the explanation! The new code comment is very helpful!

zend_hash_add_new(Z_ARRVAL_P(return_value), key, current);
Z_TRY_ADDREF_P(current);
}
} ZEND_HASH_FOREACH_END();
}

PHP_METHOD(SplFixedArray, __unserialize)
Expand Down
4 changes: 2 additions & 2 deletions ext/spl/tests/gh10907.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,10 +112,10 @@ object(SplFixedArray)#1 (3) {
}
=================
Test with adding members
string(161) "O:15:"MySplFixedArray":5:{i:0;s:12:"test value 1";i:1;s:12:"test value 2";i:2;N;s:9:"my_string";i:0;s:19:"my_dynamic_property";s:25:"my_dynamic_property_value";}"
string(180) "O:15:"MySplFixedArray":5:{i:0;s:12:"test value 1";i:1;s:12:"test value 2";i:2;N;s:9:"my_string";s:15:"my_string_value";s:19:"my_dynamic_property";s:25:"my_dynamic_property_value";}"
object(MySplFixedArray)#1 (5) {
["my_string"]=>
int(0)
string(15) "my_string_value"
["my_dynamic_property"]=>
string(25) "my_dynamic_property_value"
[0]=>
Expand Down
69 changes: 69 additions & 0 deletions ext/spl/tests/gh10925.phpt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,69 @@
--TEST--
Properties serialization for SplFixedArray should have updated properties
--FILE--
<?php
#[AllowDynamicProperties]
class MySplFixedArray extends SplFixedArray {
public $x;
public int $y = 3;
}

$x = new MySplFixedArray(2);
var_dump($x->y);
$x->y = 2;
var_dump($x->y);
$serialized = serialize($x);
var_dump($serialized);
var_dump(unserialize($serialized));

$x->dynamic_property = "dynamic_property_value";
$serialized = serialize($x);
var_dump($serialized);
var_dump(unserialize($serialized));

$x->dynamic_property = "dynamic_property_value2";
$x->y = 4;
$serialized = serialize($x);
var_dump($serialized);
var_dump(unserialize($serialized));
?>
--EXPECT--
int(3)
int(2)
string(61) "O:15:"MySplFixedArray":4:{i:0;N;i:1;N;s:1:"x";N;s:1:"y";i:2;}"
object(MySplFixedArray)#2 (4) {
["x"]=>
NULL
["y"]=>
int(2)
[0]=>
NULL
[1]=>
NULL
}
string(115) "O:15:"MySplFixedArray":5:{i:0;N;i:1;N;s:1:"x";N;s:1:"y";i:2;s:16:"dynamic_property";s:22:"dynamic_property_value";}"
object(MySplFixedArray)#2 (5) {
["x"]=>
NULL
["y"]=>
int(2)
["dynamic_property"]=>
string(22) "dynamic_property_value"
[0]=>
NULL
[1]=>
NULL
}
string(116) "O:15:"MySplFixedArray":5:{i:0;N;i:1;N;s:1:"x";N;s:1:"y";i:4;s:16:"dynamic_property";s:23:"dynamic_property_value2";}"
object(MySplFixedArray)#2 (5) {
["x"]=>
NULL
["y"]=>
int(4)
["dynamic_property"]=>
string(23) "dynamic_property_value2"
[0]=>
NULL
[1]=>
NULL
}