Re: [PATCHv3 6/6] entry.c: fix a memleak

[Junio C Hamano <gitster@pobox.com>]

Stefan Beller <sbeller@google.com> writes:

> From: John Keeping <john@keeping.me.uk>
>
> stream_blob_to_fd() always frees the filter now, so there is no memory
> leak in entry.c:152 just before the `goto finish`.
>
> Signed-off-by: John Keeping <john@keeping.me.uk>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>  
>  I added Johns signoff here tentatively, John can you confirm the sign off?
>  
>  streaming.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/streaming.c b/streaming.c
> index 2ff036a..811fcc2 100644
> --- a/streaming.c
> +++ b/streaming.c
> @@ -507,8 +507,11 @@ int stream_blob_to_fd(int fd, unsigned const char *sha1, struct stream_filter *f
>  	int result = -1;
>  
>  	st = open_istream(sha1, &type, &sz, filter);
> -	if (!st)
> +	if (!st) {
> +		if (filter)
> +			free_stream_filter(filter);
>  		return result;
> +	}

I think this one is fine. I see you moved the callsite to free this
thing from entry.c down to streaming (stream_blob_to_fd()), which
means you would need to adjust the title of the change, too.

It somewhat feels dirty that we free what the caller gave us, but
given that closing a filtered stream frees the filter at the end,
I think this is the right place to discard the filter.  In essence,
the overall use of the API in this code is equivalent to

	filter = ... prepare a filter ...
        if (open_istream(... filter) == OK) {
		... use filter ...
                close_istream(); /* which frees the filter */
	} else {
           	/* we failed to free filter here without the patch */
		free_stream_filter(filter);
	}

so the patch makes sense.

We may want to teach free_stream_filter() that a request to free a
NULL filter is OK, though. That would be a separate clean-up topic.


>  	if (type != OBJ_BLOB)
>  		goto close_and_exit;
>  	for (;;) {