| Message ID | dc821efb-bf2a-10bb-4547-98d4ba11dbb3@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [01/11] rev-parse: fix a leak with --abbrev-ref | expand |
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... > 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
On 11-jun-2023 23:12:39, Jeff King wrote: > 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 */ Simpler. Nice. Thanks!
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); }
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. This leak can be reviewed with: $ for a in {1..10}; do git branch foo_${a}; done $ git rev-parse --abbrev-ref refs/heads/foo_{1..10} Direct leak of 171 byte(s) in 10 object(s) allocated from: ... in xstrdup wrapper.c ... in expand_ref refs.c ... in repo_dwim_ref refs.c ... in show_rev builtin/rev-parse.c ... in cmd_rev_parse builtin/rev-parse.c ... in run_builtin git.c We have this leak since a45d34691e (rev-parse: --abbrev-ref option to shorten ref name, 2009-04-13) when "--abbrev-ref" was introduced. Let's fix it. Signed-off-by: Rubén Justo <rjusto@gmail.com> --- builtin/rev-parse.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)