Skip to content

Fix invalid dictNext usage when pubsubshard_channels became empty #13033

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.

Sign up for GitHub

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

Jump to bottom
Merged
oranagra merged 1 commit into from
Feb 6, 2024
Merged

Fix invalid dictNext usage when pubsubshard_channels became empty #13033

oranagra merged 1 commit into from
Feb 6, 2024

Conversation

@enjoy-binbin
Copy link
Contributor

@enjoy-binbin enjoy-binbin commented Feb 6, 2024

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.

@enjoy-binbin
Fix invalid dictNext usage when pubsubshard_channels became empty
41a37e5 
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
Copy link
Contributor Author

enjoy-binbin commented Feb 6, 2024

i checked other places, we are goods.

@enjoy-binbin enjoy-binbin mentioned this pull request Feb 6, 2024
Merged
@enjoy-binbin enjoy-binbin requested a review from oranagra February 6, 2024 11:19
@oranagra oranagra merged commit 8096515 into redis:unstable Feb 6, 2024
@enjoy-binbin enjoy-binbin deleted the fix_pubsubshard_channels branch February 6, 2024 13:50
@enjoy-binbin
Copy link
Contributor Author

enjoy-binbin commented Feb 7, 2024

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

oranagra commented Feb 7, 2024

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:

  1. 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.
  2. 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.
  3. 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
Copy link
Collaborator

guybe7 commented Feb 7, 2024

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

@oranagra
Copy link
Member

oranagra commented Feb 7, 2024

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

enjoy-binbin added a commit to enjoy-binbin/redis that referenced this pull request Feb 7, 2024
@enjoy-binbin
Fix heap-use-after-free when pubsubshard_channels became NULL
930dc9e 
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.
@enjoy-binbin enjoy-binbin mentioned this pull request Feb 7, 2024
Merged
oranagra added a commit that referenced this pull request Feb 7, 2024
@enjoy-binbin @oranagra @guybe7
Fix heap-use-after-free when pubsubshard_channels became NULL (#13038)
81666a6 
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 <oran@redislabs.com>
Co-authored-by: guybe7 <guy.benoish@redislabs.com>
roggervalf pushed a commit to roggervalf/redis that referenced this pull request Feb 11, 2024
@enjoy-binbin @roggervalf
Fix invalid dictNext usage when pubsubshard_channels became empty (re…
b3aabb8 
…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.
roggervalf pushed a commit to roggervalf/redis that referenced this pull request Feb 11, 2024
@enjoy-binbin @oranagra
@guybe7 @roggervalf
Fix heap-use-after-free when pubsubshard_channels became NULL (redis#…
d07a9bc 
…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 <oran@redislabs.com>
Co-authored-by: guybe7 <guy.benoish@redislabs.com>
funny-dog pushed a commit to funny-dog/redis that referenced this pull request Sep 17, 2025
@enjoy-binbin
Fix invalid dictNext usage when pubsubshard_channels became empty (re…
aab5490 
…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.
funny-dog pushed a commit to funny-dog/redis that referenced this pull request Sep 17, 2025
@enjoy-binbin @oranagra @guybe7
Fix heap-use-after-free when pubsubshard_channels became NULL (redis#…
c787866 
…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 <oran@redislabs.com>
Co-authored-by: guybe7 <guy.benoish@redislabs.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@oranagra oranagra oranagra approved these changes

Assignees

No one assigned

Labels

None yet

Projects

Redis 7.4
Status: Done

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@enjoy-binbin @oranagra @guybe7