WP should catch serialization errors in options and meta fields

[Denis-de-Bernardy]

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.

[markjaquith]

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

[hakre]

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.

[Denis-de-Bernardy]

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

[Denis-de-Bernardy]

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

• ​http://bugs.php.net/bug.php?id=27428

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

[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?

[Denis-de-Bernardy]

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.

[hakre]

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.

[hakre]

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.

[Denis-de-Bernardy]

Reclosing this. See also #6784.

[Denis-de-Bernardy]

See also #12302