Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

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

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

Merged
nicolas-grekas merged 1 commit intosymfony:4.4fromlstrojny:memcached-adapter-double-encoding
Sep 9, 2020
Merged

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

nicolas-grekas merged 1 commit intosymfony:4.4fromlstrojny:memcached-adapter-double-encoding
Sep 9, 2020

Conversation

@lstrojny
Copy link
Contributor

QA
Branch?4.4
Bug fix?yes
New feature?no
Deprecations?no
Ticketsn.A.
LicenseMIT
Doc PRFix double encoding in memcached which lead to overlong keys being generated

Because the memcached adapter usesrawurlencode() 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 toAbstractAdapterTrait::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).

Copy link
Member

@nicolas-grekasnicolas-grekas left a comment

Choose a reason for hiding this comment

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

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?

@nicolas-grekasnicolas-grekas changed the titleFix double encoding issue in Memcached adapter[Cache] Fix double encoding issue in Memcached adapterSep 8, 2020
@lstrojny
Copy link
ContributorAuthor

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.

@nicolas-grekasnicolas-grekas changed the title[Cache] Fix double encoding issue in Memcached adapter[Cache] Fix key encoding issue in Memcached adapterSep 8, 2020
@nicolas-grekas
Copy link
Member

nicolas-grekas commentedSep 8, 2020•
edited
Loading

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

@lstrojny
Copy link
ContributorAuthor

lstrojny commentedSep 9, 2020•
edited
Loading

@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?

@nicolas-grekas
Copy link
Member

Thank you@lstrojny.

@nicolas-grekasnicolas-grekas merged commitafd4027 intosymfony:4.4Sep 9, 2020
@lstrojnylstrojny deleted the memcached-adapter-double-encoding branchSeptember 9, 2020 09:27
@nicolas-grekas
Copy link
Member

Arg, github actionjust failed with

1) Symfony\Component\Cache\Tests\Simple\MemcachedCacheTextModeTest::testGetMultipleFailed asserting that two strings are equal.--- Expected+++ Actual@@ @@-'foo'+'value'/home/runner/work/symfony/symfony/vendor/cache/integration-tests/src/SimpleCacheTest.php:331

@lstrojny
Copy link
ContributorAuthor

@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 likegetMulti() does not always return in order

@nicolas-grekas
Copy link
Member

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
Copy link
ContributorAuthor

@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
Copy link
Member

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
Copy link
ContributorAuthor

lstrojny commentedSep 9, 2020•
edited
Loading

This would fix it:

diff --git a/src/Symfony/Component/Cache/Traits/AbstractTrait.php b/src/Symfony/Component/Cache/Traits/AbstractTrait.phpindex 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
Copy link
ContributorAuthor

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

@nicolas-grekas
Copy link
Member

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

@lstrojny
Copy link
ContributorAuthor

Done, see#38126

nicolas-grekas added a commit that referenced this pull requestSep 10, 2020
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#38108With 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-grekasCommits-------15c21db [Cache] Limit cache version character range
This was referencedSep 27, 2020
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@nicolas-grekasnicolas-grekasnicolas-grekas approved these changes

Assignees

No one assigned

Projects

None yet

Milestone

4.4

Development

Successfully merging this pull request may close these issues.

4 participants

@lstrojny@nicolas-grekas@javiereguiluz@carsonbot
[8]ページ先頭
©2009-2025 Movatter.jp