Improve error handling for unserialize()

[jrf]

From https://core.trac.wordpress.org/ticket/59231:

​Make unserialize() emit a warning for trailing bytes

While based on the current test suite, WP is not directly affected by this, the ​`maybe_unserialize()` function could still be confronted by data with trailing bytes.

However, the call to the PHP native unserialize() within maybe_unserialize() silences all (PHP 8.0+: non-fatal) errors, so this new warning will not affect WP or its ecosystem as long as the maybe_unserialize() function is used.

Having said that, a critical look at maybe_unserialize() may be warranted as the new warning in PHP is related to security issues discovered in other projects, so WP may want to consider rejecting unserialization for data throwing this warning.

Also note that there are 7 uses of unserialize() in total within WP Core, one within maybe_unserialize(), but the function is also used in 6 other places and 5 of those do not use error silencing.

​Improve unserialize() error handling

This, again, affects the ​`maybe_unserialize()` function and this time, the code should probably be adjusted to handle the new errors which unserialize() can now throw.

The change does not affect unserializing valid data, but in the case of invalid data, the type of and severity of the notices/warnings/catchable exceptions have been changed.

All 7 uses of unserialize() in WP Core should be reviewed and for the 6 uses outside of the maybe_unserialize() function, it should be reviewed whether they can/should switch to using maybe_unserialize() and/or whether they should get their own (improved) error handling.

[hellofromTonya]

6.4 RC1 is happening a few hours. Moving this to 6.5.

[chaion07]

Thanks @jrf for reporting this. We reviewed this ticket during a recent bug-scrub session. Based on the feedback received we are making the following changes:

1. Considering the lack of progress with this ticket we are in n favor of updating the milestone to 6.6
2. However this is a PHP Compatibility ticket (and a Task) which often only get attention late in a cycle, punting too early may delay the ticket getting necessary discussion. That said, Tonya is active and may pull it back into 6.5 if that's appropriate

Cheers!

Props to @costdev & @mukesh27

[azaozz]

a critical look at maybe_unserialize() may be warranted as the new warning in PHP is related to security issues discovered in other projects

+1. As a minimum thinking that the warnings from unserialize() should not be silenced when WP is in development mode (see ​https://developer.wordpress.org/reference/functions/wp_is_development_mode/).

Also thinking it makes sense to use maybe_unserialize() instead of unserialize() in more places/as appropriate as an attempt to maintain backwards compatibility (no warnings) in production in PHP 8.0+.

[hellofromTonya]

With the last scheduled 6.6 beta is next week, moving this ticket to the next major. This ticket needs more time for discussion and investigation to determine its resolution.

[chaion07]

Thanks @jrf for reporting this. As we move closer to the RC1 for 6.7 in a matter of days and with no progress in this cycle, we are updating the milestone based on the feedback received from a recent bug-scrub session. Thanks.

Props to @peterwilsoncc

Cheers!

[johnbillion]

Removing the php-compatibility focus because this is an enhancement to error handling that isn't needed in order for PHP 8.3 compatibility to be finalised.

This needs a patch in order to progress.

Trac ticket: https://core.trac.wordpress.org/ticket/59233

[dmsnell]

I left some notes in the linked PR, but I wanted to raise another two issues not addressed alone by setting the error handler. First of all though, thanks @azaozz for the tip about development mode. That’s a nice enhancement.

1. unserialize() can also throw after unserializing in a way not caught by the error handler. this happens when class instances are serialized and then throw during their __wakeup() invocation (or if any other related code runs and throws during the wakeup).
2. it’s often useful to know the difference between failing to unserialize and successfully unserializing a false value.

The first point can be handled by wrapping the call to unserialize() with a try/catch. For the second point I have found it useful to pass in an optional out param set as a boolean to indicate success or failure.

<?php

$did_unserialize = false;
$data = maybe_unserialize( 'b:0;', $did_unserialize );

$did_unserialize === true;
$data === false;

Now with a failing-to-unserialize class…

<?php
class Breakawake {
        public function __wakeup() {
                throw new Error( 'uh oh' );
        }
}

$did_unserialize = false;
$data = maybe_unserialize( 'O:10:"Breakawake":0:{}', $did_unserialize );

$did_unserialize === false;
$data === false;

The same logic could apply as well to non-existent classes during unserialization, as unserialize() returns an instance of __PHP_Incomplete_Class when the class being unserialized doesn’t exist (configurable through the unserialize_callback_func directive and the allowed_classes option).

Although these suggestions all present changes of behavior, the maybe_serialize() function would not break the call-sites and the only cases where the behavior would change is where the unserialization fails in some way, in ways not currently detected. I think it’s within the intent of the function and this ticket to improve how maybe_unserialize() detects error in addition to error handling, though a separate ticket would also be fine as an enhancement.

[dmsnell]

Apologies for the immediate follow-up, but one thing that passing in a &$did_unserialize argument could enable is skipping the check for is_serialized() which seems like it is guaranteed to fail in certain cases as serialization forms change. Maybe this could happen anyway, but the check of a user-defined is_serialized() seems unnecessary when we know that unserialize() will report if it fails.