Re: [PATCH 01/11] rev-parse: fix a leak with --abbrev-ref
- From
Jeff King <peff@peff.net>
- Date
- Jun 12, 2023, 03:12 UTC
- Message-ID
- <20230612031239.GA306352@coredump.intra.peff.net>
- In-Reply-To
- <dc821efb-bf2a-10bb-4547-98d4ba11dbb3@gmail.com>
On Sun, Jun 11, 2023 at 08:49:17PM +0200, Rubén Justo wrote:
Show 7 quoted lines
> 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...
Show 34 quoted lines
> 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