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

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Oct 21, 2021, 05:00 UTC
Message-ID: <CAPig+cSnrrT_wELSLbk=Shfu_e_6YM9LpMw2X75-oN0QqOAY3Q@mail.gmail.com>
In-Reply-To: <b0c79904ab7bdaee7a1bc7a55b0fb26b1f2cf8d3.1634787555.git.me@ttaylorr.com>

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/

Show 19 quoted lines

> 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).