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

On Mon, Sep 20, 2021 at 02:06:01AM -0400, Eric Sunshine wrote:

On Sun, Sep 19, 2021 at 5:34 PM Carlo Marcelo Arenas Belón
[off-list ref] wrote:

quoted

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

I haven't been following this thread closely, but the mention of the
static buffer in hex.c invalidates the premise of this patch, as far
as I can tell. The "static buffer" is actually a ring of four buffers
which oid_to_hex() uses, one after another, into which it formats an
OID as hex. This allows a caller to format up to -- and only up to --
four OIDs without worrying about allocating its own memory for the hex
result. Beyond four, the caller can't use oid_to_hex() without doing
some sort of memory management itself, whether that be duplicating the
result of oid_to_hex() or by allocating its own buffers and calling
oid_to_hex_r() instead.

Thanks; this then explains why as I was suspecting add_rev_cmdline_list()
was indeed buggy, and might had even triggered the workaround of doing the
strdup.

Therefore (if I'm reading this correctly), it is absolutely correct
for add_rev_cmdline() to be duplicating that string to ensure that the
hexified OID value remains valid, and incorrect for this patch to be
removing the call to xstrdup().

Indeed, but the values that are being strdup were never used anyway, so
I suspect the original code might had just put it as a logical default.

We might do better instead as shown in the following patch (again, second
hunk not relevant for the current code); I suspect if we were to land this,
the last hunks probably should be done first in an independent patch, as
well.

Carlo
-------- >8 --------
Subject: [PATCH] revision: remove xstrdup() of name in add_rev_cmdline()

a765499a08 (revision.c: treat A...B merge bases as if manually
specified, 2013-05-13) adds calls to this function in a loop,
abusing oid_to_hex (at that time called sha1_to_hex).

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

All names we will ever get should come from the commandline or be
constant values, so it is safe not to xstrdup and clean them up.

Just like the struct object *item, that is referenced in the same
struct, name isn't owned or managed so correct both issues by making
sure all entries are indeed constant or valid global pointers (from
the real command line) and remove the leak.

Signed-off-by: Carlo Marcelo Arenas Belón <redacted>
---
 revision.c | 15 +++++----------
 1 file changed, 5 insertions(+), 10 deletions(-)

diff --git a/revision.c b/revision.c
index ce62192dd8..829af28658 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;

@@ -1504,10 +1499,10 @@ static void add_rev_cmdline_list(struct rev_info *revs,
 				 int whence,
 				 unsigned flags)
 {
+	static const char *synthetic = ".synthetic";
 	while (commit_list) {
 		struct object *object = &commit_list->item->object;
-		add_rev_cmdline(revs, object, oid_to_hex(&object->oid),
-				whence, flags);
+		add_rev_cmdline(revs, object, synthetic, whence, flags);
 		commit_list = commit_list->next;
 	}
 }

@@ -1753,7 +1748,7 @@ struct add_alternate_refs_data {
 static void add_one_alternate_ref(const struct object_id *oid,
 				  void *vdata)
 {
-	const char *name = ".alternate";
+	static const char *name = ".alternate";
 	struct add_alternate_refs_data *data = vdata;
 	struct object *obj;
 

@@ -1940,7 +1935,7 @@ static int handle_dotdot_1(const char *arg, char *dotdot,
 			   struct object_context *a_oc,
 			   struct object_context *b_oc)
 {
-	const char *a_name, *b_name;
+	static const char *a_name, *b_name;
 	struct object_id a_oid, b_oid;
 	struct object *a_obj, *b_obj;
 	unsigned int a_flags, b_flags;

-- 
2.33.0.911.gbe391d4e11