Investigate weird serialization logic

[tillkruss]

My best guess is that this is never called. Thoughts @naxvog?

In 3.0 we should re-think the serialization.

[naxvog]

Just to summarize our slack conversation:

• The original change was made years ago
• The last condition of maybe_serialize serializes an already serialized string (nonsense)
• All non strings should be serialized

We agreed on taking no further action at the moment in 2.x but will refactor the method in a future 3.0 release in order to maintain compatibility.

[tillkruss]

Always serialize, except if it already was serialized? @naxvog thoughts?

[tillkruss]

Does Redis currently contain double serialized data? If so, what were the use cases?

[tillkruss]

<?php
class Naxvog_Test {
        private $creation_time;
        private $serialization_time;
        public $some_prop;
        private $some_secret_prop;
        public function __construct() {
                $this->creation_time = time();
                $this->some_secret_prop = 'baz';
                $this->some_prop = 'bar';
        }
        public function __serialize() {
                return [
                        'serialization_time' => time(),
                        'creation_time' => $this->creation_time,
                ];
        }
        public function __unserialize( $data ) {
                $this->serialization_time = $data['serialization_time'];
                $this->creation_time = $data['creation_time'];
        }
}
add_action( 'init', function() {
        $a = new Naxvog_Test();
        $s = serialize( $a );
        #var_dump( $a, $s );
        #var_dump( unserialize( $s ) );
        if ( isset( $_GET['debug-set'] ) ) {
                wp_cache_set( 'naxvog_test_obj1', $a );
                wp_cache_set( 'naxvog_test_obj2', $s );
        }
        if ( isset( $_GET['debug-get'] ) ) {
                var_dump( wp_cache_get( 'naxvog_test_obj1' ) );
                var_dump( wp_cache_get( 'naxvog_test_obj2' ) );
        }
} );

[tillkruss]

Should we check is_serialized() twice in maybe_unserialize()?

[naxvog]

I would rather prefer a conversion script running on plugin update resolving such issues but this might be a non trivial approach, would most likely require a LUA script and will take some time (best suited for action scheduler).

[naxvog]

I can't think of any valid way to serialize twice. Sure there might be serialized data within a serialized object but this is expected behaviour.

Have to look again but I'm fairly sure that I have not found any double serialized key our docker dev environment. Will have a look on my production redis instance. Should be fairly easy to find.

[naxvog]

OK found some instances (ran vim searching for [[:cntrl:]]s:[[:digit:]]\+:"O on a copy of the rdb database):

• Plugin Update Checker (option keys puc_external_updates-[slug], puc_external_updates_theme-[slug])
• WPML (option keys: wpml_config_index + others)
• Easy Social Share Buttons (seems to be using Plugin Update Checker as the option key external_updates-easy-social-share-buttons suggests)
• Google Sitemap Generator (option key sm_status)

[tillkruss]

Can we simply solve this by running this twice?

redis-cache/includes/object-cache.php

Lines 1979 to 1985 in fbfc86f

	// Don't attempt to unserialize data that wasn't serialized going in.
	if ( $this->is_serialized( $original ) ) {
	// phpcs:ignore WordPress.PHP.NoSilencedErrors.Discouraged, WordPress.PHP.DiscouragedPHPFunctions.serialize_unserialize
	$value = @unserialize( $original );
	return is_object( $value ) ? clone $value : $value;
	}

[naxvog]

Have to look in the code of those plugins to confirm that this is not intended. If not we should find the cause for this double serialization instead.

[tillkruss]

Running unserialize() twice seems risky, in case plugins do their own serialization.

[tillkruss]

@naxvog Thoughts on the latest push?

[naxvog]

Why are we cloning the unserialized object right away?

[naxvog]

is_string test is the first test in the is_serialized method - we can drop the first condition.

[naxvog]

For reference the WordPress maybe_serialize function is as follows:

function maybe_serialize( $data ) {
    if ( is_array( $data ) || is_object( $data ) ) {
        return serialize( $data );
    }
 
    /*
     * Double serialization is required for backward compatibility.
     * See https://core.trac.wordpress.org/ticket/12930
     * Also the world will end. See WP 3.6.1.
     */
    if ( is_serialized( $data, false ) ) {
        return serialize( $data );
    }
 
    return $data;
}

Don't think we should follow the backward compatibility discussed in https://core.trac.wordpress.org/ticket/12930

[tillkruss]

Alright:

• We're now flush the cache right after upgrading to 2.0.16, to flush weird double serialized data
• All data going into the object cache is serialized (strings, booleans, etc.)
• All data going out of the cache is unserialized once

@naxvog: I haven't tested any of this, but I'm happy with the overall approch

[naxvog]

To simplify things we could just flush the cache every time the plugin is updated.

Alternatively we should avoid polluting the options table with version specific entries - a general roc_version storing the last known version would be better. Migrations would only run if the current plugin version is newer.

[tillkruss]

Agreed 👌🏻