cephfs: free C.CString allocations in OpenSnapDiff#1254
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes a C-heap memory leak in OpenSnapDiff by ensuring C.CString() allocations are freed after calling into the dynamically-loaded ceph_open_snapdiff wrapper.
Changes:
- Extracted the four
C.CString()calls inOpenSnapDiffinto named variables. - Added
defer C.free(...)for each allocated C string to guarantee cleanup on success and early-return error paths.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
anoopcs9
left a comment
There was a problem hiding this comment.
Changes lgtm. See below for some process oriented suggestions:
- Commit message title should mention the component in which the change is proposed. Please replace
fixwithcephfs. - Commit message is missing
Signed-off-by:tag.
|
@lawrence3699 Are you planning to work on my above request? |
|
@Mergifyio update |
❌ Mergify doesn't have permission to updateDetailsFor security reasons, Mergify can't update this pull request. Try updating locally. |
0fc99a8 to
5fcd5d0
Compare
5fcd5d0 to
31cd264
Compare
I've taken the liberty of rebasing (and fixing the commit‑message title) the PR. @lawrence3699, I'm still unsure which name and email address should appear in the |
|
@anoopcs9 let's give @lawrence3699 two weeks to respond to your request for Sign-off. If @lawrence3699 is unable to respond by that time I suggest taking the patches into a branch we control and putting a Co-authored-by and then "owning" the patch ourselves with one of our sign offs. The patch is not trivial but it's not complex either and seems pretty straight forward. I think that would be a good compromise in case lawrence3699 doesn't respond, WDYT? |
That is very reasonable. But for small changes like this I would reduce the time to a few days. |
|
Thanks for the suggestion. I had picked two weeks because it's a decent vacation length. :-) |
31cd264 to
2829d74
Compare
Co-Authored-by: chaoliang yan (lawrence3699) Signed-off-by: Anoop C S <anoopcs@cryptolab.net>
2829d74 to
e5c636c
Compare
Given that the source branch is allowed to be updated by maintainers I've uploaded a modified version. Let me know whether we should proceed with the current branch or create a new one. |
|
Sounds good to me. |
|
Do we care about the failing pre-squid job? I think it's a packaging issue on the ceph side. But rather than just hit the approve button I felt I should double check. |
I wanted to at least try and see what's wrong, but the |
Merge Queue Status
This pull request spent 20 seconds in the queue, including 3 seconds running CI. Required conditions to merge
|
Summary
C.CString()allocates C heap memory that must be freed withC.free(). The 4 string allocations passed toopen_snapdiff_dlsym()are never freed, causing a memory leak on every call. If the function returns early due to an error, all 4 strings leak.Extracted each
C.CString()into a named variable with a correspondingdefer C.free().Test plan