Add fallback hydration for properties#411
Add fallback hydration for properties#411madmajestro wants to merge 1 commit intoigbinary:masterigbinary/igbinary:masterfrom madmajestro:fix-fallback-hydration-of-propertiesmadmajestro/igbinary:fix-fallback-hydration-of-propertiesCopy head branch name to clipboard
Conversation
|
I would suggest adding another test where both |
| int mangle_property_names = 0; | ||
|
|
||
| #if PHP_VERSION_ID >= 80000 | ||
| if (ce->__serialize) mangle_property_names = 1; |
There was a problem hiding this comment.
Have not yet reviewed actual substance yet, but for style consistency; please use an explicit comparison and braces:
if (ce->__serialize != NULL) {
...
}
Also please use tabs for indentation.
There was a problem hiding this comment.
I'm now using an explicit comparison and curly braces, but I've already used tabs in the C code and spaces in the PHPT tests, just like the other __serialize*.phpt tests do. Should I still use tabs in the tests?
Indeed this is very much required, though there are quite a few tests already. Tests can always be improved. |
157ec93 to
3973a6a
Compare
|
I've added a test to check the behavior of |
3973a6a to
7e01f54
Compare
|
I have just pushed an optimized version. This version uses the mangled representation of the property name already present in zend_property_info. This saves the memory allocation and string copying done in |
7e01f54 to
31f2cf3
Compare
|
@tricky |
31f2cf3 to
b15eb2a
Compare
|
I have now implemented the fallback hydration, as it exists in PHP's internal serializer since PHP 7.2. This should resolve several issues. In addition, the behavior of igbinary hopefully should now be identical to PHP's serializer, allowing it to be used again as a drop-in replacement. |
b15eb2a to
1d0cb26
Compare
|
@tricky |
1d0cb26 to
a78b797
Compare
|
I've re-analyzed the behavior of the PHP serializer and Igbinary once more and noticed a few edge cases where PHP and Igbinary behaved differently. I've fixed these issues and added / revised tests so that the behavior of PHP and Igbinary is now tested and compared during the tests. I hope this simplifies the review process and helps to identify differences between PHP and Igbinary in the future. |
|
Friendly ping |
|
Friendly Ping |
| if ((info->flags & ZEND_ACC_VIRTUAL)) { | ||
| php_error_docref(NULL, E_WARNING, | ||
| "Cannot unserialize value for virtual property %s::$%s", | ||
| ZSTR_VAL(info->ce->name), ZSTR_VAL(key_str)); |
There was a problem hiding this comment.
When this branch fires for a private or protected virtual property, key_str is still the mangled zend_string (e.g. \0Class\0prop for private, \0*\0prop for protected). ZSTR_VAL(key_str) then contains embedded NULs, and %s in php_error_docref truncates at the first NUL — the user sees Cannot unserialize value for virtual property Test::$ followed by garbage/nothing instead of the property name.
The included __serialize_025.phpt only exercises a public virtual property, so this case is not covered by tests. Suggest using info->name (the unmangled name from properties_info) or the local unmangled_prop:
"Cannot unserialize value for virtual property %s::$%s",
ZSTR_VAL(info->ce->name), ZSTR_VAL(info->name));There was a problem hiding this comment.
The test to add
--TEST--
Virtual-property warning is truncated when key was originally protected/private
--SKIPIF--
<?php if (PHP_VERSION_ID < 80400) { echo "skip virtual properties need PHP 8.4+"; } ?>
--FILE--
<?php
// Bytes generated once (PHP 8.5) from:
// class Test { protected int $prop = 1; }
// bin2hex(igbinary_serialize(new Test()));
// 17 04 "Test" | 14 01 (array,1) | 11 07 \0*\0prop | 06 01 (int 1)
$data = hex2bin('0000000217045465737414011107002a0070726f700601');
// Same class name, but `prop` is now a public *virtual* property.
class Test {
public int $prop {
get => 1;
}
}
igbinary_unserialize($data);
?>
--EXPECTF--
Warning: igbinary_unserialize(): Cannot unserialize value for virtual property Test::$prop in %s on line %d
There was a problem hiding this comment.
Thanks for suggesting a fix and a test!
|
Stylistic nit, not blocking — a few lines below the new fallback block, the existing code reassigns info = zend_get_typed_property_info_for_slot(Z_OBJ_P(z_deref), prototype_value);When the fallback above populated |
a78b797 to
fbbe73c
Compare
I am now using two different variables. That makes it clearer. |
fbbe73c to
5dc295d
Compare
If the visibility of a property has changed between serialization and unserialization, or if a private or protected property needs to be restored, but the serialized data was generated by a __serialize method while no __unserialize method exists in the class, the property value cannot be found during hydration in the object's hash table. The reason for this is that in these cases the property name in the serialized representation does not match the prefixed/mangled property name of the current class definition. To resolve this, an attempt is made to determine the correctly prefixed property name by performing a lookup with the unprefixed property name in the properties_info hash table of the class. The implementation is borrowed from the current version of php-src/ext/standard/var_unserializer.re to align the behavior to the serializer of PHP >= 7.2. See 7cb5bdf64a95bd70623d33d6ea122c13b01113bd / php/php-src#2494 for the initial implementation in PHP 7.2.
5dc295d to
aa92730
Compare
|
This issue is blocking our upgrade to PHP 8.5, is there anything blocking this being merged and a final 3.2.17 release being created? |
If the visibility of a property has changed between serialization and unserialization, or if a private or protected property needs to be restored, but the serialized data was generated by a __serialize method while no __unserialize method exists in the class, the property value cannot be found during hydration in the object's hash table. The reason for this is that in these cases the property name in the serialized representation does not match the prefixed/mangled property name of the current class definition. To resolve this, an attempt is made to determine the correctly prefixed property name by performing a lookup with the unprefixed property name in the properties_info hash table of the class.The implementation is borrowed from the current version of php-src/ext/standard/var_unserializer.re to align the behavior to the serializer of PHP >= 7.2. See 7cb5bdf64a95bd70623d33d6ea122c13b01113bd / php/php-src#2494 for the initial implementation in PHP 7.2.
Closes #410
Closes #387
Closes #156