Fix invalid dictNext usage when pubsubshard_channels became empty by enjoy-binbin · Pull Request #13033 · redis/redis

enjoy-binbin · 2024-02-06T11:18:14Z

After #12822, when pubsubshard_channels became empty, kvstoreDictDelete
will delete the dict (which is the only one currently deleting dicts that
become empty) and in the next loop, we will make an invalid call to dictNext.

After the dict becomes empty, we break out of the loop without calling dictNext.

After redis#12822, when pubsubshard_channels became empty, kvstoreDictDelete will delete the dict (which is the only one currently deleting dicts that become empty) and in the next loop, we will make an invalid call to dictNext. After the dict becomes empty, we break out of the loop without calling dictNext.

enjoy-binbin · 2024-02-06T11:18:43Z

i checked other places, we are goods.

enjoy-binbin · 2024-02-07T05:12:48Z

i noticed that after fixing this issue, this use-after-free appeared. The reason is because dictReleaseIterator will call dictResetIterator to use the dict.

Add a new dictFreeIterator to call zfree(iter) directly?

https://github.com/redis/redis/actions/runs/7807800679/job/21300793128#step:9:695

==50105==ERROR: AddressSanitizer: heap-use-after-free on address 0x6060000ad030 at pc 0x563536984323 bp 0x7ffc920d8340 sp 0x7ffc920d8338
READ of size 2 at 0x6060000ad030 thread T0
    #0 0x563536984322 in dictResetIterator /home/runner/work/redis/redis/src/dict.c:981:13
    #1 0x563536984322 in dictReleaseIterator /home/runner/work/redis/redis/src/dict.c:1041:5
    #2 0x563536b3db61 in removeChannelsInSlot /home/runner/work/redis/redis/src/cluster_legacy.c:5734:5
    #3 0x563536b3db61 in clusterDelSlot /home/runner/work/redis/redis/src/cluster_legacy.c:4929:5
    #4 0x563536b5eaf1 in clusterCommandSpecial /home/runner/work/redis/redis/src/cluster_legacy.c:6098:13
    #5 0x563536b2fa44 in clusterCommand /home/runner/work/redis/redis/src/cluster.c:955:16
    #6 0x5635369a0025 in call /home/runner/work/redis/redis/src/server.c:3551:5
    #7 0x5635369a3076 in processCommand /home/runner/work/redis/redis/src/server.c:4181:9
    #8 0x563536a1a31f in processCommandAndResetClient

oranagra · 2024-02-07T06:53:12Z

this is disturbing. @guybe7 FYI.
that kvstore deletes a dict that still has an active iterator.
can you go over the code (only pubsubshard_channels related), and check if there are any others places that have that risk?

the options i see are:

as you suggested, that the caller checks if the dict still exists (a NULL check, not a size check), and calls the right release function.
that somehow we avoid deleting the dict when it has a live iterator. this is complicated since even a safe iterator is only counted in pauserehash after the first dictNext call. also it's extremely ugly.
instead of releasing the iterator via dictReleaseIterator, we release it via a kvstore wrapper, so it can check if the dict still exists.

maybe 3 is the cleanest because we do create the iterator via a kvstore function, and we do the deletion via a kvstore function, and only the dict release is done directly.
but on the other hand, fixing it this way will be risky, since people have a dictIterator and they can call the dict function directly.
i suppose it's no riskier than 1, right? so let's do that.

guybe7 · 2024-02-07T07:00:03Z

@oranagra what about changing dictReleaseIterator to properly handle a NULL dict?

oranagra · 2024-02-07T07:20:30Z

@guybe7 who will nullify the dict pointer in the iterator?

After fix for redis#13033, address sanitizer reports this heap-use-after-free error. When the pubsubshard_channels dict becomes empty, we will delete the dict, and the dictReleaseIterator will call dictResetIterator, it will use the dict so we will trigger the error. Instead of releasing the iteraotr via dictReleaseIterator, we release it via a kvstore wrapper. This PR added a new kvstoreDictReleaseIterator to handle this.

After fix for #13033, address sanitizer reports this heap-use-after-free error. When the pubsubshard_channels dict becomes empty, we will delete the dict, and the dictReleaseIterator will call dictResetIterator, it will use the dict so we will trigger the error. This PR introduced a new struct kvstoreDictIterator to wrap dictIterator. Replace the original dict iterator with the new kvstore dict iterator. --------- Co-authored-by: Oran Agra <[email protected]> Co-authored-by: guybe7 <[email protected]>

…dis#13033) After redis#12822, when pubsubshard_channels became empty, kvstoreDictDelete will delete the dict (which is the only one currently deleting dicts that become empty) and in the next loop, we will make an invalid call to dictNext. After the dict becomes empty, we break out of the loop without calling dictNext.

…13038) After fix for redis#13033, address sanitizer reports this heap-use-after-free error. When the pubsubshard_channels dict becomes empty, we will delete the dict, and the dictReleaseIterator will call dictResetIterator, it will use the dict so we will trigger the error. This PR introduced a new struct kvstoreDictIterator to wrap dictIterator. Replace the original dict iterator with the new kvstore dict iterator. --------- Co-authored-by: Oran Agra <[email protected]> Co-authored-by: guybe7 <[email protected]>

…dis#13033) After redis#12822, when pubsubshard_channels became empty, kvstoreDictDelete will delete the dict (which is the only one currently deleting dicts that become empty) and in the next loop, we will make an invalid call to dictNext. After the dict becomes empty, we break out of the loop without calling dictNext.

…13038) After fix for redis#13033, address sanitizer reports this heap-use-after-free error. When the pubsubshard_channels dict becomes empty, we will delete the dict, and the dictReleaseIterator will call dictResetIterator, it will use the dict so we will trigger the error. This PR introduced a new struct kvstoreDictIterator to wrap dictIterator. Replace the original dict iterator with the new kvstore dict iterator. --------- Co-authored-by: Oran Agra <[email protected]> Co-authored-by: guybe7 <[email protected]>

enjoy-binbin mentioned this pull request Feb 6, 2024

Refactor the per-slot dict-array db.c into a new kvstore data structure #12822

Merged

enjoy-binbin requested a review from oranagra February 6, 2024 11:19

oranagra approved these changes Feb 6, 2024

View reviewed changes

oranagra merged commit 8096515 into redis:unstable Feb 6, 2024

enjoy-binbin deleted the fix_pubsubshard_channels branch February 6, 2024 13:50

enjoy-binbin mentioned this pull request Feb 7, 2024

Fix heap-use-after-free when pubsubshard_channels became NULL #13038

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix invalid dictNext usage when pubsubshard_channels became empty#13033

Fix invalid dictNext usage when pubsubshard_channels became empty#13033
oranagra merged 1 commit intoredis:unstablefrom
enjoy-binbin:fix_pubsubshard_channels

enjoy-binbin commented Feb 6, 2024

Uh oh!

enjoy-binbin commented Feb 6, 2024

Uh oh!

enjoy-binbin commented Feb 7, 2024

Uh oh!

oranagra commented Feb 7, 2024

Uh oh!

guybe7 commented Feb 7, 2024

Uh oh!

oranagra commented Feb 7, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

enjoy-binbin commented Feb 6, 2024

Uh oh!

enjoy-binbin commented Feb 6, 2024

Uh oh!

enjoy-binbin commented Feb 7, 2024

Uh oh!

oranagra commented Feb 7, 2024

Uh oh!

guybe7 commented Feb 7, 2024

Uh oh!

oranagra commented Feb 7, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants