On Mon, Mar 20, 2023 at 04:02:52PM -0400, Taylor Blau wrote:
> --- a/pack-bitmap.c
> +++ b/pack-bitmap.c
> @@ -174,7 +174,7 @@ static struct ewah_bitmap *read_bitmap_1(struct bitmap_index *index)
> return NULL;
> }
>
> - index->map_pos += bitmap_size;
> + bitmap_index_seek(index, bitmap_size, SEEK_CUR);
> return b;
As an aside, I notice none of the callers here or in the next patch
check the return value of bitmap_index_seek(). I guess you included it
to match the return value of lseek(), but maybe it is better to return
void if nobody is looking at it.
But getting back to the bounds-checking: I think we are already
correctly bounds-checked here, because ewah_read_mmap() will make sure
that it doesn't read too far (and will return an error if there's
truncation). And if it didn't, this check-on-seek doesn't help us,
because we'll already have done out-of-bounds reads.
> @@ -230,7 +230,7 @@ static int load_bitmap_header(struct bitmap_index *index)
>
> index->entry_count = ntohl(header->entry_count);
> index->checksum = header->checksum;
> - index->map_pos += header_size;
> + 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).
> @@ -269,13 +269,15 @@ static struct stored_bitmap *store_bitmap(struct bitmap_index *index,
> static uint32_t read_be32(struct bitmap_index *bitmap_git)
> {
> uint32_t result = get_be32(bitmap_git->map + bitmap_git->map_pos);
> - bitmap_git->map_pos += sizeof(result);
> + bitmap_index_seek(bitmap_git, sizeof(uint32_t), SEEK_CUR);
> return result;
> }
The function doesn't do bounds-checks itself, but the callers do, like:
if (index->map_size - index->map_pos < 6)
return error(_("corrupt ewah bitmap: truncated header for entry %d"), i);
commit_idx_pos = read_be32(index->map, &index->map_pos);
xor_offset = read_u8(index->map, &index->map_pos);
flags = read_u8(index->map, &index->map_pos);
(and again, I'd be happy to see this magic "6" go away in favor of
checking as we read each item).
Maybe I'm misunderstanding the purpose of your series. I thought it was
to avoid reading out of bounds. But since bitmap_index_seek() triggers a
BUG(), it's not good for detecting truncated files, etc. So is it really
just meant to be a belt-and-suspenders check on the existing
bounds-checks? I guess that makes more sense with the way it is written,
but I'm just a little skeptical that it's really useful.
-Peff
