| Message ID | patch-1.1-f11eb44e4c5-20210831T134023Z-avarab@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | ccdd5d1eb14a6735c34428e856c0de33f1055520 |
| Headers | show |
| Series | mailmap.c: fix a memory leak in free_mailap_{info,entry}() | expand |
On Tue, Aug 31, 2021 at 9:43 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote: > In the free_mailmap_entry() code added in 0925ce4d49 (Add map_user() > and clear_mailmap() to mailmap, 2009-02-08) the intent was clearly to > clear the "me" structure, but while we freed parts of the > mailmap_entry structure, we didn't free the structure itself. The same > goes for the "mailmap_info" structure. > > This brings us from 50 failures when running t4203-mailmap.sh to > 49. Not really progress as far as the number of failures is concerned, > but as far as I can tell this fixes all leaks in mailmap.c > itself. There's still users of it such as builtin/log.c that call > read_mailmap() without a clear_mailmap(), but that's on them. As a standalone patch, the "50 failures" is confusing and sounds quite alarming. Adding even a tiny bit of context: s/50 failure/50 SANITIZE failures/ would help reduce the confusion. Alternatively, just dropping the second paragraph altogether would clear up any misunderstanding since the first paragraph and the patch body stand well on their own without any additional explanation. > Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
On Tue, Aug 31, 2021 at 03:42:52PM +0200, Ævar Arnfjörð Bjarmason wrote: > In the free_mailmap_entry() code added in 0925ce4d49 (Add map_user() > and clear_mailmap() to mailmap, 2009-02-08) the intent was clearly to > clear the "me" structure, but while we freed parts of the > mailmap_entry structure, we didn't free the structure itself. The same > goes for the "mailmap_info" structure. > > This brings us from 50 failures when running t4203-mailmap.sh to > 49. Not really progress as far as the number of failures is concerned, > but as far as I can tell this fixes all leaks in mailmap.c > itself. There's still users of it such as builtin/log.c that call > read_mailmap() without a clear_mailmap(), but that's on them. > > Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> > --- Thanks, the patch looks good to me. I agree with Eric that mentioning "leak failures" in the second paragraph would make it less confusing. :) -Peff
Jeff King <peff@peff.net> writes: > On Tue, Aug 31, 2021 at 03:42:52PM +0200, Ævar Arnfjörð Bjarmason wrote: > >> In the free_mailmap_entry() code added in 0925ce4d49 (Add map_user() >> and clear_mailmap() to mailmap, 2009-02-08) the intent was clearly to >> clear the "me" structure, but while we freed parts of the >> mailmap_entry structure, we didn't free the structure itself. The same >> goes for the "mailmap_info" structure. >> >> This brings us from 50 failures when running t4203-mailmap.sh to >> 49. Not really progress as far as the number of failures is concerned, >> but as far as I can tell this fixes all leaks in mailmap.c >> itself. There's still users of it such as builtin/log.c that call >> read_mailmap() without a clear_mailmap(), but that's on them. >> >> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> >> --- > > Thanks, the patch looks good to me. I agree with Eric that mentioning > "leak failures" in the second paragraph would make it less confusing. :) Here is what I queued. Thanks, all. From ccdd5d1eb14a6735c34428e856c0de33f1055520 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= <avarab@gmail.com> Date: Tue, 31 Aug 2021 15:42:52 +0200 Subject: [PATCH] mailmap.c: fix a memory leak in free_mailap_{info,entry}() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In the free_mailmap_entry() code added in 0925ce4d49 (Add map_user() and clear_mailmap() to mailmap, 2009-02-08) the intent was clearly to clear the "me" structure, but while we freed parts of the mailmap_entry structure, we didn't free the structure itself. The same goes for the "mailmap_info" structure. This brings the number of SANITIZE=leak failures in t4203-mailmap.sh down from 50 to 49. Not really progress as far as the number of failures is concerned, but as far as I can tell this fixes all leaks in mailmap.c itself. There's still users of it such as builtin/log.c that call read_mailmap() without a clear_mailmap(), but that's on them. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com> --- mailmap.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/mailmap.c b/mailmap.c index d1f7c0d272..e1c8736093 100644 --- a/mailmap.c +++ b/mailmap.c @@ -36,6 +36,7 @@ static void free_mailmap_info(void *p, const char *s) s, debug_str(mi->name), debug_str(mi->email)); free(mi->name); free(mi->email); + free(mi); } static void free_mailmap_entry(void *p, const char *s) @@ -51,6 +52,7 @@ static void free_mailmap_entry(void *p, const char *s) me->namemap.strdup_strings = 1; string_list_clear_func(&me->namemap, free_mailmap_info); + free(me); } /*
diff --git a/mailmap.c b/mailmap.c index 462b3956340..40ce152024d 100644 --- a/mailmap.c +++ b/mailmap.c @@ -37,6 +37,7 @@ static void free_mailmap_info(void *p, const char *s) s, debug_str(mi->name), debug_str(mi->email)); free(mi->name); free(mi->email); + free(mi); } static void free_mailmap_entry(void *p, const char *s) @@ -52,6 +53,7 @@ static void free_mailmap_entry(void *p, const char *s) me->namemap.strdup_strings = 1; string_list_clear_func(&me->namemap, free_mailmap_info); + free(me); } /*