Re: [PATCH 1/1] midx.c: fix an integer overflow

From: Damien Robert <damien.olivier.robert@gmail.com>
To: Jeff King <peff@peff.net>
Cc: git@vger.kernel.org, Junio C Hamano <gitster@pobox.com>,
	Derrick Stolee <dstolee@microsoft.com>,
	William Baker <William.Baker@microsoft.com>
Subject: Re: [PATCH 1/1] midx.c: fix an integer overflow
Date: Sat, 29 Feb 2020 16:38:55 +0100	[thread overview]
Message-ID: <20200229153855.o2s2lv4qiltej4ej@doriath> (raw)
In-Reply-To: <20200228185525.GB1408759@coredump.intra.peff.net>

From Jeff King, Fri 28 Feb 2020 at 13:55:25 (-0500) :
> Makes sense. Such a midx shouldn't be generated in the first place, but
> we should handle it robustly if we do see one.

This midx was actually written by `git multi-pack-index write`, when there
is no pack files in the store.

>   for (i = 1; i < m->num_objects; i++) {
>           ...
> 	  nth_midxed_object_oid(&oid1, m, i - 1);
> 	  nth_midxed_object_oid(&oid2, m, i);
> 	  ...
>   }

We could, but this mean that we have to shift all values of i by one in the
body. My patch has a smaller diff :)

> Though I almost wonder if we should be catching "m->num_objects == 0"
> early and declaring the midx to be bogus

This is probably the best solution. Should I also catch m->num_objects == 1?
Having a midx with only one pack does not make much sense either.

> (it's not _technically_ wrong, but I'd have to suspect a bug in anything
> that generated a 0-object midx file).

So mid.c:926 calls write_midx_header unconditionally. 
	written = write_midx_header(f, num_chunks, packs.nr - dropped_packs);
Maybe we could check that packs.nr - dropped_packds is > 0 first.

So I can reroll.
- I'll add a warning to verify if there is no pack in the midx. What about
  when there is one pack?
- Should I update write_midx_internal to not write anything if there is no
  pack?  What about if there is only one pack?
- Should I add tests?

-- 
Damien