Re: [PATCH v2 2/3] Fix memory leak in apply_patch in apply.c.

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 3, 2012, 21:51 UTC
Message-ID: <7v7gz173pk.fsf@alter.siamese.dyndns.org>
In-Reply-To: <e631bb2059c800f9d49eed51cfa5ba4d04106a2e.1330785363.git.jaredhance@gmail.com>

Jared Hance <jaredhance@gmail.com> writes:

Show 5 quoted lines

> 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.

Show 5 quoted lines

>
> 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.

Show 15 quoted lines

> Signed-off-by: Jared Hance <jaredhance@gmail.com>
> ---
>  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.