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

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 5, 2021, 19:42 UTC
Message-ID: <xmqqa6si1ety.fsf@gitster.c.googlers.com>
In-Reply-To: <7c209432-82e6-7785-a5c1-1068ec3f0378@gmail.com>

Derrick Stolee <stolee@gmail.com> writes:

Show 21 quoted lines

> On 2/4/2021 7:00 PM, Junio C Hamano wrote:
>> "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.

In other words, if size_t turns out to be too small, doing multiplication in uint64_t would not help at all and add_chunk() API needs its parameter types updated [*].

    side note: I really wish that the language and the compiler
    helped us so that we didn't have to do this---after all, our
    function prototype says the result will be passed as a certain
    type, so it would be nice if the arithmetic to compute that
    result were automatically carried out in a way not to cause
    truncation.

Show 8 quoted lines

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

Hmph, I somehow had an impression that they did not have truncation issue in the original context, but perhaps I was wrong. OK.

Show 6 quoted lines

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