[Cache] Fix key encoding issue in Memcached adapter #38108

lstrojny · 2020-09-08T09:38:38Z

Q	A
Branch?	4.4
Bug fix?	yes
New feature?	no
Deprecations?	no
Tickets	n.A.
License	MIT
Doc PR	Fix double encoding in memcached which lead to overlong keys being generated

Because the memcached adapter uses rawurlencode() to encode each and every key, keys will sometimes be too long and therefore hit the memcached limit of 250 bytes. This happens when the key is small enough to be below 250 when passed to AbstractAdapterTrait::getId() and is then blown up over the 250 bytes limit in memcached adapter without validating the length again.

Looking through the code, it seems that the double encoding is wholly unnecessary assuming if one makes sure the namespace is properly encoded. This PR therefore removes the double encoding and instead uses rawurlencode on the namespace (which is in turn properly accounted for when calculating whether or not we are over the ID limit).

nicolas-grekas

Thanks for the PR.
I understand that rawurlencode increases the length of keys, but encoding them is nonetheless required since memcached doesn't allow spaces in them, while we do.
This breaks it currently, isn't it?

src/Symfony/Component/Cache/Tests/Adapter/AdapterTestCase.php

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

lstrojny · 2020-09-08T15:27:01Z

Thanks for the PR.
I understand that rawurlencode increases the length of keys, but encoding them is nonetheless required since memcached doesn't allow spaces in them, while we do.
This breaks it currently, isn't it?

Great point, indeed, spaces would now break. So what I did now is adding a custom encoding function that replaces memcached reserved characters with Symfony reserved characters but keep the length of the key.

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

nicolas-grekas · 2020-09-08T20:59:16Z

LGTM but there are some failures to investigate in Github Actions.

lstrojny · 2020-09-09T08:05:14Z

@nicolas-grekas tests pass now, fixed further encoding issue. I accidentally force-pushed over your changes but restored the variable rename. I've seen that you removed the rawurlencode() for the namespace and I expanded the testcase to demonstrate why I think it’s needed. Can you elaborate on why you think it’s not needed?

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

nicolas-grekas · 2020-09-09T09:23:08Z

Thank you @lstrojny.

nicolas-grekas · 2020-09-09T09:29:08Z

Arg, github action just failed with

1) Symfony\Component\Cache\Tests\Simple\MemcachedCacheTextModeTest::testGetMultiple
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'foo'
+'value'

/home/runner/work/symfony/symfony/vendor/cache/integration-tests/src/SimpleCacheTest.php:331

lstrojny · 2020-09-09T09:54:26Z

@nicolas-grekas it's a randomly failing test, try this:

for x in {0..1000} ; do echo $x ; simple-phpunit --filter "testGetMultiple$" src/Symfony/Component/Cache/Tests/Simple/MemcachedCacheTextModeTest.php || break ; done

From a first glimpse it looks like getMulti() does not always return in order

nicolas-grekas · 2020-09-09T09:57:39Z

From a first glimpse it looks like getMulti() does not always return in order

oh, then we might want to make tests resilient to this. If you want to give it a try, PR welcome :)

lstrojny · 2020-09-09T10:35:52Z

@nicolas-grekas correction, it is indeed the patch. It happens when the namespace version contains a slash. This seems to me to be a violation of "internal protocol" in the sense that an adapter should expect to not deal with keys containing PSR-6 reserved characters. WDYT?

nicolas-grekas · 2020-09-09T10:40:17Z

Adapters can deal with non-psr6 chars actually. We might have a conflict with versioning then. Would using : work instead of the slash?
Alternatively, we could make parsing for the version more robust, to prevent the conflict, if that makes sense (I don't remember and I didn't check :) )

lstrojny · 2020-09-09T10:41:36Z

This would fix it:

diff --git a/src/Symfony/Component/Cache/Traits/AbstractTrait.php b/src/Symfony/Component/Cache/Traits/AbstractTrait.php
index d4a16959a8..d865bd30cb 100644
--- a/src/Symfony/Component/Cache/Traits/AbstractTrait.php
+++ b/src/Symfony/Component/Cache/Traits/AbstractTrait.php
@@ -119,7 +119,7 @@ trait AbstractTrait
                 }
             }
             $namespaceToClear = $this->namespace.$namespaceVersionToClear;
-            $namespaceVersion = substr_replace(base64_encode(pack('V', mt_rand())), static::NS_SEPARATOR, 5);
+            $namespaceVersion = self::createNewNamespaceVersion();
             try {
                 $cleared = $this->doSave([static::NS_SEPARATOR.$this->namespace => $namespaceVersion], 0);
             } catch (\Exception $e) {
@@ -268,7 +268,7 @@ trait AbstractTrait
                     $this->namespaceVersion = $v;
                 }
                 if ('1'.static::NS_SEPARATOR === $this->namespaceVersion) {
-                    $this->namespaceVersion = substr_replace(base64_encode(pack('V', time())), static::NS_SEPARATOR, 5);
+                    $this->namespaceVersion = self::createNewNamespaceVersion();
                     $this->doSave([static::NS_SEPARATOR.$this->namespace => $this->namespaceVersion], 0);
                 }
             } catch (\Exception $e) {
@@ -300,4 +300,9 @@ trait AbstractTrait
     {
         throw new \DomainException('Class not found: '.$class);
     }
+
+    private static function createNewNamespaceVersion(): string
+    {
+        return substr_replace(bin2hex(pack('V', mt_rand())), static::NS_SEPARATOR, 7);
+    }
 }

