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

[Derrick Stolee <derrickstolee@github.com>]

On 3/21/2023 2:05 PM, Jeff King wrote:
> 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.

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

Thanks,
-Stolee