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.

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

Fix memory leak in streamGetEdgeID #10753

Merged
merged 1 commit into from
May 22, 2022
Merged

Fix memory leak in streamGetEdgeID #10753

merged 1 commit into from
May 22, 2022

Conversation

Yuuoniy
Copy link
Contributor

@Yuuoniy Yuuoniy commented May 19, 2022 •
edited
Loading

si is initialized by streamIteratorStart(), we should call streamIteratorStop() on it when done. #10752

@Yuuoniy
Fix memory leak in streamGetEdgeID
61775e0 
si is initialized by streamIteratorStart(), we should call
streamIteratorStop() on it when done.
@enjoy-binbin enjoy-binbin added the state:to-be-merged The PR should be merged soon, even if not yet ready, this is used so that it won't be forgotten label May 19, 2022
@oranagra oranagra linked an issue May 22, 2022 that may be closed by this pull request
[BUG] memory leak in streamGetEdgeID #10752
Closed
@oranagra
Copy link
Member

@Yuuoniy thank you.
p.s. the reason why this doesn't usually leak is that rax iterator has static buffers for short keys and shallow stacks.
for streams, the keys are always short, so the key would never leak any memory.
but the stack may leak on large streams.

this looks like a regression in redis 7.0 (#9127) that could in cause leaks in trimming (XADD, XTRIM), and also XDEL and AOFRW.

@itamarhaber @guybe7 correct me if i'm wrong.

@oranagra oranagra merged commit 4a7a4e4 into redis:unstable May 22, 2022
@oranagra oranagra added the release-notes indication that this issue needs to be mentioned in the release notes label May 22, 2022
@Yuuoniy
Copy link
Contributor Author

Yuuoniy commented May 22, 2022

@oranagra I see, thanks for your explanation. I found this one when went through the PR #10353 , and thought it will result in memory leak as well.

@oranagra oranagra mentioned this pull request Jun 8, 2022
Merged
@oranagra
Copy link
Member

Apparently there's a CVE for this issue: GHSA-35rf-7vhx-9phr

enjoy-binbin pushed a commit to enjoy-binbin/redis that referenced this pull request Jul 31, 2023
@Yuuoniy @enjoy-binbin
Fix memory leak in streamGetEdgeID (redis#10753)
ded592d 
si is initialized by streamIteratorStart(), we should call
streamIteratorStop() on it when done.

regression introduced in redis#9127 (redis 7.0)
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

@enjoy-binbin enjoy-binbin enjoy-binbin approved these changes

Assignees
No one assigned
Labels
release-notes indication that this issue needs to be mentioned in the release notes state:to-be-merged The PR should be merged soon, even if not yet ready, this is used so that it won't be forgotten
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[BUG] memory leak in streamGetEdgeID
3 participants
@Yuuoniy @oranagra @enjoy-binbin