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

[Junio C Hamano <gitster@pobox.com>]

Eric Sunshine <sunshine@sunshineco.com> writes:

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