NotificationsYou must be signed in to change notification settings
Fork9.7k
Star30.8k

TagAwareAdapter over non-binary memcached connections corrupts memcache#27416

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

nicolas-grekas merged 1 commit intosymfony:3.4frompalex-fpt:27405

Jun 8, 2018

Merged

TagAwareAdapter over non-binary memcached connections corrupts memcache#27416

nicolas-grekas merged 1 commit intosymfony:3.4frompalex-fpt:27405

Jun 8, 2018

Conversation

Copy link

palex-fpt commentedMay 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.

carsonbot added Status: Needs Review Bug labels

May 30, 2018

Copy link

Member

nicolas-grekas commentedMay 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 force-pushed the27405 branch from33880e2 to7e41ff6Compare

May 30, 2018 06:17

nicolas-grekas reviewed

May 30, 2018

View reviewed changes

src/Symfony/Component/Cache/Traits/MemcachedTrait.php Outdated

		*
		* @param $key
		*/
		privatestaticfunctionencodeKey($key)

Copy link

Member

Choose a reason for hiding this comment

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

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

src/Symfony/Component/Cache/Traits/MemcachedTrait.php Outdated

		}
		$this->maxIdLength -=strlen($client->getOption(\Memcached::OPT_PREFIX_KEY));
		$this->client =$client;
		$this->isTextProtocol = !$client->getOption(\Memcached::OPT_BINARY_PROTOCOL);

Copy link

Member

Choose a reason for hiding this comment

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

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

Copy link

Author

palex-fptMay 30, 2018•
edited
Loading

Choose a reason for hiding this comment

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

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

client loses control over cache keys (mcrouter uses key names for app-level routing)
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.

Copy link

Member

Choose a reason for hiding this comment

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

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.

nicolas-grekas reviewed

May 30, 2018

View reviewed changes

src/Symfony/Component/Cache/Traits/MemcachedTrait.php Outdated


		if ($this->isTextProtocol) {
		$encoded_values =array();
		array_walk($values,function ($value,$key)use (&$encoded_values) {$encoded_values[rawurlencode($key)] =$value; });

Copy link

Member

nicolas-grekasMay 30, 2018•
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link

Member

nicolas-grekas commentedMay 30, 2018

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

nicolas-grekas added this to the3.4 milestone

May 30, 2018

palex-fpt force-pushed the27405 branch from380ee83 to7261d74Compare

May 30, 2018 07:52

carsonbot added Status: Reviewed and removed Status: Needs Review labels

May 30, 2018

nicolas-grekas approved these changes

May 30, 2018

View reviewed changes

Copy link

Member

nicolas-grekas commentedMay 30, 2018

(but tests fail)

stof reviewed

May 30, 2018

View reviewed changes

src/Symfony/Component/Cache/Tests/Adapter/MemcachedAdapterTest.php Outdated

		);
		}

		publicfunctiontestNonBinaryMemcachedMode()

Copy link

Member

stofMay 30, 2018

Choose a reason for hiding this comment

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

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 overridingcreateCachePool, to run the full adapter tests in non-binary mode

Copy link

Author

palex-fptMay 30, 2018

Choose a reason for hiding this comment

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

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

Copy link

Author

palex-fptMay 31, 2018

Choose a reason for hiding this comment

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

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.

Copy link

Member

nicolas-grekasMay 31, 2018

Choose a reason for hiding this comment

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

Or rely on the namespace to do so?

Copy link

Author

palex-fptMay 31, 2018

Choose a reason for hiding this comment

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

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

src/Symfony/Component/Cache/Traits/MemcachedTrait.php

		$encodedValues[rawurlencode($key)] =$value;
		}

		return$this->checkResultCode($this->getClient()->setMulti($encodedValues,$lifetime));

Copy link

Member

stofMay 30, 2018

Choose a reason for hiding this comment

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

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

Copy link

Author

palex-fptMay 30, 2018

Choose a reason for hiding this comment

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

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

palex-fpt force-pushed the27405 branch 2 times, most recently from6ce07dd to96c6cf9Compare

May 31, 2018 06:06

nicolas-grekas reviewed

Jun 4, 2018

View reviewed changes

src/Symfony/Component/Cache/Tests/Simple/MemcachedCacheTextModeTest.php

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

		returnnewMemcachedCache($client,str_replace('\\','.',__CLASS__),$defaultLifetime);

Copy link

Member

nicolas-grekasJun 4, 2018

Choose a reason for hiding this comment

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

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?

Copy link

Author

palex-fptJun 5, 2018•
edited
Loading

Choose a reason for hiding this comment

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

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

bugsymfony#27405[Cache] TagAwareAdapter should not corrupt memcache…

67d4e6d

…d connection in ascii mode

palex-fpt force-pushed the27405 branch from96c6cf9 to67d4e6dCompare

June 5, 2018 07:08

Copy link

Member

nicolas-grekas commentedJun 8, 2018

Thank you@palex-fpt.

nicolas-grekas merged commit67d4e6d intosymfony:3.4

Jun 8, 2018

nicolas-grekas added a commit that referenced this pull request

Jun 8, 2018

bug#27416TagAwareAdapter over non-binary memcached connections corr…

88098f3

…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 deleted the 27405 branch

June 9, 2018 00:03

This was referencedJun 25, 2018

Release v3.4.12#27704

Merged

Release v4.0.12#27705

Merged

Release v4.1.1#27706

Merged

Labels

Bug Status: Reviewed

Movatterモバイル変換

Uh oh!

TagAwareAdapter over non-binary memcached connections corrupts memcache#27416

TagAwareAdapter over non-binary memcached connections corrupts memcache#27416

Uh oh!

Conversation

palex-fpt commentedMay 30, 2018

Uh oh!

nicolas-grekas commentedMay 30, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

palex-fptMay 30, 2018• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nicolas-grekasMay 30, 2018• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nicolas-grekas commentedMay 30, 2018

Uh oh!

nicolas-grekas commentedMay 30, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

palex-fptJun 5, 2018• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nicolas-grekas commentedJun 8, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

palex-fptMay 30, 2018•
edited
Loading

nicolas-grekasMay 30, 2018•
edited
Loading

palex-fptJun 5, 2018•
edited
Loading