-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[Cache] Limit cache version character range #38126
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
Conversation
$id = key($keys); | ||
throw new InvalidArgumentException(sprintf('Could not match value id "%s" to keys "%s".', $id, implode('", "', $keys))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is what lead to the mix up of values. The assumption here is: if the id of the value cannot be found in the original keys, let’s just assume it to be the first one. Instead of returning a wrong value, we should create a miss here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I realize I wasn’t precise in my language: this is not the root cause of the issue but the subsequent error that lead to values being mixed up. The root cause was that they version ID contained a slash, that slash was replaced with a null byte in decodeKey
and therefore the keys no longer matched and the piece of code above was triggered.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI, we missed patching this other file, now fixed:
25941ff
$id = key($keys); | ||
throw new InvalidArgumentException(sprintf('Could not match value id "%s" to keys "%s".', $id, implode('", "', $keys))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
@nicolas-grekas the original randomness is restored, the patch should be good now |
Thank you @lstrojny |
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