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.

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

TagAwareAdapter over non-binary memcached connections corrupts memcache #27416

Merged
merged 1 commit into from Jun 8, 2018

Conversation

Projects
None yet
4 participants
@palex-fpt
Copy link

palex-fpt commented May 30, 2018

Q A
Branch? 3.4
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? no
Fixed tickets #27405
License MIT
Doc PR

TagAwareAdapter uses non-ascii symbols in key names. It breaks memcached connections in non-binary mode.

@nicolas-grekas

This comment has been minimized.

Copy link
Member

nicolas-grekas commented May 30, 2018

This approach doesn't work, eg. a simple space is also forbidden in keys when using the non binary protocol.

Instead of changing the prefix for all backends (which has the downside of invalidating all existing pools btw), we should consider encoding keys, only in the memcached adapter.

@palex-fpt palex-fpt force-pushed the palex-fpt:27405 branch from 33880e2 to 7e41ff6 May 30, 2018

*
* @param $key
*/
private static function encodeKey($key)

This comment has been minimized.

@nicolas-grekas

nicolas-grekas May 30, 2018

Member

I would suggest to inline the function and remove the method.

@@ -49,6 +50,7 @@ private function init(\Memcached $client, $namespace, $defaultLifetime)
}
$this->maxIdLength -= strlen($client->getOption(\Memcached::OPT_PREFIX_KEY));
$this->client = $client;
$this->isTextProtocol = !$client->getOption(\Memcached::OPT_BINARY_PROTOCOL);

This comment has been minimized.

@nicolas-grekas

nicolas-grekas May 30, 2018

Member

What do you think of always encoding the key, binary or not? That'd make the stored data independent from the actual protocol.

This comment has been minimized.

@palex-fpt

palex-fpt May 30, 2018

Author

It adds some overhead. But it should not be high. There're two possible problems:

  1. client loses control over cache keys (mcrouter uses key names for app-level routing)
  2. non-symfony and/or non-php clients would have difficulties to share cache.

I don't share memcache with non-symfony apps, so I`m ok with it.

This comment has been minimized.

@nicolas-grekas

nicolas-grekas May 30, 2018

Member

I think that's ok: the overhead should be negligible vs the network round-trips, and url encoding preserves most of the chars in the keys, making them still handleable.
Note that keys should be decoded on retrieval.

@@ -198,6 +200,12 @@ protected function doSave(array $values, $lifetime)
$lifetime += time();
}
if ($this->isTextProtocol) {
$encoded_values = array();
array_walk($values, function ($value, $key) use (&$encoded_values) { $encoded_values[rawurlencode($key)] = $value; });

This comment has been minimized.

@nicolas-grekas

nicolas-grekas May 30, 2018

Member

I generally don't like array_walk(), too much references there. Should be done with a foreach instead.
The temp var should use camel case coding name also.

@nicolas-grekas

This comment has been minimized.

Copy link
Member

nicolas-grekas commented May 30, 2018

All methods should encode/decode on read/write. Some are missing for now.
LGTM otherwise thanks.

@nicolas-grekas nicolas-grekas added this to the 3.4 milestone May 30, 2018

@palex-fpt palex-fpt force-pushed the palex-fpt:27405 branch from 380ee83 to 7261d74 May 30, 2018

@nicolas-grekas

This comment has been minimized.

Copy link
Member

nicolas-grekas commented May 30, 2018

(but tests fail)

@@ -191,4 +192,29 @@ public function provideDsnWithOptions()
array(\Memcached::OPT_SOCKET_RECV_SIZE => 1, \Memcached::OPT_SOCKET_SEND_SIZE => 2, \Memcached::OPT_RETRY_TIMEOUT => 8),
);
}
public function testNonBinaryMemcachedMode()

This comment has been minimized.

@stof

stof May 30, 2018

Member

Instead of adding a few new tests for that, covering only a small part of the API, I think we should instead have a separate test case overriding createCachePool, to run the full adapter tests in non-binary mode

This comment has been minimized.

@palex-fpt

palex-fpt May 30, 2018

Author

MemcachedAdapterTest tests MemcachedAdapter::createConnection only. Should I add separate test classes to test MemcachedAdapter get, set, etc.. methods in text and binary modes?

This comment has been minimized.

@palex-fpt

palex-fpt May 31, 2018

Author

It looks like I should add key containing \0 and spaces into \Cache\IntegrationTests\SimpleCacheTest::validKeys

This way it would be tested against all cache implementations.

This comment has been minimized.

@nicolas-grekas

nicolas-grekas May 31, 2018

Member

Or rely on the namespace to do so?

This comment has been minimized.

@palex-fpt

palex-fpt May 31, 2018

Author

I added 'si\0mp le' key to \Cache\IntegrationTests\SimpleCacheTest::validKeys and ran tests local. All cache tests succeeded.

$encodedValues[rawurlencode($key)] = $value;
}
return $this->checkResultCode($this->getClient()->setMulti($encodedValues, $lifetime));

This comment has been minimized.

@stof

stof May 30, 2018

Member

this is not decoding keys in the returned value. Can it have keys in it ?

This comment has been minimized.

@palex-fpt

palex-fpt May 30, 2018

Author

\Memcached::setMulti returns bool. Did I missed something else?

@palex-fpt palex-fpt force-pushed the palex-fpt:27405 branch 2 times, most recently from 6ce07dd to 96c6cf9 May 30, 2018

{
$client = $defaultLifetime ? AbstractAdapter::createConnection('memcached://'.getenv('MEMCACHED_HOST'), array('binary_protocol' => false)) : self::$client;
return new MemcachedCache($client, str_replace('\\', '.', __CLASS__), $defaultLifetime);

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Jun 4, 2018

Member

Since the 2nd arg here is the namespace and is part of the key passed to memcached, we could add \0 and spaces to test the encoding?

This comment has been minimized.

@palex-fpt

palex-fpt Jun 5, 2018

Author

base test \Symfony\Component\Cache\Tests\Simple\CacheTestCase::validKeys adds \0 to tested keys

Aleksey Prilipko

@palex-fpt palex-fpt force-pushed the palex-fpt:27405 branch from 96c6cf9 to 67d4e6d Jun 5, 2018

@nicolas-grekas

This comment has been minimized.

Copy link
Member

nicolas-grekas commented Jun 8, 2018

Thank you @palex-fpt.

@nicolas-grekas nicolas-grekas merged commit 67d4e6d into symfony:3.4 Jun 8, 2018

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
fabbot.io Your code looks good.
Details

nicolas-grekas added a commit that referenced this pull request Jun 8, 2018

bug #27416 TagAwareAdapter over non-binary memcached connections corr…
…upts memcache (Aleksey Prilipko)

This PR was merged into the 3.4 branch.

Discussion
----------

TagAwareAdapter over non-binary memcached connections corrupts memcache

| Q             | A
| ------------- | ---
| Branch?       | 3.4
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | no
| Fixed tickets | #27405
| License       | MIT
| Doc PR        |

TagAwareAdapter uses non-ascii symbols in key names. It breaks memcached connections in non-binary mode.

Commits
-------

67d4e6d bug #27405 [Cache] TagAwareAdapter should not corrupt memcached connection in ascii mode

@palex-fpt palex-fpt deleted the palex-fpt:27405 branch Jun 9, 2018

This was referenced Jun 25, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.