WordPress.org

Make WordPress Core

Opened 6 years ago

Closed 6 years ago

Last modified 5 years ago

#9549 closed defect (bug) (fixed)

WP should catch serialization errors in options and meta fields

Reported by: Denis-de-Bernardy Owned by:
Milestone: 2.8 Priority: normal
Severity: critical Version: 2.8
Component: General Keywords: has-patch dev-feedback
Focuses: Cc:

Description

Related to: #8804 and #6532.

One some servers, with some configs, you occasionally get serialized data with an erroneous strlen(). It's clearly php related, but it does break sites.

The bug has to do with the fact that the string length function that is internally used by serialize() doesn't like utf8 much. You get erroneous string length values -- even when strlen() returns the correct value, and even when you overload the strlen() function. This makes it borderline impossible to reproduce on an english site, but it definitely occurs out in the wild.

Anyway, the end result is a corrupt array that is then passed into WP as a string. On occasion, this leads to fatal errors. (see the above two bugs.)

I've mostly seen this happen with text widgets or equivalent; more rarely with post meta fields and the like. It seems to me that this could be corrected by returning a straight false when unserialization fails, as is done in the attached patch.

Attachments (1)

9549.diff (576 bytes) - added by Denis-de-Bernardy 6 years ago.

Download all attachments as: .zip

Change History (12)

@Denis-de-Bernardy6 years ago

comment:1 @Denis-de-Bernardy6 years ago

  • Keywords has-patch dev-feedback added

comment:2 @markjaquith6 years ago

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

(In [10955]) Return false for corrupted serialized strings. props Denis-de-Bernardy. fixes #9549

comment:3 @hakre6 years ago

  • Resolution fixed deleted
  • Severity changed from normal to critical
  • Status changed from closed to reopened

Let's assume that is_serialized() does a propper job, then I can live with that patch. But it does not #9663. Additionally that wild-guessing about MBCS and strlen() vs. unserialize really gets on my nerves. Please link hard facts on having a bug there in PHP core or leave it. but guessing around does not help to get these really hard bugs out of the core.

Having users overload functions while not knowing what they do is their fault. Developers must at least stick with the default implementation, you can't take responsibility for environments where stuff is massivly overloaded and therefore re-defined.

So I must say (and I did that in the past) maybe_serialize() and maybe_unserialize() should be a pair. The last changeset did break that pattern. This might lead to even more problems, therefore I reopen the ticket and suggest to revert the last change.

comment:4 @Denis-de-Bernardy6 years ago

Oh, it's not wild guessing. I've seen this occur so many times it's beyond imaginable. For hard bug references, see:

http://www.google.com/search?hl=en&num=100&q=site%3Abugs.php.net+unserialize+offset

comment:5 @Denis-de-Bernardy6 years ago

Trouble is, almost all of the reports for the same bug are along the lines of this one:

Error gets reported, and gets ignored or filed as bogus from lack of a working, "here's how to systematically reproduce" example that the php devs can use. It more or less randomly occurs from a platform to the next.

But errors definitely are around:

http://www.google.com/search?hl=en&num=100&q=unserialize+error+offset

comment:6 follow-up: @hakre6 years ago

Has anybody checked that option values are stored binary-safe in the database? If not, we do not need to wonder if unserizalizing errors occur here and there. Maybe making the store safe first?

comment:7 in reply to: ↑ 6 @Denis-de-Bernardy6 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

Replying to hakre:

Has anybody checked that option values are stored binary-safe in the database? If not, we do not need to wonder if unserizalizing errors occur here and there. Maybe making the store safe first?

Well, they definitely aren't stored in a binary safe way. *Maybe* that would fix things indeed. But then we'd potentially have multitudes of other potential issues, along the lines of no longer being able to manipulate meta_key in queries with SQL functions without casting it first.

My personal take on this is that it's a horrible idea to use non-native columns to store all of this meta information and that the posts table should get additional fields as needed. But then, what do I know. ;-)

Anyway, re-closing this as fixed, since your other ticket raises the is_serialized issue. The fix that was applied in this ticket prevents sites from getting fatal errors when an array stored in options or meta tables fails to get unserialized. That is a huge improvement in itself.

comment:8 @hakre6 years ago

But now leaves open to the fact that the maybe_(un)serialize() functions do not work in pair with each other. there was already done a regression check on a similiar issue and that time it was clearly argumented to not break that: #7133

it should be no problem to imagine the testcase that will be breaking with this changeset here.

comment:9 @hakre6 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

For being safe: Option values can be stored with a checksum into database sotaht they still can be manipulated because they exist as strings but it is also possible to check wether their integrity is broken.

comment:10 @Denis-de-Bernardy6 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

Reclosing this. See also #6784.

Note: See TracTickets for help on using tickets.