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

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 21, 2021, 16:27 UTC
Message-ID: <xmqqr1cemjhl.fsf@gitster.g>
In-Reply-To: <b0c79904ab7bdaee7a1bc7a55b0fb26b1f2cf8d3.1634787555.git.me@ttaylorr.com>

Taylor Blau <me@ttaylorr.com> writes:

Show 6 quoted lines

> 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.
>
> Call the more appropriate close_midx() which does free those resources,
> which causes t5319.3 to pass when Git is compiled with SANITIZE=leak.

Nice.

By the way, the function starts like so:

    int verify_midx_file(struct repository *r, const char *object_dir, unsigned flags)
    {
            struct pair_pos_vs_id *pairs = NULL;
            uint32_t i;
            struct progress *progress = NULL;
            struct multi_pack_index *m = load_multi_pack_index(object_dir, 1);
            verify_midx_error = 0;

            if (!m) {
                    int result = 0;
                    struct stat sb;
                    char *filename = get_midx_filename(object_dir);
                    if (!stat(filename, &sb)) {
                            error(_("multi-pack-index file exists, but failed to parse"));
                            result = 1;
                    }
                    free(filename);
                    return result;
            }

but after seeing die() sprinkled in load_multi_pack_index() with checks during parsing, I am not sure if this is a good error reporting mechanism we are seeing here.

It is wonderful to plug leaks here and there, but to be honest, even with only a very little parts I saw in this code, I think there are other things that need clean-up here.

Also, the way the file-scope global verify_midx_error is used is beyond words _ugly_, if the only reason for its use is to make midx_report() look simpler, which is what I think is happening.

Not very impressed, but all of the above is not a new issue introduced by this patch.

Thanks.