diff mbox series

t0006: date_mode can leak .strftime_fmt member

Message ID patch-1.1-15f5bd3e4f4-20211116T183025Z-avarab@gmail.com (mailing list archive)
State New, archived
Headers show
Series t0006: date_mode can leak .strftime_fmt member | expand

Commit Message

Ævar Arnfjörð Bjarmason Nov. 16, 2021, 6:31 p.m. UTC 
From: Junio C Hamano <gitster@pobox.com>

As there is no date_mode_release() API function, and given the
set of current callers it probably is not worth adding one, let's
release the .strftime_fmt member that is obtained from strdup()
before the caller of show_date() is done with it.

This allows us to mark t0006 as passing under the leak sanitizer.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---

A trivial leak test from Junio that fell between the cracks. Submitted
with my suggested fix-up in
https://lore.kernel.org/git/211104.86mtmki5ol.gmgdl@evledraar.gmail.com/

 t/helper/test-date.c | 4 +++-
 t/t0006-date.sh      | 2 ++
 2 files changed, 5 insertions(+), 1 deletion(-)

Comments

Junio C Hamano Nov. 16, 2021, 7:04 p.m. UTC | #1 
Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> As there is no date_mode_release() API function, and given the
> set of current callers it probably is not worth adding one, let's
> release the .strftime_fmt member that is obtained from strdup()
> before the caller of show_date() is done with it.

I do not know what the last line exactly wants to say.  Perhaps the
original author meant "after", not "before"? ;-)
Jeff King Nov. 16, 2021, 7:31 p.m. UTC | #2 
On Tue, Nov 16, 2021 at 07:31:12PM +0100, Ævar Arnfjörð Bjarmason wrote:

> As there is no date_mode_release() API function, and given the
> set of current callers it probably is not worth adding one, let's
> release the .strftime_fmt member that is obtained from strdup()
> before the caller of show_date() is done with it.

It does feel a bit ugly to assume that we can touch strftime_fmt here,
especially since we don't even confirm that we parsed DATE_STRFTIME.
You initialize it as NULL and the current code doesn't touch it
otherwise, so there's no bug. But it would be reasonable for other date
formats to store ancillary data as a union with strftime_fmt, which
would invalidate this.

It also seems like other callers will need to do similar cleanup. E.g.,
"git -c log.date=format:foo log" has the same leak. So maybe it is worth
adding an actual cleanup function.

-Peff
diff mbox series

Patch

diff --git a/t/helper/test-date.c b/t/helper/test-date.c
index 099eff4f0fc..27a36a5c5fe 100644
--- a/t/helper/test-date.c
+++ b/t/helper/test-date.c
@@ -34,7 +34,7 @@  static void show_human_dates(const char **argv)
 
 static void show_dates(const char **argv, const char *format)
 {
-	struct date_mode mode;
+	struct date_mode mode = { 0 };
 
 	parse_date_format(format, &mode);
 	for (; *argv; argv++) {
@@ -53,6 +53,8 @@  static void show_dates(const char **argv, const char *format)
 
 		printf("%s -> %s\n", *argv, show_date(t, tz, &mode));
 	}
+
+	free((void *)mode.strftime_fmt);
 }
 
 static void parse_dates(const char **argv)
diff --git a/t/t0006-date.sh b/t/t0006-date.sh
index 6b757d71692..5d01f57b270 100755
--- a/t/t0006-date.sh
+++ b/t/t0006-date.sh
@@ -1,6 +1,8 @@ 
 #!/bin/sh
 
 test_description='test date parsing and printing'
+
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 # arbitrary reference time: 2009-08-30 19:20:00