WordPress.org

Make WordPress Core

Opened 7 years ago

Closed 7 years ago

Last modified 6 years ago

#4029 closed defect (bug) (invalid)

maybe_serialize() can do double-serialize

Reported by: takayukister Owned by:
Milestone: Priority: normal
Severity: major Version: 2.2
Component: General Keywords: has-patch dev-feedback
Focuses: Cc:

Description

I'm not sure if this is a problem, but I'm little confused. When a string argument is passed to maybe_serialize(), it serialize the string even if the string is an object which is already serialized. Is it necessary??

Attachments (2)

maybe_serialize.diff (426 bytes) - added by takayukister 7 years ago.
Serialize if target is *not* serialized.
4029.testcases.diff (2.7 KB) - added by jacobsantos 6 years ago.
Test cases for maybe_serialize()

Download all attachments as: .zip

Change History (13)

takayukister7 years ago

Serialize if target is *not* serialized.

comment:1 rob1n7 years ago

  • Keywords dev-feedback added; serialize removed
  • Milestone changed from 2.3 to 2.2
  • Priority changed from low to normal
  • Severity changed from normal to major

Haha, I'm guessing we don't use maybe_serialize at all in the code.

But yes, I'm puzzled too and your patch makes sense.

comment:2 ryan7 years ago

Looks like it's been broken since it was introduce in [4382]. Fix looks good to me.

comment:3 ryan7 years ago

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

(In [5102]) Correct reversed logic in maybe_serialize. Props takayukister. fixes #4029

comment:4 mdawaffe7 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

I think the original logic was intentional.

If you want to store a serialized string, you store it as twice serialized so that when you retrieve it, it comes back once serialized.

MarkJaquith?

comment:5 masquerade7 years ago

I believe its intentional also. This way, nobody can fake a serialized string that is an array with a bizzare amount of elements for the sole purpose of crashing PHP and the webserver. I remember this being a bug in the past because you could put a serialized string in some of the options fields and it would be deserialized, meaning you could inject objects/large arrays/etc.

comment:6 ryan7 years ago

But using maybe_serialize() outside the context of options.php could be confusing. You could triple serialize if not careful, yes?

comment:7 takayukister7 years ago

I am sorry but I found that my first patch may cause another problem.

Original maybe_serialize() does not apply serialization to plain (not serialized) string, so there are many plain string values existing in options, postmeta and usermeta tables. My patched maybe_serialize() reverse the behavior and serialize all the plain strings.

If there are codes or plugins that access directly to that tables and expect to get plain string value, they may fail.

I'm sorry for my carelessness. I think it would be good to make another patches if necessary.

comment:8 ryan7 years ago

[5109] reverts.

comment:9 markjaquith7 years ago

  • Milestone 2.2 deleted
  • Resolution set to invalid
  • Status changed from reopened to closed

Intentional. Ensures that input == output (using maybe_unserialize()). Otherwise, as masquerade pointed out, you can enter a serialized array into a string input and get an array as the output. There were documented server-crash scenarios from this.

The other solution was serializing everything, but this makes the options and meta tables really unfriendly.

comment:10 hakre6 years ago

Some questions:

  • who did invent that function maybe_serialize()?
  • where is documented when this function should be used?
  • where is documented why this function has been implemented?

comment:11 DD326 years ago

where is documented when this function should be used?
where is documented why this function has been implemented?

I'm not sure on either account, However, The use of maybe_serialize()/ maybe_unserialize() functions are used when storing metadata(post, taxonomy, options), It means that a string can be saved as a pure string(unserialised) - which reduces on operations needed, It means that a integer gets straight into the db, it means that an array gets serialized before, etc. maybe_unserialize() does the same, if the input is serialised, it spits out the unserialised version, else lets it pass straight theough

jacobsantos6 years ago

Test cases for maybe_serialize()

Note: See TracTickets for help on using tickets.