diff.c: fix a double-free regression in a18d66cefb

My a18d66cefb9 (diff.c: free "buf" in diff_words_flush(), 2022-03-04) has what it retrospect is a rather obvious bug (I don't know what I was thinking, if it all): We use the "emitted_symbols" allocation in append_emitted_diff_symbol() N times, but starting with a18d66cefb9 we'd free it after its first use! The correct way to free this data would have been to add the free() to the existing free_diff_words_data() function, so let's do that. The "ecbdata->diff_words->opt->emitted_symbols" might be NULL, so let's add a trivial free_emitted_diff_symbols() helper next to the function that appends to it. This fixes the "no effect on show from" leak tested for in the preceding commit. Perhaps confusingly this change will skip that test under SANITIZE=leak, but otherwise opt-in the "t4015-diff-whitespace.sh" test. The reason is that a18d66cefb9 "fixed" the leak in the preceding "no effect on diff" test, but for the first call to diff_words_flush() the "wol->buf" would be NULL, so we wouldn't double-free (and SANITIZE=address would see nothing amiss). With this change we'll still pass that test, showing that we've also fixed leaks on this codepath. We then have to skip the new "no effect on show" test because it happens to trip over an unrelated memory leak (in revision.c). The same goes for "move detection with submodules". Both of them pass with SANITIZE=address though, which would error on the "no effect on show" test before this change. Reported-by: Michael J Gruber <git@grubix.eu> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Ævar Arnfjörð Bjarmason committed Mar 17, 2022 at 15:55 UTC 77e56d55ba6eee1c6efe08d4872e5001ed8e563a
2 files changed +13 -4
diff.c
+9 -2
@@ -800,6 +800,14 @@ static void append_emitted_diff_symbol(struct diff_options *o,
800 f->line = e->line ? xmemdupz(e->line, e->len) : NULL;
801 }
802
803 +static void free_emitted_diff_symbols(struct emitted_diff_symbols *e)
804 +{
805 + if (!e)
806 + return;
807 + free(e->buf);
808 + free(e);
809 +}
810 +
811 struct moved_entry {
812 const struct emitted_diff_symbol *es;
813 struct moved_entry *next_line;
@@ -2150,7 +2158,6 @@ static void diff_words_flush(struct emit_callback *ecbdata)
2158
2159 for (i = 0; i < wol->nr; i++)
2160 free((void *)wol->buf[i].line);
2153 - free(wol->buf);
2161
2162 wol->nr = 0;
2163 }
@@ -2228,7 +2235,7 @@ static void free_diff_words_data(struct emit_callback *ecbdata)
2235 {
2236 if (ecbdata->diff_words) {
2237 diff_words_flush(ecbdata);
2231 - free (ecbdata->diff_words->opt->emitted_symbols);
2238 + free_emitted_diff_symbols(ecbdata->diff_words->opt->emitted_symbols);
2239 free (ecbdata->diff_words->opt);
2240 free (ecbdata->diff_words->minus.text.ptr);
2241 free (ecbdata->diff_words->minus.orig);
t/t4015-diff-whitespace.sh
+4 -2
@@ -6,6 +6,8 @@
6 test_description='Test special whitespace in diff engine.
7
8 '
9 +
10 +TEST_PASSES_SANITIZE_LEAK=true
11 . ./test-lib.sh
12 . "$TEST_DIRECTORY"/lib-diff.sh
13
@@ -1636,7 +1638,7 @@ test_expect_success 'no effect on diff from --color-moved with --word-diff' '
1638 test_cmp expect actual
1639 '
1640
1639 -test_expect_failure 'no effect on show from --color-moved with --word-diff' '
1641 +test_expect_success !SANITIZE_LEAK 'no effect on show from --color-moved with --word-diff' '
1642 git show --color-moved --word-diff >actual &&
1643 git show --word-diff >expect &&
1644 test_cmp expect actual
@@ -2022,7 +2024,7 @@ test_expect_success '--color-moved rewinds for MIN_ALNUM_COUNT' '
2024 test_cmp expected actual
2025 '
2026
2025 -test_expect_success 'move detection with submodules' '
2027 +test_expect_success !SANITIZE_LEAK 'move detection with submodules' '
2028 test_create_repo bananas &&
2029 echo ripe >bananas/recipe &&
2030 git -C bananas add recipe &&