On Fri, Mar 24, 2023 at 02:06:43PM -0400, Derrick Stolee wrote:

quoted

quoted

+	bitmap_index_seek(index, header_size, SEEK_CUR);
 	return 0;
 }

Likewise this function already has bounds checks at the top:

	if (index->map_size < header_size + the_hash_algo->rawsz)
		return error(_("corrupted bitmap index (too small)"));

I'd be perfectly happy if we swapped that our for checking the bounds on
individual reads, but the extra checking in the seek step here just
seems redundant (and again, too late).

I think it would be nice to replace all of these custom bounds
checks with a check within bitmap_index_seek() and error conditions
done in response to an error code returned by that method. It keeps
the code more consistent in the potential future of changing the
amount to move the map_pos and the amount checked in these conditions.

Yeah, that's what I was getting at. But doing it at seek time is too
late. We'll have just read off the end of the array.

You really need an interface more like "make sure there are N bytes for
me to read at offset X". Then you can read and advance past them.

For individual items where you want to copy the bytes out anyway (like a
be32) you can have a nice interface like:

  if (read_be32(bitmap_git, &commit_idx_pos) < 0 ||
      read_u8(bitmap_git, &xor_offset) < 0 ||
      read_u8(bitmap_git, &flags) < 0)
	return error("truncated bitmap entry");

But given that there is only one spot that calls these, that kind of
refactoring might not be worth it (right now it just uses the magic
number "6" right before grabbing the data).

-Peff