WordPress.org

Make WordPress Core

Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#26521 closed enhancement (duplicate)

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

Reported by: kkarpieszuk Owned by:
Milestone: Priority: normal
Severity: normal Version: 3.7
Component: Performance Keywords:
Focuses: Cc:

Description

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.

Attachments (1)

maybe_unserialize.php (342 bytes) - added by kkarpieszuk 5 years ago.
full code of proposed function

Download all attachments as: .zip

Change History (6)

@kkarpieszuk
5 years ago

full code of proposed function

#1 in reply to: ↑ description @nacin
5 years ago

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.

#2 @kkarpieszuk
5 years ago

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

#3 @kkarpieszuk
5 years ago

  • Resolution set to invalid
  • Status changed from new to closed

#4 @markoheijnen
5 years ago

  • Milestone Awaiting Review deleted

#5 @SergeyBiryukov
5 years ago

  • Resolution changed from invalid to duplicate
  • Version changed from trunk to 3.7

Duplicate of #16504.

Note: See TracTickets for help on using tickets.