transports: smart: fix memory leak when skipping symbolic refs#4368
Conversation
| goto on_error; | ||
| goto on_continue; | ||
|
|
||
| on_continue: |
There was a problem hiding this comment.
This label name is misleading, as we don't just go here during continue, but for each iteration.
There was a problem hiding this comment.
Yeah, I had some problems here finding a proper name
| git_revwalk_sorting(walk, GIT_SORT_TIME); | ||
|
|
||
| for (i = 0; i < refs.count; ++i) { | ||
| git_reference *ref; |
There was a problem hiding this comment.
This is not guaranteed to be NULL so if we don't match the GIT_REFS_TAGS_DIR (or match it, whichever way around), then we call git_reference_free on an uninitialised value.
|
How about performing a free at the beginning of the loop plus at the end of the function? That should cover both cases without the need for a hand-rolled |
|
If we do free at the beginning of the loop, we'd require three different locations where we free the reference: at the beginning of the loop, after the loop and on success. That's really painful, in my opinion, which is why I went with the thing at the end of the loop. |
When we setup the revision walk for negotiating references with a remote, we iterate over all references, ignoring tags and symbolic references. While skipping over symbolic references, we forget to free the looked up reference, resulting in a memory leak when the next iteration simply overwrites the variable. Fix that issue by freeing the reference at the beginning of each iteration and collapsing return paths for error and success.
pks-t
force-pushed
the
pks/smart-negotiate-revwalk-memleak
branch
from
35f78b6 to
7cb705c
Compare
|
Ah, we're actually able to collapse the two different out paths. So this version looks good to me. |
|
Thanks, @pks-t, this is much easier to read. |
3 participants
When we setup the revision walk for negotiating references with a
remote, we iterate over all references, ignoring tags and symbolic
references. While skipping over symbolic references, we forget to free
the looked up reference, resulting in a memory leak when the next
iteration simply overwrites the variable.
Fix that issue by more clearly scoping the leaking variable to the loop
and freeing it after every iteration.