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

Fix possible memory corruption in FLUSHALL when a client watches more than one key #11854

Conversation

ranshid
Copy link
Collaborator

@ranshid ranshid commented Feb 28, 2023

Avoid calling unwatchAllKeys when running touchAllWatchedKeysInDb (which was unnecessary)
This can potentially lead to use-after-free and memory corruption when the next entry pointer held by the watched keys iterator is freed when unwatching all keys of a specific client.
found with address sanitizer, added a test which will not always fail (depending on the random dict hashing seed)
problem introduced in #9829 (Reids 7.0)

This can potentially lead to use-after-free when the next entry
pointer held by the watched keys iterator is freed.
@ranshid
Copy link
Collaborator Author

ranshid commented Feb 28, 2023

@oranagra still not able to reproduce so maybe I will refactor the test once I am able to.

@oranagra oranagra changed the title Avoid calling unwatchAllKeys when running touchAllWatchedKeysInDb Fix possible memory corruption in FLUSHALL when a client watches more than one key Feb 28, 2023
tests/unit/multi.tcl Outdated Show resolved Hide resolved
@oranagra oranagra merged commit 18017df into redis:unstable Feb 28, 2023
@oranagra oranagra added the release-notes indication that this issue needs to be mentioned in the release notes label Feb 28, 2023
oranagra pushed a commit to oranagra/redis that referenced this pull request Feb 28, 2023
… than one key (redis#11854)

Avoid calling unwatchAllKeys when running touchAllWatchedKeysInDb (which was unnecessary)
This can potentially lead to use-after-free and memory corruption when the next entry
pointer held by the watched keys iterator is freed when unwatching all keys of a specific client.
found with address sanitizer, added a test which will not always fail (depending on the random
dict hashing seed)
problem introduced in redis#9829 (Reids 7.0)

Co-authored-by: Oran Agra <oran@redislabs.com>
(cherry picked from commit 18017df)
@oranagra oranagra mentioned this pull request Feb 28, 2023
oranagra pushed a commit that referenced this pull request Feb 28, 2023
… than one key (#11854)

Avoid calling unwatchAllKeys when running touchAllWatchedKeysInDb (which was unnecessary)
This can potentially lead to use-after-free and memory corruption when the next entry
pointer held by the watched keys iterator is freed when unwatching all keys of a specific client.
found with address sanitizer, added a test which will not always fail (depending on the random
dict hashing seed)
problem introduced in #9829 (Reids 7.0)

Co-authored-by: Oran Agra <oran@redislabs.com>
(cherry picked from commit 18017df)
@BCathcart
Copy link
Contributor

Would this also be a potential issue in touchWatchedKey() where similar behaviour is performed?

@oranagra
Copy link
Member

oranagra commented Mar 1, 2023

Would this also be a potential issue in touchWatchedKey() where similar behaviour is performed?

no, because it doesn't iterate on the db->watched_keys, only works on a single key.

Mixficsol pushed a commit to Mixficsol/redis that referenced this pull request Apr 12, 2023
… than one key (redis#11854)

Avoid calling unwatchAllKeys when running touchAllWatchedKeysInDb (which was unnecessary)
This can potentially lead to use-after-free and memory corruption when the next entry
pointer held by the watched keys iterator is freed when unwatching all keys of a specific client.
found with address sanitizer, added a test which will not always fail (depending on the random
dict hashing seed)
problem introduced in redis#9829 (Reids 7.0)

Co-authored-by: Oran Agra <oran@redislabs.com>
enjoy-binbin pushed a commit to enjoy-binbin/redis that referenced this pull request Jul 31, 2023
… than one key (redis#11854)

Avoid calling unwatchAllKeys when running touchAllWatchedKeysInDb (which was unnecessary)
This can potentially lead to use-after-free and memory corruption when the next entry
pointer held by the watched keys iterator is freed when unwatching all keys of a specific client.
found with address sanitizer, added a test which will not always fail (depending on the random
dict hashing seed)
problem introduced in redis#9829 (Reids 7.0)

Co-authored-by: Oran Agra <oran@redislabs.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-notes indication that this issue needs to be mentioned in the release notes
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants