On Mon, Mar 20, 2023 at 04:02:55PM -0400, Taylor Blau wrote:
> 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...
> +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
