From: Andrzej Hunt <andrzej@ahunt.org>
Date: Sun, 19 Sep 2021 15:51:38 GMT
Subject: Re: [PATCH 1/2] log: UNLEAK rev to silence a large number of leaks
Message-ID: <fd65fe57-819e-88d5-8ba1-99bb59a980bb@ahunt.org>
In-Reply-To: <YUZG0D5ayEWd7MLP@carlos-mbp.lan>



On 18/09/2021 22:06, 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.
> 
> 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);
>                         ^~~~~~~~~~~~~~~~~
> 

Casting the pointer a la "free((void *) ...)" seems to be a common 
pattern in git, and seems like a reasonable option here. AFAIUI the 
const is still needed because clients  of rev_cmdline_info shouldn't be 
changing name. But since we own and created rev_cmdline_info, we also 
know it's safe to clean it up. For comparison, here's an example of 
submodule_entry being cleaned up - all members end up needing a cast:

static void free_one_config(struct submodule_entry *entry)
{
	free((void *) entry->config->path);
	free((void *) entry->config->name);
	free((void *) entry->config->branch);
	free((void *) entry->config->update_strategy.command);
	free(entry->config);
}

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

This patch looks good to me (modulo adding the cast as discussed above), 
and is obviously much better than my approach of using an UNLEAK!

ATB,

Andrzej