Make WordPress Core

Opened 12 years ago

Last modified 22 months ago

#21650 new defect (bug)

replace serialize() with print_r() in stats() function in wp-includes/cache.php

Reported by: bobbingwide's profile bobbingwide Owned by:
Milestone: Future Release Priority: normal
Severity: normal Version: 3.4.1
Component: Cache API Keywords: has-patch needs-unit-tests
Focuses: Cc:

Description

In PHP 5.3 it is no longer possible to serialize data that contains a SimpleXMLElement object. It produces a fatal error. See https://bugs.php.net/bug.php?id=49800

The stats() function attempts to determine the allocated space for objects in the cache by using strlen() of the serialized object.

This can fail for the reason above.

Given that the figure returned is simply an estimation of the space
I propose that the code is changed to use
print_r( $cache, true ) instead of serialize( $cache )

ie. to become
echo "<li><strong>Group:</strong> $group - ( " . number_format( strlen( print_r( $cache, true ) ) / 1024, 2 ) . 'k )</li>';

This TRAC was raised after a longish chain of events starting with #18488 and the final response (today) which led to another chance discovery of a similar problem in the debug-bar plugin.

http://wordpress.org/support/topic/plugin-debug-bar-fatal-error-uncaught-exception-serialization-of-simplexmlelement-is-not-allowed

Attachments (6)

cache.php.21650.diff (564 bytes) - added by bobbingwide 12 years ago.
Replace serialize() with print_r() to avoid Fatal error (WP 3.5)
21650-try-catch.diff (692 bytes) - added by voldemortensen 10 years ago.
21650-try-catch-for-real.diff (690 bytes) - added by voldemortensen 10 years ago.
14157-ms-php.fixed.diff (1.5 KB) - added by voldemortensen 10 years ago.
21650-try-catch.fixed.diff (696 bytes) - added by voldemortensen 10 years ago.
21650.diff (889 bytes) - added by desrosj 2 years ago.

Download all attachments as: .zip

Change History (25)

@bobbingwide
12 years ago

Replace serialize() with print_r() to avoid Fatal error (WP 3.5)

#1 @scribu
12 years ago

  • Severity changed from normal to minor

The way this estimation is presented is very misleading; it makes you think you're viewing the size of the cache in memory, rather than the lenght of it's serialization.

#2 @scribu
12 years ago

Other than that, switching to print_r() seems sane.

#3 @wonderboymusic
12 years ago

  • Keywords has-patch added

#4 @wonderboymusic
11 years ago

  • Milestone changed from Awaiting Review to 3.7

#5 @nacin
11 years ago

How is a SimpleXMLElement object making its way into object cache? It should not be allowed in postmeta, options, etc., as those all serialize (even going into cache). I imagine it would only occur when you are using your own cache bucket?

Should we do a try/catch and fall back to print_r()? I'm fine with moving over to print_r() but it's probably even less of a decent representation of the approximate size in cache.

#6 follow-up: @ryan
11 years ago

A try/catch with fallback to print_r() seems sufficient to cover this edge case. serialize() does give us a better approximation.

#7 in reply to: ↑ 6 @nacin
11 years ago

  • Keywords needs-patch added; has-patch removed
  • Milestone changed from 3.7 to Future Release
  • Priority changed from normal to low

Replying to ryan:

A try/catch with fallback to print_r() seems sufficient to cover this edge case. serialize() does give us a better approximation.

Yeah, I agree. Happy to commit a patch that does this. Because this is merely a debugging situation, and it's a very particular issue, I'm pushing on this. Super edge.

#8 @voldemortensen
10 years ago

I was unable to duplicate the error, however I added a try/catch to try serialize and then print_r. Patch needs testing as I couldn't duplicate.

#9 @SergeyBiryukov
10 years ago

In 21650-try-catch.diff, looks like serialize() should be replaced with print_r() in line 645.

#10 @voldemortensen
10 years ago

Thanks SergeyBiryukov. updated.

#11 @SergeyBiryukov
10 years ago

One more thing, it should be print_r( $cache, true ) (to return the value instead of printing it directly).

#12 @voldemortensen
10 years ago

/sigh That should be it. My mind must be on the weekend or something.

#13 @SergeyBiryukov
10 years ago

14157-ms-php.fixed.diff includes some unrelated changes, but looks good otherwise.

#14 @voldemortensen
10 years ago

Last try before I quit for the weekend.

#15 @SergeyBiryukov
10 years ago

  • Keywords has-patch 4.1-early added; needs-patch removed

#16 @chriscct7
9 years ago

  • Keywords 4.1-early removed
  • Priority changed from low to normal
  • Severity changed from minor to normal

@desrosj
2 years ago

#17 @desrosj
2 years ago

  • Milestone set to 6.1

Came across this ticket going through tickets missing a milestone.

This doesn't seem to have come up again recently, but it probably wouldn't hurt to protect against custom cache groups where built-in PHP objects are potentially cached.

21650.diff is a refreshed patch for a sanity check.

#18 @jorbin
2 years ago

  • Keywords needs-unit-tests added

This seems like a good improvement, but it's one that I think requires some automated tests before it is ready for commit

#19 @audrasjb
22 months ago

  • Milestone changed from 6.1 to Future Release

With WP 6.1 RC 1 scheduled tomorrow (Oct 10, 2022), there is not much time left to address this ticket. Given it still needs unit tests, let's move this ticket to Future Release.

Ps: if you were about to send a patch and if you feel it is realistic to commit it in the next few hours, please feel free to move this ticket back to milestone 6.1.

Note: See TracTickets for help on using tickets.