Re: [PATCH 1/2] log: UNLEAK rev to silence a large number of leaks
From: Ævar Arnfjörð Bjarmason <hidden>
Date: 2021-09-19 16:29:44
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/ (local)
quoted hunk ↗ jump to hunk
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 <redacted> --- 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