Replacing base64 with hex makes sure that there will never be a slash

lstrojny · 2020-09-09T10:42:42Z

I am not sure why previously time() and mt_rand() was used in two different places.

nicolas-grekas · 2020-09-09T10:59:19Z

I won't be able to have a look today, but please submit a PR and I will tomorrow.

lstrojny · 2020-09-09T11:41:34Z

Done, see #38126

@nicolas-grekas

This PR was squashed before being merged into the 4.4 branch. Discussion ---------- [Cache] Limit cache version character range | Q | A | ------------- | --- | Branch? | 4.4 | Bug fix? | yes | New feature? | no | Deprecations? | no | Tickets | n.A. | License | MIT | Doc PR | Follow up for #38108 With current HEAD in 4.4, this will fail eventually: `simple-phpunit --repeat 1000 --stop-on-failure --filter "testGetMultiple$" src/Symfony/Component/Cache/Tests/Simple/MemcachedCacheTextModeTest.php` After this PR it will no longer fail @nicolas-grekas Commits ------- 15c21db [Cache] Limit cache version character range

carsonbot added Status: Needs Review Bug labels Sep 8, 2020

nicolas-grekas added this to the 4.4 milestone Sep 8, 2020

javiereguiluz added the Cache label Sep 8, 2020

nicolas-grekas reviewed Sep 8, 2020

View reviewed changes

nicolas-grekas changed the title ~~Fix double encoding issue in Memcached adapter~~ [Cache] Fix double encoding issue in Memcached adapter Sep 8, 2020

nicolas-grekas reviewed Sep 8, 2020

View reviewed changes

src/Symfony/Component/Cache/Traits/MemcachedTrait.php Outdated Show resolved Hide resolved

src/Symfony/Component/Cache/Traits/MemcachedTrait.php Show resolved Hide resolved

src/Symfony/Component/Cache/Traits/MemcachedTrait.php Outdated Show resolved Hide resolved

nicolas-grekas reviewed Sep 8, 2020

View reviewed changes

src/Symfony/Component/Cache/Traits/MemcachedTrait.php Outdated Show resolved Hide resolved

nicolas-grekas changed the title ~~[Cache] Fix double encoding issue in Memcached adapter~~ [Cache] Fix key encoding issue in Memcached adapter Sep 8, 2020

nicolas-grekas reviewed Sep 9, 2020

View reviewed changes

src/Symfony/Component/Cache/Tests/Adapter/MemcachedAdapterTest.php Outdated Show resolved Hide resolved

[Cache] Fix key encoding issue in Memcached adapter

23bf9be

nicolas-grekas approved these changes Sep 9, 2020

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Review labels Sep 9, 2020

nicolas-grekas merged commit afd4027 into symfony:4.4 Sep 9, 2020

lstrojny deleted the memcached-adapter-double-encoding branch September 9, 2020 09:27

lstrojny mentioned this pull request Sep 9, 2020

[Cache] Limit cache version character range #38126

Merged

This was referenced Sep 27, 2020

Release v4.4.14 #38312

Merged

Release v5.1.6 #38313

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Cache] Fix key encoding issue in Memcached adapter #38108

[Cache] Fix key encoding issue in Memcached adapter #38108

lstrojny commented Sep 8, 2020

nicolas-grekas left a comment

lstrojny commented Sep 8, 2020

nicolas-grekas commented Sep 8, 2020 •

edited

Loading

lstrojny commented Sep 9, 2020 •

edited

Loading

nicolas-grekas commented Sep 9, 2020

nicolas-grekas commented Sep 9, 2020

lstrojny commented Sep 9, 2020

nicolas-grekas commented Sep 9, 2020

lstrojny commented Sep 9, 2020

nicolas-grekas commented Sep 9, 2020

lstrojny commented Sep 9, 2020 •

edited

Loading

lstrojny commented Sep 9, 2020

nicolas-grekas commented Sep 9, 2020

lstrojny commented Sep 9, 2020

[Cache] Fix key encoding issue in Memcached adapter #38108

[Cache] Fix key encoding issue in Memcached adapter #38108

Conversation

lstrojny commented Sep 8, 2020

nicolas-grekas left a comment

Choose a reason for hiding this comment

lstrojny commented Sep 8, 2020

nicolas-grekas commented Sep 8, 2020 • edited Loading

lstrojny commented Sep 9, 2020 • edited Loading

nicolas-grekas commented Sep 9, 2020

nicolas-grekas commented Sep 9, 2020

lstrojny commented Sep 9, 2020

nicolas-grekas commented Sep 9, 2020

lstrojny commented Sep 9, 2020

nicolas-grekas commented Sep 9, 2020

lstrojny commented Sep 9, 2020 • edited Loading

lstrojny commented Sep 9, 2020

nicolas-grekas commented Sep 9, 2020

lstrojny commented Sep 9, 2020

nicolas-grekas commented Sep 8, 2020 •

edited

Loading

lstrojny commented Sep 9, 2020 •

edited

Loading

lstrojny commented Sep 9, 2020 •

edited

Loading