From: rsbecker@nexbridge.com
Date: Thu, 22 Jan 2026 02:26:06 GMT
Subject: RE: [PATCH v2 11/18] git-compat-util.h: introduce `u32_add()`
Message-ID: <014c01dc8b46$7a2997b0$6e7cc710$@nexbridge.com>
In-Reply-To: <aXFni2tE7vn1dKFp@nand.local>

On Taylor Blau January 21, 2026 6:56 PM writes:
>On Wed, Jan 21, 2026 at 09:51:36AM +0100, Patrick Steinhardt wrote:
>> > diff --git a/midx-write.c b/midx-write.c index
>> > 87b97c70872..6006b6569c8 100644
>> > --- a/midx-write.c
>> > +++ b/midx-write.c
>> > @@ -1738,8 +1738,19 @@ static void fill_included_packs_batch(struct
>repository *r,
>> >  		 */
>> >  		expected_size = (uint64_t)pack_info[i].referenced_objects << 14;
>> >  		expected_size /= p->num_objects;
>> > -		expected_size = u64_mult(expected_size, p->pack_size);
>> > -		expected_size = u64_add(expected_size, 1u << 13) >> 14;
>> > +
>> > +		if (unsigned_mult_overflows(expected_size,
>> > +					    (uint64_t)p->pack_size))
>> > +			die(_("overflow during fixed-point multiply (%"PRIu64" "
>> > +			      "* %"PRIu64")"),
>> > +			    expected_size, (uint64_t)p->pack_size);
>> > +		expected_size = expected_size * p->pack_size;
>> > +
>> > +		if (unsigned_add_overflows(expected_size, 1u << 13))
>> > +			die(_("overflow during fixed-point rounding (%"PRIu64" "
>> > +			      " + %"PRIu64")"),
>> > +			    expected_size, (uint64_t)(1ul << 13));
>> > +		expected_size = (expected_size + (1u << 13)) >> 14;
>>
>> One downside this pattern has is that we repeat the computation, which
>> makes it easy to get it wrong or forget updating either the check or
>> the computation.
>>
>> I think ideally, we would have interfaces that combine the two
>> approaches in `u64_mult()` and `unsigned_mult_overflows()`. Something
>> like this for example:
>>
>>     static intline bool u64_mult(uint64_t a, uint64_t b, uint64_t *out)
>>     {
>>         if (unsigned_mult_overflows(a, b))
>>             return false;
>>         *out = a * b;
>>         return true;
>>     }
>
>I had considered this approach when writing, but ultimately decided against it, since
>it felt a little clunky to have to pass a pointer in to do a simple arithmetic operation.
>But I think your point about ensuring that we actually do:
>
>    if (unsigned_mult_overflows(a, b))
>      die(...);
>    result = a * b;
>
>and not "result = a * c" or some other expression which is not "a * b"
>is a good one.
>
>I dunno. The spots in this patch are the only uses of u64_mult() and u64_add(), so
>I'm hesitant to keep a helper function around just for that sole use-case. I wonder if
>we should do what you suggest here for the much more frequently used st_add() /
>st_mult() / st_sub() functions?
>
>> This would let the caller handle the failure and is thus quite
>> flexible, which results in the following code:
>>
>> 	if (!u64_mult(expected_size, (uint64_t)p->pack_size, &expected_size))
>> 		die(_("overflow during fixed-point multiply (%"PRIu64" "
>> 		      "* %"PRIu64")"), expected_size, (uint64_t)p->pack_size);
>
>It does read quite cleanly, so I think I'm convinced.

Are all of these changes endian-safe?