Re: [PATCH 1/2] log: UNLEAK rev to silence a large number of leaks
flat view
From: Eric Sunshine <hidden>
Date: 2021-09-20 06:06:22
On Sun, Sep 19, 2021 at 5:34 PM Carlo Marcelo Arenas Belón [off-list ref] wrote:
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.
In this particular case, one of the callers of add_rev_cmdline() is
add_rev_cmdline_list(), which does this:
while (commit_list) {
...
add_rev_cmdline(..., oid_to_hex(...), ...);
...
}
which may call add_rev_cmdline() any number of times, quite possibly
more than four.
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().
quoted hunk ↗ jump to hunk
Signed-off-by: Carlo Marcelo Arenas Belón <redacted> ---diff --git a/revision.c b/revision.c@@ -1481,7 +1480,7 @@ static void add_rev_cmdline(struct rev_info *revs, info->rev[nr].item = item; - info->rev[nr].name = xstrdup(name); + info->rev[nr].name = name; info->rev[nr].whence = whence;@@ -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);