maybe_unserialize() don't need to use is_serialized()

[kkarpieszuk]

Hi, this is my first submission to Trac, so I hope I will fo everything well ;)

I am profiling (with xdebug) our plugin (WPML) for performance bottlenecks and I stumbled onto some small issue in WordPress core.

I see that wp-includes/functions.php::maybe_unserialize() could be written to work faster. It doesn't actually need to run is_serialized(), because PHP checks this in PHP::unserialize().

My proposition of this function (added also as attachment):

function maybe_unserialize( $original ) {
    $unserialized = @unserialize($original);
    return $unserialized ? $unserialized : $original;
}

And now profiling tests on some site (tested on few others, number of course were different but relation is quite the same):

Original function:

function maybe_unserialize() was called 1776 times. It took 0.087393 second, self: 0.023378 s.

My function:

function maybe_unserialize() was called 1776 times. It took 0.031895 second, self: 0.020972 s.

[kkarpieszuk]

full code of proposed function

[nacin]

Replying to kkarpieszuk:

Hi, this is my first submission to Trac, so I hope I will fo everything well ;)

Welcome! I am all for submissions backed by XDebug :-)

I see that wp-includes/functions.php::maybe_unserialize() could be written to work faster. It doesn't actually need to run is_serialized(), because PHP checks this in PHP::unserialize().

Unfortunately we can't change how this function is written. is_serialized() operates as a guard to make sure that only things that had been serialized by maybe_serialize() are unserialized. If you can craft a string that maybe_serialize() won't re-serialize, but unserialize() will unserialize, then you open yourself up to object injection.

A possible improvement (for a new ticket) would be to lessen the number of times maybe_unserialize() gets called. I'd be all for reducing that from 1776 to more like once-per-unique-value. We call it with every get_option() call. In an ideal world, it only gets called after DB runs. Somewhat related is #23381, though.

[kkarpieszuk]

Ok, thank you for this lesson :) I didn't know that unserilize() is possible to be object injected. You can close this ticket

[SergeyBiryukov]

Duplicate of #16504.