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

From: Eric Sunshine <sunshine@sunshineco.com>
To: Taylor Blau <me@ttaylorr.com>
Cc: "Git List" <git@vger.kernel.org>,
	"Ævar Arnfjörð Bjarmason" <avarab@gmail.com>,
	"Derrick Stolee" <dstolee@microsoft.com>,
	"Jeff King" <peff@peff.net>
Subject: Re: [PATCH 02/11] midx.c: don't leak MIDX from verify_midx_file
Date: Thu, 21 Oct 2021 01:00:45 -0400	[thread overview]
Message-ID: <CAPig+cSnrrT_wELSLbk=Shfu_e_6YM9LpMw2X75-oN0QqOAY3Q@mail.gmail.com> (raw)
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/

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