On Wed, Oct 20, 2021 at 11:39 PM Taylor Blau <me@xxxxxxxxxxxx> wrote: > 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/ > 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 <me@xxxxxxxxxxxx> > --- > 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).
