-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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.
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
Fix assorted leaks found via fuzzing #4698
Conversation
git__free is insufficient if the packet is a git_pkt_ref or another type that requires freeing referenced structures.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this PR, the fixes look obviously good to me. I'd appreciate it if you could include one more commit to get rid of this if (pkt) git_pkt_free(pkt)
mess, even though it's not your fault but ours. :)
src/transports/smart_protocol.c
Outdated
|
||
while (1) { | ||
git__free(pkt); | ||
if (pkt) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We usually do not check whether a pointer is set before freeing it. But in this case it is necessary, as git_pkt_free
doesn't check for NULL
pointers itself
src/transports/smart_protocol.c
Outdated
@@ -622,7 +622,9 @@ int git_smart__download_pack( | |||
} | |||
} | |||
|
|||
git__free(pkt); | |||
if (pkt) { | |||
git_pkt_free(pkt); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Huh, one more time. I know it's not your fault, but instead the existing code is to blame. But I'd highly appreciate if you did a preliminary commit which fixes git_pkt_free
to return early in case a NULL
pointer is being passed to it.
By the way, did you already see #4433? |
I hadn't! Thanks for the pointer, I'll look into that past attempt. |
Fixed |
Thanks for this. 🎉 |
These were all found by my work-in-progress oss-fuzz fuzzer.