| Message ID | patch-09.20-77fcdeb9284-20221228T175512Z-avarab@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | leak fixes: various simple leak fixes | expand |
Am 28.12.22 um 19:00 schrieb Ævar Arnfjörð Bjarmason: > When "add_to_tip_table()" is called with a non-zero > "shorten_unambiguous" we always return an xstrdup()'d string, which > we'd then xstrdup() again, leaking memory. See [1] and [2] for how > this leak came about. > > 1. 98c5c4ad015 (name-rev: allow to specify a subpath for --refs > option, 2013-06-18) > 2. b23e0b9353e (name-rev: allow converting the exact object name at > the tip of a ref, 2013-07-07) > > Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> > --- > builtin/name-rev.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/builtin/name-rev.c b/builtin/name-rev.c > index 15535e914a6..24f4438eb01 100644 > --- a/builtin/name-rev.c > +++ b/builtin/name-rev.c > @@ -313,7 +313,8 @@ static void add_to_tip_table(const struct object_id *oid, const char *refname, > > ALLOC_GROW(tip_table.table, tip_table.nr + 1, tip_table.alloc); > oidcpy(&tip_table.table[tip_table.nr].oid, oid); > - tip_table.table[tip_table.nr].refname = xstrdup(refname); > + tip_table.table[tip_table.nr].refname = shorten_unambiguous ? refname : > + xstrdup(refname); Hmm, this works based on knowledge about the inner workings of name_ref_abbrev(), which provides the refname. Could be cleaned up by inlining that short function, or by moving the xstrdup() call there. > tip_table.table[tip_table.nr].commit = commit; > tip_table.table[tip_table.nr].taggerdate = taggerdate; > tip_table.table[tip_table.nr].from_tag = from_tag;
René Scharfe <l.s.r@web.de> writes: >> - tip_table.table[tip_table.nr].refname = xstrdup(refname); >> + tip_table.table[tip_table.nr].refname = shorten_unambiguous ? refname : >> + xstrdup(refname); > > Hmm, this works based on knowledge about the inner workings of > name_ref_abbrev(), which provides the refname. Could be cleaned up by > inlining that short function, or by moving the xstrdup() call there. Yeah, name_ref_abbrev() returns sometimes an allocated and some other times a borrowed piece of memory, which is a poor design that ignores memory ownership issues. Luckily the function being touched is the sole caller of it, and I agree with you that inlining may give us a better fix.
diff --git a/builtin/name-rev.c b/builtin/name-rev.c index 15535e914a6..24f4438eb01 100644 --- a/builtin/name-rev.c +++ b/builtin/name-rev.c @@ -313,7 +313,8 @@ static void add_to_tip_table(const struct object_id *oid, const char *refname, ALLOC_GROW(tip_table.table, tip_table.nr + 1, tip_table.alloc); oidcpy(&tip_table.table[tip_table.nr].oid, oid); - tip_table.table[tip_table.nr].refname = xstrdup(refname); + tip_table.table[tip_table.nr].refname = shorten_unambiguous ? refname : + xstrdup(refname); tip_table.table[tip_table.nr].commit = commit; tip_table.table[tip_table.nr].taggerdate = taggerdate; tip_table.table[tip_table.nr].from_tag = from_tag;
When "add_to_tip_table()" is called with a non-zero "shorten_unambiguous" we always return an xstrdup()'d string, which we'd then xstrdup() again, leaking memory. See [1] and [2] for how this leak came about. 1. 98c5c4ad015 (name-rev: allow to specify a subpath for --refs option, 2013-06-18) 2. b23e0b9353e (name-rev: allow converting the exact object name at the tip of a ref, 2013-07-07) Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- builtin/name-rev.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)