| Message ID | 20200228162450.1720795-1-damien.olivier.robert+git@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/1] midx.c: fix an integer overflow | expand |
On Fri, Feb 28, 2020 at 05:24:49PM +0100, Damien Robert wrote: > When verifying a midx index with 0 objects, the > m->num_objects - 1 > overflows to 4294967295. > > Fix this. Makes sense. Such a midx shouldn't be generated in the first place, but we should handle it robustly if we do see one. > diff --git a/midx.c b/midx.c > index 37ec28623a..6ffe013089 100644 > --- a/midx.c > +++ b/midx.c > @@ -1127,7 +1127,7 @@ int verify_midx_file(struct repository *r, const char *object_dir, unsigned flag > if (flags & MIDX_PROGRESS) > progress = start_sparse_progress(_("Verifying OID order in multi-pack-index"), > m->num_objects - 1); > - for (i = 0; i < m->num_objects - 1; i++) { > + for (i = 0; i + 1 < m->num_objects; i++) { > struct object_id oid1, oid2; > > nth_midxed_object_oid(&oid1, m, i); [...] nth_midxed_object_oid(&oid2, m, i + 1); Perhaps it would be simpler as: for (i = 1; i < m->num_objects; i++) { ... nth_midxed_object_oid(&oid1, m, i - 1); nth_midxed_object_oid(&oid2, m, i); ... } Though I almost wonder if we should be catching "m->num_objects == 0" early and declaring the midx to be bogus (it's not _technically_ wrong, but I'd have to suspect a bug in anything that generated a 0-object midx file). -Peff
Jeff King <peff@peff.net> writes: >> - for (i = 0; i < m->num_objects - 1; i++) { >> + for (i = 0; i + 1 < m->num_objects; i++) { >> struct object_id oid1, oid2; >> >> nth_midxed_object_oid(&oid1, m, i); > [...] nth_midxed_object_oid(&oid2, m, i + 1); > > Perhaps it would be simpler as: > > for (i = 1; i < m->num_objects; i++) { > ... > nth_midxed_object_oid(&oid1, m, i - 1); > nth_midxed_object_oid(&oid2, m, i); > ... > } "Count up while i+1 is smaller than..." looked extremely unnatural and it was hard to grok, at least to me. This for (i = 0; i < m->num_objects - 1; i++) { might have been more palatable, but yours is much better. > Though I almost wonder if we should be catching "m->num_objects == 0" > early and declaring the midx to be bogus (it's not _technically_ wrong, > but I'd have to suspect a bug in anything that generated a 0-object midx > file). That, too ;-)
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?
From Junio C Hamano, Fri 28 Feb 2020 at 12:39:50 (-0800) : > "Count up while i+1 is smaller than..." looked extremely unnatural and > it was hard to grok, at least to me. This > for (i = 0; i < m->num_objects - 1; i++) { > > might have been more palatable, but yours is much better. This is probably a question of taste. The for (i = 0; i < m->num_objects - 1; i++) { looks like someone who forgot to use <= instead of < to me (until the body of the for explain that we are actually iterating over two consecutive objects), while for (i = 0; i + 1 < m->num_objects; i++) { makes it clear that we are iterating over two objects (and has the advantage of not overflowing :)) > > Though I almost wonder if we should be catching "m->num_objects == 0" > > early and declaring the midx to be bogus (it's not _technically_ wrong, > > but I'd have to suspect a bug in anything that generated a 0-object midx > > file). > That, too ;-) Yeah I'll go for that solution in my reroll.
diff --git a/midx.c b/midx.c index 37ec28623a..6ffe013089 100644 --- a/midx.c +++ b/midx.c @@ -1127,7 +1127,7 @@ int verify_midx_file(struct repository *r, const char *object_dir, unsigned flag if (flags & MIDX_PROGRESS) progress = start_sparse_progress(_("Verifying OID order in multi-pack-index"), m->num_objects - 1); - for (i = 0; i < m->num_objects - 1; i++) { + for (i = 0; i + 1 < m->num_objects; i++) { struct object_id oid1, oid2; nth_midxed_object_oid(&oid1, m, i);
When verifying a midx index with 0 objects, the m->num_objects - 1 overflows to 4294967295. Fix this. Signed-off-by: Damien Robert <damien.olivier.robert+git@gmail.com> --- midx.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)