Re: [PATCH 6/6] pack-bitmap.c: factor out `bitmap_index_seek_commit()`
- From
Jeff King <peff@peff.net>
- Date
- Mar 21, 2023, 18:13 UTC
- Message-ID
- <20230321181315.GH3119834@coredump.intra.peff.net>
- In-Reply-To
- <82049ed09e1695db644d1d4cf17557214e54dcea.1679342296.git.me@ttaylorr.com>
On Mon, Mar 20, 2023 at 04:02:55PM -0400, Taylor Blau wrote:
Show 7 quoted lines
> Factor out a common pattern within `lazy_bitmap_for_commit()` where we > seek to a given position (expecting to read the start of an individual > bitmap entry). > > Both spots within `lazy_bitmap_for_commit()` emit a common error, so > factor out the whole routine into its own function to DRY things up a > little.
OK, so this case makes more sense to me as a bounds-check, because we are seeking to an arbitrary offset.
But...
Show 14 quoted lines
> +static int bitmap_index_seek_commit(struct bitmap_index *bitmap_git,
> + struct object_id *oid,
> + size_t pos)
> +{
> + const int bitmap_header_size = 6;
> +
> + bitmap_index_seek(bitmap_git, pos, SEEK_SET);
> +
> + if (bitmap_git->map_size - bitmap_git->map_pos < bitmap_header_size)
> + return error(_("corrupt ewah bitmap: truncated header for "
> + "bitmap of commit \"%s\""),
> + oid_to_hex(oid));
> + return 0;
> +}So here we seek to the position, and then make sure we have 6 bytes to read, which makes sense. But in the seek step, are we worried that we will seek to somewhere bogus? If so, we'll get a BUG(). But is that the right thing if somebody feeds a bogus "pos" that moves past truncation?
I'm not 100% sure on where these offsets come from. But it looks like they're coming from the bitmap lookup table. In which case a bogus value there should be an error(), and not a BUG(), I would think.
-Peff