Make WordPress Core

Opened 13 years ago

Closed 12 years ago

#16597 closed defect (bug) (fixed)

Bug in the maybe_serialize() function - it serializing also serailize objects.

Reported by: maorb's profile maorb Owned by: nacin's profile nacin
Milestone: 3.3 Priority: normal
Severity: major Version: 3.0.5
Component: Formatting Keywords: has-patch
Focuses: Cc:



I have discovered this bug by accident, when trying to import posts using the csv importer plugin.
One of my lines contained a serialized array to be saved.
The serialized data was ok, but saved to the db with a prefix of s:196:" and suffix of ";.
The reason is that the add_post_meta() function uses the add_metadata() , which uses the maybe_serialize() function to determine if need to serialize or not.
The thing is that it is serializing anyway!

Look at wp-includes/functions.php, lines 1005-1013 -

function maybe_serialize( $data ) {
	if ( is_array( $data ) || is_object( $data ) )
		return serialize( $data );

	if ( is_serialized( $data ) )
		return serialize( $data );

	return $data;

It says to serialize also if the data is_serialized....

if ( is_serialized( $data ) )
		return serialize( $data );

The correct should be -

function maybe_serialize( $data ) {
	if ( is_array( $data ) || is_object( $data ) )
		return serialize( $data );

	if ( is_serialized( $data ) )
		return $data ;

	return $data;

Otherwise, the data is being serialized twice and then WP can't show the real unserialized data ..

After I changed it and rechecked passing serialized data to the add_meta_data() function - it all went ok.

This should be fixed in core.

Here is a reference also in the forums


Attachments (1)

16597.patch (484 bytes) - added by SergeyBiryukov 12 years ago.

Download all attachments as: .zip

Change History (11)

#2 @nacin
13 years ago

  • Owner set to nacin
  • Status changed from new to accepted

Double-serialize is intentional.

In 3.2 I will add a quick comment there letting people know it is intentional and I'll cross-reference r14074. Let's just leave this open for a few days before I forget.

#3 @maorb
13 years ago

Why it should be double serialized?
This means that if one will pass serialized data to the add_metadata() function, it will never be parsed ok, since it will add the second serialization.
This causes breaking of the data when pulling out from the db..
If double serialize is a must, maybe should have addition to the get_metadata() to handle it better?


#4 @nacin
13 years ago

If you serialize something then pass it to add_metadata(), then you should be expected to unserialize it when you fetch it from get_metadata(). Whatever goes in, comes out.

In turn, the functions already serialize -- that's the point of maybe_serialize(), it gets used in the transient, option, site option, and metadata APIs. Just pass an array or object and it'll take care of it. In a nutshell, you're thinking too hard. :-)

When this last came up, I wrote a blog post on it:

#5 @maorb
13 years ago

I agree with you that what goes in should come out.
Anyway, there are situations of different methods to insert data and thus it sometimes will go as an array and sometimes as a serialized data.
For this example I encountered - there are some meta keys that are indeed being passed as an array, where the end user just saves data from the admin panel.
But, in this case for the same site, there is a way of importing csv file to the database.
All these custom fields that are being saved into one meta_key and the meta_value is the serialized array, cannot be passed as an object when it comes to a csv file, since you cannot pass an array in a csv, thus when data comes from csv it must be serialized first.

I thought that the maybe_serialize() should check if the data passes it alreay serialized and not to serialize it again..

#6 @hakre
12 years ago

If you import a CSV, the import script should make use of the is_serailized() function on each field. In case this function returns true, unserialize it. Then give only unserialized values to the add_post_meta()/add_metadata() functions and you should be fine.

Edit: Or use maybe_unserialize() on CSV data fields which might be more straight forward (and does what I wrote).

Last edited 12 years ago by hakre (previous) (diff)

#7 @SergeyBiryukov
12 years ago

  • Milestone Awaiting Review deleted
  • Resolution set to duplicate
  • Status changed from accepted to closed

#8 @SergeyBiryukov
12 years ago

  • Keywords has-patch added

Added patch with a comment, as per nacin's suggestion (just in case).

#9 @nacin
12 years ago

  • Milestone set to 3.3
  • Resolution duplicate deleted
  • Status changed from closed to reopened

#10 @markjaquith
12 years ago

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

In [18807]:

Add note about why double-serialization is done. props SergeyBiryukov. fixes #16597

Note: See TracTickets for help on using tickets.