Re: [PATCH 02/20] packfile.c: prevent overflow in `load_idx()`

> On Thu, Jul 13, 2023 at 09:21:55AM +0100, Phillip Wood wrote:
>>> diff --git a/packfile.c b/packfile.c
>>> index 89220f0e03..70acf1694b 100644
>>> --- a/packfile.c
>>> +++ b/packfile.c
>>> @@ -186,7 +186,7 @@ int load_idx(const char *path, const unsigned int hashsz, void *idx_map,
>>>    		     */
>>>    		    (sizeof(off_t) <= 4))
>>>    			return error("pack too large for current definition of off_t in %s", path);
>>> -		p->crc_offset = 8 + 4 * 256 + nr * hashsz;
>>> +		p->crc_offset = st_add(8 + 4 * 256, st_mult(nr, hashsz));
>>
>> p->crc_offset is a uint32_t so we're still prone to truncation here unless
>> we change the crc_offset member of struct packed_git to be a size_t. I
>> haven't checked if the other users of crc_offset would need adjusting if we
>> change its type.
> 
> Thanks for spotting. Luckily, this should be a straightforward change:
> 
>      $ git grep crc_offset
>      builtin/index-pack.c:	idx1 = (((const uint32_t *)((const uint8_t *)p->index_data + p->crc_offset))
>      object-store-ll.h:	uint32_t crc_offset;
>      packfile.c:		p->crc_offset = st_add(8 + 4 * 256, st_mult(nr, hashsz));
> 
> The single usage in index-pack is OK, so we only need to change its type
> to a size_t.

> I could see an argument that this should be an off_t, since it is an
> offset into a file. But since we memory map the whole thing anyway, I
> think we are equally OK to treat it as a pointer offset. A similar
> argument is made in f86f769550e (compute pack .idx byte offsets using
> size_t, 2020-11-13), so I am content to leave this as a size_t.