Re: [PATCH 07/20] midx: check size of pack names chunk
- From
Jeff King <peff@peff.net>
- Date
- Oct 11, 2023, 23:06 UTC
- Message-ID
- <20231011230641.GI518221@coredump.intra.peff.net>
- In-Reply-To
- <ZSa2nbVDTXvFZvSx@nand.local>
On Wed, Oct 11, 2023 at 10:52:13AM -0400, Taylor Blau wrote:
Show 19 quoted lines
> On Mon, Oct 09, 2023 at 05:05:14PM -0400, Jeff King wrote:
> > @@ -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.Right. If it is 0, then we should have 0 avail here in the first loop.
I was more worried while writing this that:
cur_pack_name = end + 1;
later in the loop could get us an off-by-one. But we know we are always pointing to one past an available NUL there, so at most our subtraction will equal m->chunk_pack_names_len.
Show 12 quoted lines
> 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.You end up with two die() calls, then. One for "hey, we somehow read too far", and one for "hey, I ran out of data while reading the entry". And the first one cannot be triggered.
IOW, I think your die() here is a BUG().
-Peff