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

["Carlo Marcelo Arenas Belón" <carenas@gmail.com>]

On Sun, Sep 19, 2021 at 06:13:43PM +0200, Ævar Arnfjörð Bjarmason wrote:
> 
> On Sat, Sep 18 2021, Carlo Marcelo Arenas Belón wrote:
> 
> > 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 was instead leaning towards keeping it as a "const char *" and removing
the strdup as shown in the patch below (obviously the last hunk not relevant
to your series).

This object doesn't hold or even manipulate, the objects it contains, so
it might be also a cleaner API to ensure it only keeps references and
doesn't own any in the more CS sense (note I am not a CS guy, so maybe I
get the concept here wrong).

Ironically the original patch that added the strdup was because of leak
related work, but I think that in this case might had gotten it backwards.

Even if we start holding pointers to names that are not static, I would
expect whoever created those buffers to own the data anyway.

Carlo

CC Michael for advise as the original author
------ >8 ------
Subject: [PATCH] revision: remove dup() of name in add_rev_cmdline()

df835d3a0c (add_rev_cmdline(): make a copy of the name argument,
2013-05-25) adds it, probably introducing a leak.

All names we will ever get will either come from the commandline
or be pointers to a static buffer in hex.c, so it is safe not to
xstrdup and clean them up (just like the struct object *item).

Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com>
---
 revision.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/revision.c b/revision.c
index ce62192dd8..b20bc58ccd 100644
--- a/revision.c
+++ b/revision.c
@@ -1468,7 +1468,6 @@ static int limit_list(struct rev_info *revs)
 
 /*
  * Add an entry to refs->cmdline with the specified information.
- * *name is copied.
  */
 static void add_rev_cmdline(struct rev_info *revs,
 			    struct object *item,
@@ -1481,7 +1480,7 @@ static void add_rev_cmdline(struct rev_info *revs,
 
 	ALLOC_GROW(info->rev, nr + 1, info->alloc);
 	info->rev[nr].item = item;
-	info->rev[nr].name = xstrdup(name);
+	info->rev[nr].name = name;
 	info->rev[nr].whence = whence;
 	info->rev[nr].flags = flags;
 	info->nr++;
@@ -1490,10 +1489,6 @@ static void add_rev_cmdline(struct rev_info *revs,
 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;
-- 
2.33.0.911.gbe391d4e11