Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Investigate weird serialization logic #277

Open
wants to merge 5 commits into
base: develop
Choose a base branch
from
Open

Investigate weird serialization logic #277

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
99fa699
fix condition
tillkruss Sep 11, 2020
c7ff576
simplify `maybe_serialize()`
tillkruss Sep 28, 2020
f0d5147
tweaks
tillkruss Sep 28, 2020
7db6db3
don't unserialize twice
tillkruss Sep 29, 2020
a6513be
flush cache after upgrading
tillkruss Sep 29, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions includes/class-plugin.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,13 +80,26 @@ private function __construct() {
$this->screen = 'settings_page_redis-cache';
}

$this->run_migrations();
$this->add_actions_and_filters();

if ( is_admin() && ! wp_next_scheduled( 'rediscache_discard_metrics' ) ) {
wp_schedule_event( time(), 'hourly', 'rediscache_discard_metrics' );
}
}

/**
* Run migrations
*
* @return void
*/
public function run_migrations() {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed 👌🏻

if ( WP_REDIS_VERSION === '2.0.16' && ! get_option( 'roc_migrated_2_0_16' ) ) {
wp_flush_cache();
update_option( 'roc_migrated_2_0_16', true, true );
}
}

/**
* Adds all necessary hooks
*
Expand Down
15 changes: 6 additions & 9 deletions includes/object-cache.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1976,7 +1976,7 @@ protected function maybe_unserialize( $original ) {
return igbinary_unserialize( $original );
}

// Don't attempt to unserialize data that wasn't serialized going in.
// Unserialize data that was serialized
if ( $this->is_serialized( $original ) ) {
// phpcs:ignore WordPress.PHP.NoSilencedErrors.Discouraged, WordPress.PHP.DiscouragedPHPFunctions.serialize_unserialize
$value = @unserialize( $original );
Expand Down Expand Up @@ -2006,17 +2006,13 @@ protected function maybe_serialize( $data ) {
return igbinary_serialize( $data );
}

if ( is_array( $data ) || is_object( $data ) ) {
// phpcs:ignore WordPress.PHP.DiscouragedPHPFunctions.serialize_serialize
return serialize( $data );
}

// Don't serialize data that's already serialized
if ( $this->is_serialized( $data, false ) ) {
// phpcs:ignore WordPress.PHP.DiscouragedPHPFunctions.serialize_serialize
return serialize( $data );
return $data;
}

return $data;
// phpcs:ignore WordPress.PHP.DiscouragedPHPFunctions.serialize_serialize
return serialize( $data );
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

<?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' ) );
        }
} );

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

@naxvog naxvog Sep 28, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

@naxvog naxvog Sep 28, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;
}

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

/**
Expand Down Expand Up @@ -2073,6 +2069,7 @@ protected function is_serialized( $data, $strict = true ) {
return false;
}
}

$token = $data[0];

switch ( $token ) {
Expand Down