Re: [PATCH 1/2] bundle: plug resource leak

From: Jeff King <peff@peff.net>
Date: Mar 2, 2016, 08:54 UTC
Message-ID: <20160302085432.GB30295@sigill.intra.peff.net>
In-Reply-To: <xmqqvb556abd.fsf@gitster.mtv.corp.google.com>

On Tue, Mar 01, 2016 at 03:35:34PM -0800, Junio C Hamano wrote:

Show 29 quoted lines

> The bundle header structure holds two lists of refs and object
> names, which should be released when the user is done with it.
> 
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>  bundle.c    | 12 ++++++++++++
>  bundle.h    |  1 +
>  transport.c |  1 +
>  3 files changed, 14 insertions(+)
> 
> diff --git a/bundle.c b/bundle.c
> index 506ac49..9c5a6f0 100644
> --- a/bundle.c
> +++ b/bundle.c
> @@ -102,6 +102,18 @@ int is_bundle(const char *path, int quiet)
>  	return (fd >= 0);
>  }
>  
> +void release_bundle_header(struct bundle_header *header)
> +{
> +	int i;
> +
> +	for (i = 0; i < header->prerequisites.nr; i++)
> +		free(header->prerequisites.list[i].name);
> +	free(header->prerequisites.list);
> +	for (i = 0; i < header->references.nr; i++)
> +		free(header->references.list[i].name);
> +	free(header->references.list);
> +}

Looks good. It's probably not worth adding a release_ref_list() to handle the repeated data structures.

I do find it hard to believe that the bundle code had to invent its own ref storage data structure, and couldn't just use "struct ref" like all of the other code. It doesn't look like we ever sort it or do non-sequential access. The linked-list "struct ref" probably would have been fine.

Not a problem you are introducing, of course, but if you are touching this code a lot, it might be worth seeing how painful it is.

-Peff