Show 66 quoted lines
>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.rsbecker@nexbridge.com <rsbecker@nexbridge.com>