Opened 3 months ago
Last modified 7 weeks ago
#59233 new task (blessed)
Improve error handling for unserialize()
Reported by: |
|
Owned by: | |
---|---|---|---|
Milestone: | 6.5 | Priority: | normal |
Severity: | normal | Version: | |
Component: | General | Keywords: | php83 2nd-opinion |
Focuses: | php-compatibility | Cc: |
Description
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()
withinmaybe_unserialize()
silences all (PHP 8.0+: non-fatal) errors, so this new warning will not affect WP or its ecosystem as long as themaybe_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 withinmaybe_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 themaybe_unserialize()
function, it should be reviewed whether they can/should switch to usingmaybe_unserialize()
and/or whether they should get their own (improved) error handling.
6.4 RC1 is happening a few hours. Moving this to 6.5.