Re: [PATCH 5/6] pack-bitmap.c: use `bitmap_index_seek()` where possible

From: Jeff King <peff@peff.net>
Date: Mar 21, 2023, 18:05 UTC
Message-ID: <20230321180525.GG3119834@coredump.intra.peff.net>
In-Reply-To: <9a3e45b78b7810e0116848f1de80096b04285a55.1679342296.git.me@ttaylorr.com>

On Mon, Mar 20, 2023 at 04:02:52PM -0400, Taylor Blau wrote:

Show 9 quoted lines

> --- 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.

Show 8 quoted lines

> @@ -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).

Show 8 quoted lines

> @@ -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