Re: [PATCH 07/20] midx: check size of pack names chunk

From: Taylor Blau <me@ttaylorr.com>
Date: Oct 11, 2023, 14:52 UTC
Message-ID: <ZSa2nbVDTXvFZvSx@nand.local>
In-Reply-To: <20231009210514.GG3282181@coredump.intra.peff.net>

On Mon, Oct 09, 2023 at 05:05:14PM -0400, Jeff King wrote:

Show 8 quoted lines

> @@ -176,9 +176,16 @@ struct multi_pack_index *load_multi_pack_index(const char *object_dir, int local
>
>  	cur_pack_name = (const char *)m->chunk_pack_names;
>  	for (i = 0; i < m->num_packs; i++) {
> +		const char *end;
> +		size_t avail = m->chunk_pack_names_len -
> +				(cur_pack_name - (const char *)m->chunk_pack_names);
> +

This patch all looks good to me, but reading this hunk gave me a little bit of pause. I was wondering what might happen if chunk_pack_names_len was zero, and subtracting some other non-zero size_t from it might cause us to wrap around.

But I think that's a non-issue here, since we'd set cur_pack_name to the beginning of the chunk, and compute avail as 0 - (m->chunk_pack_names - m->chunk_pack_names), and get 0 back, causing our memchr() lookup below to fail, and for us to report this chunk is garbage.

And since cur_pack_name monotonically increases, I think that there is an inductive argument to be made that this subtraction is always safe to do. But it couldn't hurt to do something like:

    size_t read = cur_pack_name - (const char *)m->chunk_pack_names;
    size_t avail = m->chunk_pack_names_len;

    if (read > avail)
        die("...");
    avail -= read;

to make absolutely sure that we would never underflow here.

Thanks, Taylor