Re: Re* [PATCH v8 2/2] tests: add a test mode for SANITIZE=leak, run it in CI
From: Ævar Arnfjörð Bjarmason <hidden>
Date: 2021-11-04 10:19:42
Subsystem:
the rest · Maintainer:
Linus Torvalds
On Wed, Nov 03 2021, Junio C Hamano wrote:
Ævar Arnfjörð Bjarmason [off-list ref] writes:quoted
The CI target uses a new GIT_TEST_PASSING_SANITIZE_LEAK=true test mode. When running in that mode, we'll assert that we were compiled with SANITIZE=leak. We'll then skip all tests, except those that we've opted-in by setting "TEST_PASSES_SANITIZE_LEAK=true". ... This is how tests that don't set "TEST_PASSES_SANITIZE_LEAK=true" will be skipped under GIT_TEST_PASSING_SANITIZE_LEAK=true:I've been playing with this locally, but cannot shake the nagging feeling that GIT_TEST_PASSING_SANITIZE_LEAK must default to true. Otherwise, it is one more thing they need to find out and set when they do make SANITYZE=leak test because they want to be a good developer and to ensure that they did not introduce new leaks. If we want to encourage folks to locally run the leak checks before declaring their own work "done", that is. Those who are hunting for and cleaning up existing leaks can and should set it to false, no?
I agree that that would make a lot more sense and be more useful :) That was the behavior of the patch I originally suggested for integrating this SANITIZE=leak[1], but due to feedback on it I ended up keeping the pre-image behavior of how SANITIZE=leak worked, unless there were any opt-in test modes etc. in play: https://lore.kernel.org/git/patch-1.4-a61a294132-20210714T001007Z-avarab@gmail.com/ (local) I think at this point it's probably better to just keep it as it is...
quoted hunk ↗ jump to hunk
in any case, here is a small fallout out of my adventure into this corner. ----- >8 --------- >8 --------- >8 --------- >8 ----- Subject: t0006: date_mode can leak .strftime_fmt member 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 <redacted> --- t/helper/test-date.c | 2 ++ t/t0006-date.sh | 2 ++ 2 files changed, 4 insertions(+)diff --git c/t/helper/test-date.c w/t/helper/test-date.c index 099eff4f0f..e15ea02626 100644 --- c/t/helper/test-date.c +++ w/t/helper/test-date.c@@ -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); }
I'd notice that failure before, but hadn't looked into it. That was easier to fix than I thought. This fix looks good to me, except that you also need to change this at the top:
diff --git a/t/helper/test-date.c b/t/helper/test-date.c
index e15ea026267..9defeb57360 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++) {
I.e. this makes this specific thing pass, but in other tests we'd end up freeing a non-NULL and randomly initialized pointer unless we init it to zero.
quoted hunk ↗ jump to hunk
static void parse_dates(const char **argv)diff --git c/t/t0006-date.sh w/t/t0006-date.sh index 6b757d7169..5d01f57b27 100755 --- c/t/t0006-date.sh +++ w/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
And yeah, that's all that's needed in the test file then.