Re: [PATCH 1/2] log: UNLEAK rev to silence a large number of leaks

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Sat, Sep 18 2021, Carlo Marcelo Arenas Belón wrote:

> My equivalent version for these fixes is obviously more verbose but IMHO
> not that ugly (and as safe)
>
> It avoids the need to UNLEAK early by changing the program flow also for
> the early return so the cleanup could be centralized in one single
> function.
>
> Both, the cmdline and mailmap arrays (and the objects they accumulate)
> are cleaned in a "reusable" way.
>
> Note that the cleaning of the "name" in the cmdline item throws a warning
> as shown below which I intentionally didn't fix, as it would seem that
> either the use of const there or the need to strdup is wrong.  So hope
> someone that knows this code better could chime in.

It should just be a "char *", I got that wrong in my version posted in
the side-thread[1] & mentioned in the side-reply[2].

(I think I got it right in some earlier version days ago, it should be a
'char *' like anyting we malloc, but brainfart when re-doing/re-basing
those changes).

Yours here below has a bug where you free() rev_cmdline_info items, you
need to use release_revisions_cmdline_rev().

I should have said in [2], but thanks a lot to you and Andrzej for
following up on the mess in t0000-basic.sh addressed by my v7
re-roll. It'll be really nice to get some of these leaks fixed & tested
for.

I think I was rather curt in [2] after a long debugging session, just
saying I appreciate it. Hopefully we can figure out some plan for mostly
pulling in the same direction with regards to the way forward.

1. https://github.com/git/git/commit/06380cd4f56f4c542685eb7aa79e28285fe02c55
2. https://lore.kernel.org/git/87a6k8daeu.fsf@evledraar.gmail.com/

> Carlo
> ------ >8 ------
> Subject: [PATCH] builtin/log: leaks from `git show` in t0000
>
> obviously not ready, since the following will need to be corrected:
>
>   revision.c:1496:8: warning: passing 'const char *' to parameter of type 'void *' discards qualifiers [-Wincompatible-pointer-types-discards-qualifiers]
>                   free(info->rev[i].name);
>                        ^~~~~~~~~~~~~~~~~
>
> Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
> ---
>  builtin/log.c |  8 ++++++--
>  revision.c    | 20 ++++++++++++++++++++
>  revision.h    |  5 +++++
>  3 files changed, 31 insertions(+), 2 deletions(-)
>
> diff --git a/builtin/log.c b/builtin/log.c
> index f75d87e8d7..1b1c1f53f4 100644
> --- a/builtin/log.c
> +++ b/builtin/log.c
> @@ -645,8 +645,10 @@ int cmd_show(int argc, const char **argv, const char *prefix)
>  	opt.tweak = show_setup_revisions_tweak;
>  	cmd_log_init(argc, argv, prefix, &rev, &opt);
>  
> -	if (!rev.no_walk)
> -		return cmd_log_walk(&rev);
> +	if (!rev.no_walk) {
> +		ret = cmd_log_walk(&rev);
> +		goto done;
> +	}
>  
>  	count = rev.pending.nr;
>  	objects = rev.pending.objects;
> @@ -702,6 +704,8 @@ int cmd_show(int argc, const char **argv, const char *prefix)
>  		}
>  	}
>  	free(objects);
> +done:
> +	repo_clear_revisions(&rev);
>  	return ret;
>  }
>  
> diff --git a/revision.c b/revision.c
> index 0dabb5a0bc..ce62192dd8 100644
> --- a/revision.c
> +++ b/revision.c
> @@ -1487,6 +1487,18 @@ static void add_rev_cmdline(struct rev_info *revs,
>  	info->nr++;
>  }
>  
> +static void clear_rev_cmdline(struct rev_info *revs)
> +{
> +	struct rev_cmdline_info *info = &revs->cmdline;
> +	size_t i, nr = info->nr;
> +
> +	for (i = 0; i < nr; i++)
> +		free(info->rev[i].name);
> +
> +	FREE_AND_NULL(info->rev);
> +	info->nr = info->alloc = 0;
> +}
> +
>  static void add_rev_cmdline_list(struct rev_info *revs,
>  				 struct commit_list *commit_list,
>  				 int whence,
> @@ -1845,6 +1857,14 @@ void repo_init_revisions(struct repository *r,
>  	init_display_notes(&revs->notes_opt);
>  }
>  
> +void repo_clear_revisions(struct rev_info *revs)
> +{
> +	if (revs->mailmap)
> +		clear_mailmap(revs->mailmap);
> +	FREE_AND_NULL(revs->mailmap);
> +	clear_rev_cmdline(revs);
> +}
> +
>  static void add_pending_commit_list(struct rev_info *revs,
>  				    struct commit_list *commit_list,
>  				    unsigned int flags)
> diff --git a/revision.h b/revision.h
> index 0c65a760ee..f695c41cee 100644
> --- a/revision.h
> +++ b/revision.h
> @@ -358,6 +358,11 @@ void repo_init_revisions(struct repository *r,
>  			 struct rev_info *revs,
>  			 const char *prefix);
>  
> +/*
> + * Free all structures dynamically allocated for the provided rev_info
> + */
> +void repo_clear_revisions(struct rev_info *revs);
> +
>  /**
>   * Parse revision information, filling in the `rev_info` structure, and
>   * removing the used arguments from the argument list. Returns the number