Re: [PATCH 1/4] midx repack: avoid integer overflow on 32 bit systems

[Taylor Blau <me@xxxxxxxxxxxx>]

On Tue, May 20, 2025 at 04:04:24PM +0100, Phillip Wood wrote:
> diff --git a/midx-write.c b/midx-write.c
> index dd3b3070e55..c7cb2315431 100644
> --- a/midx-write.c
> +++ b/midx-write.c
> @@ -1699,19 +1699,23 @@ static void fill_included_packs_batch(struct repository *r,
>  	for (i = 0; total_size < batch_size && i < m->num_packs; i++) {
>  		int pack_int_id = pack_info[i].pack_int_id;
>  		struct packed_git *p = m->packs[pack_int_id];
> -		size_t expected_size;
> +		uint64_t expected_size;
>
>  		if (!want_included_pack(r, m, pack_kept_objects, pack_int_id))
>  			continue;
>
> -		expected_size = st_mult(p->pack_size,
> -					pack_info[i].referenced_objects);
> +		expected_size = uint64_mult(p->pack_size,
> +					    pack_info[i].referenced_objects);

Makes sense.

>  		expected_size /= p->num_objects;
>
>  		if (expected_size >= batch_size)
>  			continue;
>
> -		total_size += expected_size;
> +		if (unsigned_add_overflows (total_size, (size_t)expected_size))
> +			total_size = SIZE_MAX;
> +		else
> +			total_size += expected_size;
> +

But this part I am not totally following. Here we have 'total_size'
declared as a size_t, and 'expected_size' as a uint64_t, and (on 32-bit
systems) down-cast to a 32-bit unsigned value.

So if 'expected_size' is larger than SIZE_MAX, we should set
'total_size' to SIZE_MAX. But that may not happen, say if
'expected_size' is (2^32-1<<32). Should total_size also be declared as a
uint64_t here?

I wondered if it might be easier to count down from the given batch_size
instead of adding up to it (requiring the second
unsigned_add_overflows() check). I tried it out and got this instead:

--- 8< ---
diff --git a/midx-write.c b/midx-write.c
index 48a4dc5e94..f81dd9ff6d 100644
--- a/midx-write.c
+++ b/midx-write.c
@@ -1671,7 +1671,7 @@ static void fill_included_packs_batch(struct repository *r,
 				      size_t batch_size)
 {
 	uint32_t i;
-	size_t total_size;
+	uint64_t remaining = batch_size;
 	struct repack_info *pack_info;
 	int pack_kept_objects = 0;

@@ -1695,23 +1695,23 @@ static void fill_included_packs_batch(struct repository *r,

 	QSORT(pack_info, m->num_packs, compare_by_mtime);

-	total_size = 0;
-	for (i = 0; total_size < batch_size && i < m->num_packs; i++) {
+	for (i = 0; i < m->num_packs; i++) {
 		int pack_int_id = pack_info[i].pack_int_id;
 		struct packed_git *p = m->packs[pack_int_id];
-		size_t expected_size;
+		uint64_t expected_size, factor;

 		if (!want_included_pack(r, m, pack_kept_objects, pack_int_id))
 			continue;

-		expected_size = st_mult(p->pack_size,
-					pack_info[i].referenced_objects);
-		expected_size /= p->num_objects;
+		factor = pack_info[i].referenced_objects / p->num_objects;
+		if (p->pack_size > UINT64_MAX / factor)
+			die(...);

-		if (expected_size >= batch_size)
-			continue;
+		expected_size = p->pack_size * factor;
+		if (expected_size > remaining)
+			break;

-		total_size += expected_size;
+		remaining -= expected_size;
 		include_pack[pack_int_id] = 1;
 	}
--- >8 ---

That reduces the two overflow checks down to one, and avoids the need to
introduce a uint64_t-specific variant of the st_add() function.

Thanks,
Taylor