Re: [PATCH] mailmap.c: fix a memory leak in free_mailap_{info,entry}()

From: Junio C Hamano <gitster@pobox.com>
To: Jeff King <peff@peff.net>
Cc: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>,
	git@vger.kernel.org,
	"Marius Storm-Olsen" <marius@storm-olsen.com>
Subject: Re: [PATCH] mailmap.c: fix a memory leak in free_mailap_{info,entry}()
Date: Tue, 31 Aug 2021 12:46:10 -0700	[thread overview]
Message-ID: <xmqq8s0htn25.fsf@gitster.g> (raw)
In-Reply-To: <YS6FKEApva30sKgl@coredump.intra.peff.net> (Jeff King's message of "Tue, 31 Aug 2021 15:38:16 -0400")

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);
 }
 
 /*
-- 
2.33.0-323-g897a01baa9

