Skip to content

Make igbinary_serialize()/igbinary_unserialize() match serialize()/unserialize() again - #423

Open
nicolas-grekas wants to merge 9 commits into
igbinary:masterfrom
nicolas-grekas:serializer-parity
Open

nicolas-grekas wants to merge 9 commits into
igbinary:masterfrom
nicolas-grekas:serializer-parity

Conversation

@nicolas-grekas

Copy link
Copy Markdown

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:

  1. __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 version
  2. objects serialized as null (also when Serializable::serialize() returns null) don't shift the ids of later back-references anymore: such payloads couldn't be unserialized
  3. objects without a properties table are serialized from their slots, like serialize() does since 8.1, instead of allocating a table that stays attached to every object
  4. igbinary_unserialize() accepts the allowed_classes and max_depth options, and counts the depth on arrays and objects only
  5. class names, autoloader exceptions and properties sent to Serializable-only classes are checked like unserialize() does
  6. __unserialize() can overwrite readonly defaults on PHP 8.6
  7. two use-after-free: when a hook grows the array being serialized, and when unserialization fails after objects with pending __wakeup()/__unserialize() calls

The 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

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().
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment