Re: [PATCH 01/11] rev-parse: fix a leak with --abbrev-ref
From: Jeff King <hidden>
Date: 2023-06-12 03:13:26
Subsystem:
the rest · Maintainer:
Linus Torvalds
On Sun, Jun 11, 2023 at 08:49:17PM +0200, Rubén Justo wrote:
To handle "--abbrev-ref" we use shorten_unambiguous_ref(). This function takes a refname and returns a shortened refname, which is a newly allocated string that needs to be freed. Unfortunately, the refname variable is reused to receive the shortened one. Therefore, we lose the original refname, which needs to be freed as well, producing a leak.
Makes sense, but...
quoted hunk ↗ jump to hunk
diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c index 852e49e340..9fd7095431 100644 --- a/builtin/rev-parse.c +++ b/builtin/rev-parse.c@@ -141,7 +141,7 @@ static void show_rev(int type, const struct object_id *oid, const char *name) if ((symbolic || abbrev_ref) && name) { if (symbolic == SHOW_SYMBOLIC_FULL || abbrev_ref) { struct object_id discard; - char *full; + char *full, *to_free = NULL; switch (repo_dwim_ref(the_repository, name, strlen(name), &discard, &full,@@ -156,9 +156,11 @@ static void show_rev(int type, const struct object_id *oid, const char *name) */ break; case 1: /* happy */ - if (abbrev_ref) + if (abbrev_ref) { + to_free = full; full = shorten_unambiguous_ref(full, abbrev_ref_strict); + } show_with_type(type, full); break; default: /* ambiguous */@@ -166,6 +168,7 @@ static void show_rev(int type, const struct object_id *oid, const char *name) break; } free(full); + free(to_free); } else { show_with_type(type, name); }
The lifetime of "to_free" is much longer than it needs to be here. We'd usually use a "to_free" pattern when the memory is aliased to another (usually const) pointer, and the allocation needs to live as long as that other pointer. But here, nobody cares about the old value as soon as we replace it. We could almost just free() it ahead of time, but of course we also need to pass it to the shortening function. So maybe:
diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
index 852e49e340..6dc8548e1f 100644
--- a/builtin/rev-parse.c
+++ b/builtin/rev-parse.c@@ -156,9 +156,12 @@ static void show_rev(int type, const struct object_id *oid, const char *name) */ break; case 1: /* happy */ - if (abbrev_ref) - full = shorten_unambiguous_ref(full, + if (abbrev_ref) { + char *old = full; + full = shorten_unambiguous_ref(old, abbrev_ref_strict); + free(old); + } show_with_type(type, full); break; default: /* ambiguous */ -Peff