Re: [PATCH 02/11] midx.c: don't leak MIDX from verify_midx_file

Eric Sunshine [off-list ref] writes:

On Wed, Oct 20, 2021 at 11:39 PM Taylor Blau [off-list ref] wrote:

quoted

The function midx.c:verify_midx_file() allocate a MIDX struct by calling
load_multi_pack_index(). But when cleaning up, it calls free() without
freeing any resources associated with the MIDX.

s/allocate/allocates/

quoted

Call the more appropriate close_midx() which does free those resources,
which causes t5319.3 to pass when Git is compiled with SANITIZE=leak.

Signed-off-by: Taylor Blau <redacted>
---

diff --git a/midx.c b/midx.c

@@ -1611,7 +1611,7 @@ int verify_midx_file(struct repository *r, const char *object_dir, unsigned flag
-               return verify_midx_error;
+               goto cleanup;
        }

@@ -1689,7 +1689,9 @@ int verify_midx_file(struct repository *r, const char *object_dir, unsigned flag
        stop_progress(&progress);

+cleanup:
        free(pairs);
+       close_midx(m);

        return verify_midx_error;
 }

Using the `goto` idiom to ensure cleanup makes perfect sense. For a
few reasons[*], I did spend a some moments wondering if the cognitive
load might be a bit lower by instead adding two close_midx() calls --
one at the early return and one just before the normal return --
rather than using a `goto`, but it's so subjective that it's not worth
worrying about.

FOOTNOTES

[*] First, unlike most cases in which the `goto` jumps over relatively
short blocks of code, the distance in this case between `goto` and the
new label is significant and it's not easy to see at a glance what is
being skipped and how important it might be. Second, `pairs` is still
NULL at the point of the `goto`; I spent extra time checking and
double-checking what free(pairs) was doing in this instance. Finally,
there are enough start/stop-progress calls in this function that it
requires extra mental effort to determine that this `goto` is indeed
safe (at least for the present).

Carefully reading the patch is very good, but at least in a patch
like this, it is immediately obvious that the patch is not making
any of the things the footnote worries about any worse than the
original, by replacing "return" (which by definition will skip all
the things the goto did jump over) with a "goto" to the bottom, no?

Thanks.