Make igbinary_serialize()/igbinary_unserialize() match serialize()/unserialize() again - #423
Open
nicolas-grekas wants to merge 9 commits into
Open
nicolas-grekas wants to merge 9 commits into
nicolas-grekas wants to merge 9 commits into
Conversation
Hold a reference to the array while walking it, like serialize() does since php/php-src#22714: a hook that appends to the array through a PHP reference reallocated it in place and the iterator then read freed memory. Also release the properties table on error paths. It leaked when a nested value failed to serialize, which made igbinary_serialize_deep_nesting.phpt report leaks on debug builds.
Port php_var_serialize_get_sleep_props(): uninitialized typed properties are skipped instead of throwing (igbinary#273, igbinary#377), missing properties are skipped with a warning on PHP 8+, duplicate names are reported, and a __sleep() that doesn't return an array serializes the object as null. Warning levels and messages follow the running PHP version. Objects serialized as null, by __sleep() or by Serializable::serialize() returning null, are now removed from the table of reference ids: igbinary_unserialize() doesn't count nulls, so later back-references pointed to the wrong value or failed with "invalid reference".
Port the optimization serialize() got in PHP 8.1: objects that have no properties table are serialized from their property slots. Building the table left it allocated for the lifetime of each object, which made memory usage grow after igbinary_serialize() (igbinary#402). Uninitialized typed properties are now skipped instead of being written as null padding, so the property count matches serialize() (igbinary#272).
unserialize_max_depth was checked for every value, including scalars, so igbinary_unserialize() failed one level earlier than unserialize(). The depth is now increased when entering a non-empty array or an object. References to references, which igbinary_serialize() never emits, are rejected so that recursion stays bounded by the depth limit.
Reject empty class names, names with a leading NUL byte or namespace separator, and names that are not valid class names, instead of creating an incomplete class or resolving another class. Stop when an autoloader throws, and lock the serializer state around autoloading and unserialize_callback_func like unserialize() does. Also fail with "Erroneous data format" when properties are provided for a class that implements Serializable without __unserialize(): such classes can only be created by their own unserialize() method.
igbinary_unserialize() now accepts the same $options as unserialize() (igbinary#123). Classes that are not allowed are unserialized as __PHP_Incomplete_Class objects and enums are not affected, like with unserialize(). The C API is unchanged.
PHP 8.6 allows readonly properties to have a default value, and unserialize() marks them as reinitializable while calling __unserialize() (php/php-src#22588). Do the same, otherwise __unserialize() throws "Cannot modify readonly property".
Some failure paths released the partially unserialized value right away. That ran destructors in the middle of unserialization and freed objects still listed for a deferred __wakeup() or __unserialize() call, which igbinary_unserialize_data_deinit() then wrote to. A truncated __unserialize() payload was also deferred while uninitialized. The partial value is now released once igbinary_unserialize() is done, after objects that won't be woken up are marked as destructed, like unserialize() does for the object that failed. Typed properties whose value failed to unserialize are reset to their default, like php/php-src#22481 does, and the serializer state is locked around __wakeup().
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
serialize() and unserialize() changed quite a bit since PHP 7.4 and igbinary didn't follow, so that it's not a drop-in replacement anymore. This PR ports those changes, one commit per topic, best reviewed commit by commit:
__sleep()skips uninitialized typed properties instead of throwing, skips missing properties, reports duplicate names, and serializes the object as null when it doesn't return an array, with the warnings of the running PHP versionSerializable::serialize()returns null) don't shift the ids of later back-references anymore: such payloads couldn't be unserializedserialize()does since 8.1, instead of allocating a table that stays attached to every objectigbinary_unserialize()accepts theallowed_classesandmax_depthoptions, and counts the depth on arrays and objects onlySerializable-only classes are checked likeunserialize()does__unserialize()can overwrite readonly defaults on PHP 8.6__wakeup()/__unserialize()callsThe format doesn't change; some payloads get smaller since uninitialized properties aren't padded with nulls anymore.
Fix #123, fix #272, fix #273, fix #377, fix #402