Re: [PATCH v2 2/3] Fix memory leak in apply_patch in apply.c.
From: Junio C Hamano <hidden>
Date: 2016-06-15 22:53:12
Jared Hance [off-list ref] writes:
In addition, the list of fragments should be free'd. To fix this, the utility function free_patch has been implemented. It loops over the entire patch list, and in each patch, loops over the fragment list, freeing the fragments, followed by the patch in the list. It frees both patch and patch->next.
Right encapsulation and abstraction. Good.
The main caveat is that the text in a fragment, ie, patch->fragments->patch, may or may not need to be free'd. The text is dynamically allocated and needs to be freed iff the patch is a binary patch, as allocation occurs in inflate_it.
Can't we do better than "is this for a binary patch"? I find this part
not very forward-thinking implementation that relies on an implementation
detail that happens to hold true for today's code.
At least
if ((buf.buf <= fragment->patch &&
(fragment->patch < buf.buf + buf.len))
; /* this is inside the original buffer */
else
free(fragment->patch);
or something?
Strictly speaking, even the above is not forward-thinking enough, as the
code deep in the callchain is free to replace ->patch with an unfreeable
string. I think the right way to handle this is to add a single bitfield
"should_free" to the struct fragment and default it to 'false', and make
the place that replace the patch field with different string responsible
for flipping the bit to 'true'. Your "free_patch()" can then rely on that
bit to make the decision.
quoted hunk
Signed-off-by: Jared Hance <redacted> --- builtin/apply.c | 30 +++++++++++++++++++++++++++--- 1 files changed, 27 insertions(+), 3 deletions(-)diff --git a/builtin/apply.c b/builtin/apply.c index 389898f..a73d339 100644 --- a/builtin/apply.c +++ b/builtin/apply.c@@ -196,6 +196,30 @@ struct patch { struct patch *next; }; +static void free_patch(struct patch *patch) { + while(patch != NULL) {
Style:
static void free_patch(struct patch *patch)
{
while (patch) {
...
Thanks.