Re: [PATCH 2/2] pack-bitmap.c: ensure pseudo-merge offset reads are bounded

From: Taylor Blau <me@ttaylorr.com>
Date: Jun 6, 2024, 22:25 UTC
Message-ID: <ZmI3RYSsx6VRHdc9@nand.local>
In-Reply-To: <xmqqfrtpluws.fsf@gitster.g>

On Thu, Jun 06, 2024 at 12:42:59PM -0700, Junio C Hamano wrote:

> Can table_size at this point be smaller than 24, which will allow
> (table_size - 24) to be a huge number that st_mult() will
> comfortably fit?

It could be smaller than 24, but I think we're at the point of diminishing returns here. The table_size field is read from the .bitmap file itself, and we do some light bounds checking here:

    table_size = get_be64(index_end - 8);
    if (table_size > index_end - index->map - header_size)
        return error(_(...));

We could add another check to ensure that table_size is at least 24, but I'm less concerned here for a couple of reasons:

  - Since we're reading off of the index_end, we know that all of our
    reads are within the .bitmap itself, so we're not reading outside of
    the memory-mapped region.

  - Checking that index->pseudo_merges.nr is a reasonable size also
    bounds reads, but more importantly IMHO prevents a large heap
    allocation via the CALLOC_ARRAY() below.

So I think we could check the table_size value, but I'm not sure we'd gain very much by doing so.

Thanks, Taylor