Fix memory leak in saveSnapshotToConnectionSockets#2503
Merged
Conversation
We now pass in rdbSnapshotOptions options in this function, and options.conns is now malloc'ed in the caller side, so we need to zfree it when returning early due to an error. Previously, conns was malloc'ed after the error handling, so we don't have this. Introduced in valkey-io#1949. Signed-off-by: Binbin <binloveplay1314@qq.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## unstable #2503 +/- ##
============================================
+ Coverage 72.01% 72.04% +0.02%
============================================
Files 126 126
Lines 70448 70459 +11
============================================
+ Hits 50734 50759 +25
+ Misses 19714 19700 -14
🚀 New features to boost your workflow:
|
zuiderkwast
approved these changes
Aug 18, 2025
zuiderkwast
left a comment
Contributor
There was a problem hiding this comment.
The top comment of this function has this comment:
* Connections array provided will be freed after the save is completed, and
* should not be freed by the caller. */We should update it to say that this array is freed by this function (success or failure doesn't matter). Maybe also clarify that it's the conns field in the rdbSnapshotOptions struct. It's also required that this array is heap allocated. Otherwise it can't be freed. Idea:
* The connections array (the conns field in the rdbSnapshotOptions) is a
* heap-allocated array that will be freed by this function and shall not be
* freed by the caller. */It's a little odd that this struct is passed by value, but it contains a heap-allocated array. Maybe we could use a different design, but the simple fix is just like this PR and to update the comment.
Signed-off-by: Binbin <binloveplay1314@qq.com>
Contributor
|
Is it this leak seen in Daily today? https://github.com/valkey-io/valkey/actions/runs/17027650597/job/48265126023#step:6:7328 |
Member
Author
|
that one was fixed in #2502 |
allenss-amazon
pushed a commit
to allenss-amazon/valkey-core
that referenced
this pull request
Aug 19, 2025
We now pass in rdbSnapshotOptions options in this function, and options.conns is now malloc'ed in the caller side, so we need to zfree it when returning early due to an error. Previously, conns was malloc'ed after the error handling, so we don't have this. Introduced in valkey-io#1949. --------- Signed-off-by: Binbin <binloveplay1314@qq.com>
asagege
pushed a commit
to asagege/valkey
that referenced
this pull request
Aug 19, 2025
We now pass in rdbSnapshotOptions options in this function, and options.conns is now malloc'ed in the caller side, so we need to zfree it when returning early due to an error. Previously, conns was malloc'ed after the error handling, so we don't have this. Introduced in valkey-io#1949. --------- Signed-off-by: Binbin <binloveplay1314@qq.com>
rjd15372
pushed a commit
to rjd15372/valkey
that referenced
this pull request
Sep 19, 2025
We now pass in rdbSnapshotOptions options in this function, and options.conns is now malloc'ed in the caller side, so we need to zfree it when returning early due to an error. Previously, conns was malloc'ed after the error handling, so we don't have this. Introduced in valkey-io#1949. --------- Signed-off-by: Binbin <binloveplay1314@qq.com>
rjd15372
pushed a commit
that referenced
this pull request
Sep 23, 2025
We now pass in rdbSnapshotOptions options in this function, and options.conns is now malloc'ed in the caller side, so we need to zfree it when returning early due to an error. Previously, conns was malloc'ed after the error handling, so we don't have this. Introduced in #1949. --------- Signed-off-by: Binbin <binloveplay1314@qq.com>
hpatro
pushed a commit
to hpatro/valkey
that referenced
this pull request
Oct 3, 2025
We now pass in rdbSnapshotOptions options in this function, and options.conns is now malloc'ed in the caller side, so we need to zfree it when returning early due to an error. Previously, conns was malloc'ed after the error handling, so we don't have this. Introduced in valkey-io#1949. --------- Signed-off-by: Binbin <binloveplay1314@qq.com> Signed-off-by: Harkrishn Patro <harkrisp@amazon.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
We now pass in rdbSnapshotOptions options in this function, and options.conns
is now malloc'ed in the caller side, so we need to zfree it when returning early
due to an error. Previously, conns was malloc'ed after the error handling, so we
don't have this.
Introduced in #1949.