Subject: Re: [PATCH v4 04/27] format-patch: don't leak "extra_headers" or "ref_message_ids"

From: Phillip Wood <[email-masked]>
Date: Fri, 1 Apr 2022 16:13:48 +0100

Hi Ævar

On 31/03/2022 02:11, Ævar Arnfjörð Bjarmason wrote:
> Fix two memory leaks in "struct rev_info" by freeing that memory in
> cmd_format_patch(). These two are unusual special-cases in being in
> the "struct rev_info", but not being "owned" by the code in
> revision.c. I.e. they're members of the struct so that this code in
> "builtin/log.c" can pass information code in log-tree.c.

I'm not sure that I necessarily agree that these are owned by 
builtin/log.c. For rev.extra_headers it is set in builtin/log.c but 
never used there which makes me think we are transferring ownership to 
struct rev_info. For ref_message_ids it is less clear cut but having it 
owned by struct rev_info and freeing it in release_revisions() would 
make things clearer I think. Having some members owned by struct 
rev_info but others allocated and freed by other code is confusing and 
is likely to lead to memory errors. I don't think struct rev_info is 
borrowing a reference to these items as they are being allocated for 
it's exclusive use.

Best Wishes

Phillip

> See 20ff06805c6 (format-patch: resurrect extra headers from config,
> 2006-06-02) and d1566f7883f (git-format-patch: Make the second and
> subsequent mails replies to the first, 2006-07-14) for the initial
> introduction of "extra_headers" and "ref_message_ids".
> 
> We can count on repo_init_revisions() memset()-ing this data to 0
> however, so we can count on it being either NULL or something we
> allocated. In the case of "extra_headers" let's add a local "char *"
> variable to hold it, to avoid the eventual cast from "const char *"
> when we free() it.
> 
> Signed-off-by: Ævar Arnfjörð Bjarmason <[email-masked]>
> ---
>   builtin/log.c | 7 ++++++-
>   1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/builtin/log.c b/builtin/log.c
> index 634dc782cce..6f9928fabfe 100644
> --- a/builtin/log.c
> +++ b/builtin/log.c
> @@ -1747,6 +1747,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
>   	struct commit *commit;
>   	struct commit **list = NULL;
>   	struct rev_info rev;
> +	char *to_free = NULL;
>   	struct setup_revision_opt s_r_opt;
>   	int nr = 0, total, i;
>   	int use_stdout = 0;
> @@ -1947,7 +1948,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
>   		strbuf_addch(&buf, '\n');
>   	}
>   
> -	rev.extra_headers = strbuf_detach(&buf, NULL);
> +	rev.extra_headers = to_free = strbuf_detach(&buf, NULL);
>   
>   	if (from) {
>   		if (split_ident_line(&rev.from_ident, from, strlen(from)))
> @@ -2284,6 +2285,10 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
>   	strbuf_release(&rdiff1);
>   	strbuf_release(&rdiff2);
>   	strbuf_release(&rdiff_title);
> +	free(to_free);
> +	if (rev.ref_message_ids)
> +		string_list_clear(rev.ref_message_ids, 0);
> +	free(rev.ref_message_ids);
>   	UNLEAK(rev);
>   	return 0;
>   }