Re: [PATCH] test-mergesort: plug memory leaks in sort_stdin()
- From
Patrick Steinhardt <ps@pks.im>
- Date
- Oct 7, 2026, 06:13 UTC
- Message-ID
- <asXi-1RlWhqPMWjL@pks.im>
- In-Reply-To
- <20261007034205.32619-1-dilsheddilu123@gmail.com>
On Wed, Oct 07, 2026 at 09:12:05AM +0530, Muhammed Dilshad A wrote:
> The sort_stdin() helper allocates an input buffer and a memory pool for > the list of lines, but returns without releasing either. Discard the > pool and release the strbuf after printing the sorted lines.
Makes sense.
> Add a test for the sort subcommand to t0071. The existing test only > exercises the test subcommand, leaving these leaks undetected by the > regular leak-sanitized test suite.
I was briefly wondering whether we could get rid of t0071 altogether in favor of converting the tests into a unit test, and then drop the test helper. And that's certainly doable, and I'd argue it would also be the right thing to do. But unfortunately it wouldn't allow us to get rid of the test helper completely as the "mergesort sort" subcommand is used as part of our performance tests.
I would claim that the benchmark itself is of dubious value. It was nice enough to have some numbers when we were working on the implementation of the mergesort, but carrying it with us nowadays feels like a bit of a waste as chances for regression are somewhat slim here. And if we ever wanted to iterate further on the merge sort implementation we could still introduce a new benchmark, that's easy enough to do.
But anyway, that's of course a much bigger scope, and I'm fine to just fix the bugs for now.
Show 12 quoted lines
> diff --git a/t/helper/test-mergesort.c b/t/helper/test-mergesort.c > index 791e128793..3b8c428b14 100644 > --- a/t/helper/test-mergesort.c > +++ b/t/helper/test-mergesort.c > @@ -61,6 +61,8 @@ static int sort_stdin(void) > puts(lines->text); > lines = lines->next; > } > + mem_pool_discard(&lines_pool, 0); > + strbuf_release(&sb); > return 0; > }
The fix is obviously correct.
Show 14 quoted lines
> diff --git a/t/t0071-sort.sh b/t/t0071-sort.sh > index 2236a7e956..97890da29f 100755 > --- a/t/t0071-sort.sh > +++ b/t/t0071-sort.sh > @@ -8,4 +8,11 @@ test_expect_success 'DEFINE_LIST_SORT_DEBUG' ' > test-tool mergesort test > ' > > +test_expect_success 'sort stdin' ' > + printf "%s\n" c a b >input && > + printf "%s\n" a b c >expect && > + test-tool mergesort sort <input >actual && > + test_cmp expect actual > +'
And having a test makes sense, I guess.
I noticed that there's another "generate" subcommand here that is entirely unused. Do we maybe want to also remove it while at it? The test suite passes with the below diff.
Thanks!
Patrick
diff --git a/t/helper/test-mergesort.c b/t/helper/test-mergesort.c index 791e128793..9200c4bb4a 100644 --- a/t/helper/test-mergesort.c +++ b/t/helper/test-mergesort.c @@ -114,16 +114,6 @@ static struct dist { DIST(shuffle), }; -static const struct dist *get_dist_by_name(const char *name) -{ - int i; - for (i = 0; i < ARRAY_SIZE(dist); i++) { - if (!strcmp(dist[i].name, name)) - return &dist[i]; - } - return NULL; -} - static void mode_copy(int *arr UNUSED, int n UNUSED) { /* nothing */ @@ -237,41 +227,6 @@ static struct mode { MODE(unriffle_skewed), }; -static const struct mode *get_mode_by_name(const char *name) -{ - int i; - for (i = 0; i < ARRAY_SIZE(mode); i++) { - if (!strcmp(mode[i].name, name)) - return &mode[i]; - } - return NULL; -} - -static int generate(int argc, const char **argv) -{ - const struct dist *dist = NULL; - const struct mode *mode = NULL; - int i, n, m, *arr; - - if (argc != 4) - return 1; - - dist = get_dist_by_name(argv[0]); - mode = get_mode_by_name(argv[1]); - n = strtol(argv[2], NULL, 10); - m = strtol(argv[3], NULL, 10); - if (!dist || !mode) - return 1; - - ALLOC_ARRAY(arr, n); - dist->fn(arr, n, m); - mode->fn(arr, n); - for (i = 0; i < n; i++) - printf("%08x\n", arr[i]); - free(arr); - return 0; -} - static struct stats { int get_next, set_next, compare; } stats; @@ -388,14 +343,11 @@ int cmd__mergesort(int argc, const char **argv) int i; const char *sep; - if (argc == 6 && !strcmp(argv[1], "generate")) - return generate(argc - 2, argv + 2); if (argc == 2 && !strcmp(argv[1], "sort")) return sort_stdin(); if (argc > 1 && !strcmp(argv[1], "test")) return run_tests(argc - 2, argv + 2); - fprintf(stderr, "usage: test-tool mergesort generate <distribution> <mode> <n> <m>\n"); - fprintf(stderr, " or: test-tool mergesort sort\n"); + fprintf(stderr, "usage: test-tool mergesort sort\n"); fprintf(stderr, " or: test-tool mergesort test [<n>...]\n"); fprintf(stderr, "\n"); for (i = 0, sep = "distributions: "; i < ARRAY_SIZE(dist); i++, sep = ", ")