Re: [PATCH 4/5] builtin/repack.c: be more conservative with unsigned overflows

Taylor Blau [off-list ref] writes:

There are a number of places in the geometric repack code where we
multiply the number of objects in a pack by another unsigned value. We
trust that the number of objects in a pack is always representable by a
uint32_t, but we don't necessarily trust that that number can be
multiplied without overflow.

Sprinkle some unsigned_add_overflows() and unsigned_mult_overflows() in
split_pack_geometry() to check that we never overflow any unsigned types
when adding or multiplying them.

Arguably these checks are a little too conservative, and certainly they
do not help the readability of this function. But they are serving a
useful purpose, so I think they are worthwhile overall.

Suggested-by: Junio C Hamano <redacted>

I do not deserve this for my idle thinking-out-aloud speculation.
You did all the thinking and work.

I do not know if just dying is a useful thing to do, though.  These
all happen in the "discover where the split point is, to figure out
which packs to coalesce" phase where we haven't touched anything on
disk, so in that sense it is safe to die, but what recourse do users
have after seeing these errors?

For example, in the first overflow check in hunk -388,11, presumably
when total_size for a given set of packfiles does not fit in
int32_t, it won't be fruitful to attempt to coalesce such a set of
packs into one, so perhaps a better fix may be to shift the split
point earlier to find a point that lets us to pack as many as we
could?

I do not offhand know what a sensible and gentler fallback position
would be for the factor multiplication, but presumably, if the right
hand side of this ...

 		if (geometry_pack_weight(ours) < factor * total_size) {

... overflows, we can say it would definitely be larger than our
weight and continue, instead of "no, we cannot multiply total with
factor, as that would give us too big a number", I guess.

I am OK to either (1) leave the code as-is without this patch, because
the overflow would only affect the largest of packs and would be rare,
and people who would notice when they hit it would probably are the ones
with enough resource to diagnose, report and even give us a fix ;-)
or (2) apply this patch as-is, because the only people who will see
the die() would be the same ones affected by (1) above anyway.

And applying this patch as-is would give better chance than (1) for
the overflow to be noticed.

So, let's queue the patch as-is.

Thanks.

On Fri, Mar 05, 2021 at 11:32:57AM -0800, Junio C Hamano wrote:

quoted

Suggested-by: Junio C Hamano <redacted>

I do not deserve this for my idle thinking-out-aloud speculation.
You did all the thinking and work.

You're selling yourself short ;-).

I do not offhand know what a sensible and gentler fallback position
would be for the factor multiplication, but presumably, if the right
hand side of this ...

quoted

 		if (geometry_pack_weight(ours) < factor * total_size) {

... overflows, we can say it would definitely be larger than our
weight and continue, instead of "no, we cannot multiply total with
factor, as that would give us too big a number", I guess.

To answer your question about whether or not dying is sensible, I think
that there are a couple of options. You could say, "I can no longer
multiply total_size by factor without overflowing, so I'm not even going
to bother considering additional packs". I guess that makes some
progress in condensing packs, but that's as good as it would get, since
the subsequent repack would also overflow instead of making further
progress, so it would just get stuck.

Which makes me think that the other option, dying, is more sensible. But
I think that this is all a moot point as you seem to indicate anyway,
because having a large enough pack that we are verging on overflowing is
likely not a real-world problem that we are going to deal with (at least
right away).

I am OK to either (1) leave the code as-is without this patch, because
the overflow would only affect the largest of packs and would be rare,
and people who would notice when they hit it would probably are the ones
with enough resource to diagnose, report and even give us a fix ;-)
or (2) apply this patch as-is, because the only people who will see
the die() would be the same ones affected by (1) above anyway.

And applying this patch as-is would give better chance than (1) for
the overflow to be noticed.

So, let's queue the patch as-is.

Great, I'm fine with that. Thanks.

Thanks.

Taylor