Re: [PATCH v2 15/17] midx: use 64-bit multiplication for chunk sizes

From: Derrick Stolee <stolee@gmail.com>
Date: Feb 5, 2021, 12:30 UTC
Message-ID: <7c209432-82e6-7785-a5c1-1068ec3f0378@gmail.com>
In-Reply-To: <xmqq8s834c4s.fsf@gitster.c.googlers.com>

On 2/4/2021 7:00 PM, Junio C Hamano wrote:

Show 17 quoted lines

> "Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:
> 
>> From: Derrick Stolee <dstolee@microsoft.com>
>>
>> When calculating the sizes of certain chunks, we should use 64-bit
>> multiplication always. This allows us to properly predict the chunk
>> sizes without risk of overflow.
>>
>> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
>> ---
>>  midx.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> This one I find somewhat questionable for multiple reasons.
> 
>  * the fourth parameter of add_chunk() is of size_t, not uint64_t;
>    shouldn't the multiplication be done in type size_t instead?

This is probably appropriate because we will truncate to size_t if it is smaller than uint64_t.

>  * these mutiplications were introduced in "midx: use chunk-format
>    API in write_midx_internal()"; that step should use the
>    arithmetic with cast (if necessary) from the start, no?

I wanted to isolate these changes specifically so we could be careful about the multiplications and not be distracted by them when converting to the chunk-format API. The multiplications were "moved" by that patch, not "introduced".

Show 5 quoted lines

>  * There is "ctx.entries_nr * MIDX_CHUNKID_OFFSET_WIDTH" passed to
>    add_chunk(), in the post-context of the first hunk.  Shouldn't
>    that be covered as well?  I didn't grep for all uses of
>    add_chunk(), but I wouldn't be surprised if this patch missed
>    some of the calls that need the same treatment.

And here is a great example of why it was good to call out these multiplications in their own patch.

I did a full inspection of all multiplications in midx.c and found a few more instances of possible overflow. Two are on the read side, but they require the object lookup chunk to have size 4gb or larger. This is not _that_ far off from possibility! My multi-pack-index for the Windows repository is currently ~1.6 GB (in total, including the other chunks).

Thanks, -Stolee