# [PATCH 1/1] midx.c: fix an integer overflow 17 messages from 2020-02-28 to 2020-03-28. Participants: Damien Robert, Jeff King, Junio C Hamano, Derrick Stolee. Thread: https://gitlist.dev/t/52908 ## Damien Robert, 2020-02-28 16:24 Subject: [PATCH 1/1] midx.c: fix an integer overflow Message-ID: <20200228162450.1720795-1-damien.olivier.robert+git@gmail.com> URL: https://gitlist.dev/e/20200228162450.1720795-1-damien.olivier.robert%2Bgit%40gmail.com ``` When verifying a midx index with 0 objects, the m->num_objects - 1 overflows to 4294967295. Fix this. Signed-off-by: Damien Robert --- midx.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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); -- Patched on top of v2.25.1-379-gd22418c625 (git version 2.25.1) ``` ## Jeff King, 2020-02-28 18:55 Subject: Re: [PATCH 1/1] midx.c: fix an integer overflow Message-ID: <20200228185525.GB1408759@coredump.intra.peff.net> URL: https://gitlist.dev/e/20200228185525.GB1408759%40coredump.intra.peff.net In-Reply-To: <20200228162450.1720795-1-damien.olivier.robert+git@gmail.com> ``` 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 ``` ## Junio C Hamano, 2020-02-28 20:39 Subject: Re: [PATCH 1/1] midx.c: fix an integer overflow Message-ID: URL: https://gitlist.dev/e/xmqqzhd2cup5.fsf%40gitster-ct.c.googlers.com In-Reply-To: <20200228185525.GB1408759@coredump.intra.peff.net> ``` Jeff King 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 ;-) ``` ## Damien Robert, 2020-02-29 15:38 Subject: Re: [PATCH 1/1] midx.c: fix an integer overflow Message-ID: <20200229153855.o2s2lv4qiltej4ej@doriath> URL: https://gitlist.dev/e/20200229153855.o2s2lv4qiltej4ej%40doriath 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 ``` ## Damien Robert, 2020-02-29 17:15 Subject: Re: [PATCH 1/1] midx.c: fix an integer overflow Message-ID: <20200229154241.gxwb4u7l5u6iaveg@doriath> URL: https://gitlist.dev/e/20200229154241.gxwb4u7l5u6iaveg%40doriath In-Reply-To: ``` 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. -- Damien Robert http://www.normalesup.org/~robert/pro ``` ## Damien Robert, 2020-03-12 17:35 Subject: [PATCH v2 1/1] midx.c: fix an integer overflow Message-ID: <20200312173520.2401776-1-damien.olivier.robert+git@gmail.com> URL: https://gitlist.dev/e/20200312173520.2401776-1-damien.olivier.robert%2Bgit%40gmail.com In-Reply-To: <20200228162450.1720795-1-damien.olivier.robert+git@gmail.com> ``` When verifying a midx index with 0 objects, the m->num_objects - 1 overflows to 4294967295. Fix this both by checking that the midx contains at least one oid, and also that we don't write any midx when there is no packfiles. Signed-off-by: Damien Robert --- Should I add a test? It is a bit troublesome to generate a zero object midx file since this patch prevents it from using 'midx write'... midx.c | 35 +++++++++++++++++++++++------------ 1 file changed, 23 insertions(+), 12 deletions(-) diff --git a/midx.c b/midx.c index 1527e464a7..2cece7f9ea 100644 --- a/midx.c +++ b/midx.c @@ -923,6 +923,12 @@ static int write_midx_internal(const char *object_dir, struct multi_pack_index * cur_chunk = 0; num_chunks = large_offsets_needed ? 5 : 4; + if (packs.nr - dropped_packs == 0) { + error(_("no pack files to index.")); + result = 1; + goto cleanup; + } + written = write_midx_header(f, num_chunks, packs.nr - dropped_packs); chunk_ids[cur_chunk] = MIDX_CHUNKID_PACKNAMES; @@ -1124,22 +1130,27 @@ int verify_midx_file(struct repository *r, const char *object_dir, unsigned flag i, oid_fanout1, oid_fanout2, i + 1); } - 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++) { - struct object_id oid1, oid2; + if (m->num_objects == 0) + midx_report(_("Warning: the midx contains no oid.")); + else + { + 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++) { + struct object_id oid1, oid2; - nth_midxed_object_oid(&oid1, m, i); - nth_midxed_object_oid(&oid2, m, i + 1); + nth_midxed_object_oid(&oid1, m, i); + nth_midxed_object_oid(&oid2, m, i + 1); - if (oidcmp(&oid1, &oid2) >= 0) - midx_report(_("oid lookup out of order: oid[%d] = %s >= %s = oid[%d]"), - i, oid_to_hex(&oid1), oid_to_hex(&oid2), i + 1); + if (oidcmp(&oid1, &oid2) >= 0) + midx_report(_("oid lookup out of order: oid[%d] = %s >= %s = oid[%d]"), + i, oid_to_hex(&oid1), oid_to_hex(&oid2), i + 1); - midx_display_sparse_progress(progress, i + 1); + midx_display_sparse_progress(progress, i + 1); + } + stop_progress(&progress); } - stop_progress(&progress); /* * Create an array mapping each object to its packfile id. Sort it -- Patched on top of v2.26.0-rc1-6-ga56d361f66 (git version 2.25.1) ``` ## Damien Robert, 2020-03-12 18:24 Subject: Re: [PATCH v2 1/1] midx.c: fix an integer overflow Message-ID: <20200312182446.6zqjtudkptsvr6uh@feanor> URL: https://gitlist.dev/e/20200312182446.6zqjtudkptsvr6uh%40feanor In-Reply-To: <20200312173520.2401776-1-damien.olivier.robert+git@gmail.com> ``` From Damien Robert, Thu 12 Mar 2020 at 18:35:20 (+0100) : > When verifying a midx index with 0 objects, the > m->num_objects - 1 > overflows to 4294967295. > > Fix this both by checking that the midx contains at least one oid, > and also that we don't write any midx when there is no packfiles. I forgot to add: previously I was wondering about a warning when in 'write' when we only index one pack file, but this could make sense in the case of a 'midx repack'. So I only warn if there is no objects. ``` ## Derrick Stolee, 2020-03-12 18:28 Subject: Re: [PATCH v2 1/1] midx.c: fix an integer overflow Message-ID: URL: https://gitlist.dev/e/a538c497-a79d-43be-3d00-c7a619acc4e6%40gmail.com In-Reply-To: <20200312173520.2401776-1-damien.olivier.robert+git@gmail.com> ``` On 3/12/2020 1:35 PM, Damien Robert wrote: > When verifying a midx index with 0 objects, the > m->num_objects - 1 > overflows to 4294967295. > > Fix this both by checking that the midx contains at least one oid, > and also that we don't write any midx when there is no packfiles. > > Signed-off-by: Damien Robert > --- > Should I add a test? It is a bit troublesome to generate a zero object midx > file since this patch prevents it from using 'midx write'... I'm glad that your patch makes it impossible to generate a zero-object multi-pack-index, and that makes a test hard to implement. I'm not sure what history Git has for storing explicit binary content into the test suite. There really is only one "empty" multi-pack-index, but it is unfortunately still a bit big for a test case to write explicitly due to the 256-word fanout table. I _think_ the t/tXXXX directories are used for this kind of data storage, so you could generate an empty multi-pack-index from an older version of Git then store it there. Please wait for someone else on-list to say that this is a good idea, though. It may not be worth the pain of a binary file in the patch. > midx.c | 35 +++++++++++++++++++++++------------ > 1 file changed, 23 insertions(+), 12 deletions(-) > > diff --git a/midx.c b/midx.c > index 1527e464a7..2cece7f9ea 100644 > --- a/midx.c > +++ b/midx.c > @@ -923,6 +923,12 @@ static int write_midx_internal(const char *object_dir, struct multi_pack_index * > cur_chunk = 0; > num_chunks = large_offsets_needed ? 5 : 4; > > + if (packs.nr - dropped_packs == 0) { > + error(_("no pack files to index.")); nit: I would use "pack-files" here. Second best is "packfiles". > + result = 1; > + goto cleanup; > + } > + > written = write_midx_header(f, num_chunks, packs.nr - dropped_packs); > > chunk_ids[cur_chunk] = MIDX_CHUNKID_PACKNAMES; > @@ -1124,22 +1130,27 @@ int verify_midx_file(struct repository *r, const char *object_dir, unsigned flag > i, oid_fanout1, oid_fanout2, i + 1); > } > > - 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++) { > - struct object_id oid1, oid2; > + if (m->num_objects == 0) > + midx_report(_("Warning: the midx contains no oid.")); Should this "Warning: " be here? The other calls to midx_report() do not have such prefix. It could be valuable to add "warning: %s\n" to the fprintf inside midx_report(), but that should be done as its own patch. Also, it may be valuable to return from this block so you do not need to put the block below in a tabbed block, reducing the complexity of this patch. > + else > + { > + 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++) { > + struct object_id oid1, oid2; > > - nth_midxed_object_oid(&oid1, m, i); > - nth_midxed_object_oid(&oid2, m, i + 1); > + nth_midxed_object_oid(&oid1, m, i); > + nth_midxed_object_oid(&oid2, m, i + 1); > > - if (oidcmp(&oid1, &oid2) >= 0) > - midx_report(_("oid lookup out of order: oid[%d] = %s >= %s = oid[%d]"), > - i, oid_to_hex(&oid1), oid_to_hex(&oid2), i + 1); > + if (oidcmp(&oid1, &oid2) >= 0) > + midx_report(_("oid lookup out of order: oid[%d] = %s >= %s = oid[%d]"), > + i, oid_to_hex(&oid1), oid_to_hex(&oid2), i + 1); > > - midx_display_sparse_progress(progress, i + 1); > + midx_display_sparse_progress(progress, i + 1); > + } > + stop_progress(&progress); > } > - stop_progress(&progress); > > /* > * Create an array mapping each object to its packfile id. Sort it > Thanks for digging into this! -Stolee ``` ## Damien Robert, 2020-03-12 21:41 Subject: Re: [PATCH v2 1/1] midx.c: fix an integer overflow Message-ID: <20200312214123.sqja3fvuiaaspxwp@doriath> URL: https://gitlist.dev/e/20200312214123.sqja3fvuiaaspxwp%40doriath In-Reply-To: ``` From Derrick Stolee, Thu 12 Mar 2020 at 14:28:11 (-0400) : > I _think_ the t/tXXXX directories are used for this kind of data storage, > so you could generate an empty multi-pack-index from an older version of > Git then store it there. Yes I anticipated that and have one available on hand :) It weights 1116 characters. > > - 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++) { > > - struct object_id oid1, oid2; > > + if (m->num_objects == 0) > > + midx_report(_("Warning: the midx contains no oid.")); > > Should this "Warning: " be here? The other calls to midx_report() do not have such prefix. Right, I agree it should not. > Also, it may be valuable to return from this block so you do not need to put the block below in a tabbed block, reducing the complexity of this patch. Agreed: we don't want to run the other checks anyway if we don't have any objects. That'll be for v3 once I get advice on what to do for tests. ``` ## Damien Robert, 2020-03-23 22:25 Subject: [PATCH v3 1/1] midx.c: fix an integer overflow Message-ID: <20200323222515.779477-1-damien.olivier.robert+git@gmail.com> URL: https://gitlist.dev/e/20200323222515.779477-1-damien.olivier.robert%2Bgit%40gmail.com In-Reply-To: <20200312173520.2401776-1-damien.olivier.robert+git@gmail.com> ``` When verifying a midx index with 0 objects, the m->num_objects - 1 overflows to 4294967295. Fix this both by checking that the midx contains at least one oid, and also that we don't write any midx when there is no packfiles. Update the tests so that we check that `git multi-pack-index write` does not write an midx when there is no object. Signed-off-by: Damien Robert --- Since I did not receive any guidelines, I did not upload an midx with no object to check in the tests. I just modified the current tests to check that we don't produce an midx if there is no objects. midx.c | 13 +++++++++++++ t/t5319-multi-pack-index.sh | 7 +++---- 2 files changed, 16 insertions(+), 4 deletions(-) diff --git a/midx.c b/midx.c index 1527e464a7..018acc7e76 100644 --- a/midx.c +++ b/midx.c @@ -923,6 +923,12 @@ static int write_midx_internal(const char *object_dir, struct multi_pack_index * cur_chunk = 0; num_chunks = large_offsets_needed ? 5 : 4; + if (packs.nr - dropped_packs == 0) { + error(_("no pack files to index.")); + result = 1; + goto cleanup; + } + written = write_midx_header(f, num_chunks, packs.nr - dropped_packs); chunk_ids[cur_chunk] = MIDX_CHUNKID_PACKNAMES; @@ -1124,6 +1130,13 @@ int verify_midx_file(struct repository *r, const char *object_dir, unsigned flag i, oid_fanout1, oid_fanout2, i + 1); } + if (m->num_objects == 0) { + midx_report(_("the midx contains no oid")); + // remaining tests assume that we have objects, so we can + // return here + return verify_midx_error; + } + if (flags & MIDX_PROGRESS) progress = start_sparse_progress(_("Verifying OID order in multi-pack-index"), m->num_objects - 1); diff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh index 43a7a66c9d..d90dfce268 100755 --- a/t/t5319-multi-pack-index.sh +++ b/t/t5319-multi-pack-index.sh @@ -42,10 +42,9 @@ test_expect_success 'setup' ' EOF ' -test_expect_success 'write midx with no packs' ' - test_when_finished rm -f pack/multi-pack-index && - git multi-pack-index --object-dir=. write && - midx_read_expect 0 0 4 . +test_expect_success "don't write midx with no packs" ' + test_must_fail git multi-pack-index --object-dir=. write && + test_path_is_missing pack/multi-pack-index ' generate_objects () { -- Patched on top of v2.26.0 (git version 2.25.1) ``` ## Jeff King, 2020-03-24 06:01 Subject: Re: [PATCH v3 1/1] midx.c: fix an integer overflow Message-ID: <20200324060125.GA610977@coredump.intra.peff.net> URL: https://gitlist.dev/e/20200324060125.GA610977%40coredump.intra.peff.net In-Reply-To: <20200323222515.779477-1-damien.olivier.robert+git@gmail.com> ``` On Mon, Mar 23, 2020 at 11:25:15PM +0100, Damien Robert wrote: > When verifying a midx index with 0 objects, the > m->num_objects - 1 > overflows to 4294967295. > > Fix this both by checking that the midx contains at least one oid, > and also that we don't write any midx when there is no packfiles. > > Update the tests so that we check that `git multi-pack-index write` does > not write an midx when there is no object. Thanks, both sides of this make sense. > --- > Since I did not receive any guidelines, I did not upload an midx with no > object to check in the tests. I just modified the current tests to check > that we don't produce an midx if there is no objects. I'd be OK with just this, but adding a binary t/t5319/zero-objs.midx would be fine by me, too. One minor style nit: > @@ -1124,6 +1130,13 @@ int verify_midx_file(struct repository *r, const char *object_dir, unsigned flag > i, oid_fanout1, oid_fanout2, i + 1); > } > > + if (m->num_objects == 0) { > + midx_report(_("the midx contains no oid")); > + // remaining tests assume that we have objects, so we can > + // return here > + return verify_midx_error; > + } We prefer /**/ for comments, like: /* * Remaining tests assume that we have objects, so we can * return here. */ -Peff ``` ## Junio C Hamano, 2020-03-24 18:48 Subject: Re: [PATCH v3 1/1] midx.c: fix an integer overflow Message-ID: URL: https://gitlist.dev/e/xmqqtv2d1td4.fsf%40gitster.c.googlers.com In-Reply-To: <20200324060125.GA610977@coredump.intra.peff.net> ``` Jeff King writes: > I'd be OK with just this, but adding a binary t/t5319/zero-objs.midx > would be fine by me, too. Yup, that sounds like a simple way to make sure we won't regress. > One minor style nit: > >> @@ -1124,6 +1130,13 @@ int verify_midx_file(struct repository *r, const char *object_dir, unsigned flag >> i, oid_fanout1, oid_fanout2, i + 1); >> } >> >> + if (m->num_objects == 0) { >> + midx_report(_("the midx contains no oid")); >> + // remaining tests assume that we have objects, so we can >> + // return here >> + return verify_midx_error; >> + } > > We prefer /**/ for comments, like: > > /* > * Remaining tests assume that we have objects, so we can > * return here. > */ Thanks for catching it. ``` ## Damien Robert, 2020-03-26 21:35 Subject: [PATCH v4 1/1] midx.c: fix an integer overflow Message-ID: <20200326213534.399377-1-damien.olivier.robert+git@gmail.com> URL: https://gitlist.dev/e/20200326213534.399377-1-damien.olivier.robert%2Bgit%40gmail.com In-Reply-To: <20200312173520.2401776-1-damien.olivier.robert+git@gmail.com> ``` When verifying a midx index with 0 objects, the m->num_objects - 1 overflows to 4294967295. Fix this both by checking that the midx contains at least one oid, and also that we don't write any midx when there is no packfiles. Update the tests to check that `git multi-pack-index write` does not write an midx when there is no objects, and another to check that `git multi-pack-index verify` warns when it verifies an midx with no objects. For this last test, use t5319/no-objects.midx which was generated by an older version of git. Signed-off-by: Damien Robert --- Following the recommandations I uploaded an empty midx to check we don't regress. midx.c | 15 +++++++++++++++ t/t5319-multi-pack-index.sh | 13 +++++++++---- t/t5319/no-objects.midx | Bin 0 -> 1116 bytes 3 files changed, 24 insertions(+), 4 deletions(-) create mode 100644 t/t5319/no-objects.midx diff --git a/midx.c b/midx.c index 1527e464a7..a520e26395 100644 --- a/midx.c +++ b/midx.c @@ -923,6 +923,12 @@ static int write_midx_internal(const char *object_dir, struct multi_pack_index * cur_chunk = 0; num_chunks = large_offsets_needed ? 5 : 4; + if (packs.nr - dropped_packs == 0) { + error(_("no pack files to index.")); + result = 1; + goto cleanup; + } + written = write_midx_header(f, num_chunks, packs.nr - dropped_packs); chunk_ids[cur_chunk] = MIDX_CHUNKID_PACKNAMES; @@ -1124,6 +1130,15 @@ int verify_midx_file(struct repository *r, const char *object_dir, unsigned flag i, oid_fanout1, oid_fanout2, i + 1); } + if (m->num_objects == 0) { + midx_report(_("the midx contains no oid")); + /* + * Remaining tests assume that we have objects, so we can + * return here. + */ + return verify_midx_error; + } + if (flags & MIDX_PROGRESS) progress = start_sparse_progress(_("Verifying OID order in multi-pack-index"), m->num_objects - 1); diff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh index 43a7a66c9d..10c35d445d 100755 --- a/t/t5319-multi-pack-index.sh +++ b/t/t5319-multi-pack-index.sh @@ -42,10 +42,15 @@ test_expect_success 'setup' ' EOF ' -test_expect_success 'write midx with no packs' ' - test_when_finished rm -f pack/multi-pack-index && - git multi-pack-index --object-dir=. write && - midx_read_expect 0 0 4 . +test_expect_success "don't write midx with no packs" ' + test_must_fail git multi-pack-index --object-dir=. write && + test_path_is_missing pack/multi-pack-index +' + +test_expect_success "Warn if a midx contains no oid" ' + cp "$TEST_DIRECTORY"/t5319/no-objects.midx .git/objects/pack/multi-pack-index && + test_must_fail git multi-pack-index verify && + rm .git/objects/pack/multi-pack-index ' generate_objects () { diff --git a/t/t5319/no-objects.midx b/t/t5319/no-objects.midx new file mode 100644 index 0000000000000000000000000000000000000000..e466b8e08654c29effb5248cb109d81cfbcfd2f4 GIT binary patch literal 1116 zcmebEbctYOWMKe-06#}xFoS`?!{5`z4T URL: https://gitlist.dev/e/xmqqbloiitnv.fsf%40gitster.c.googlers.com In-Reply-To: <20200326213534.399377-1-damien.olivier.robert+git@gmail.com> ``` Damien Robert writes: > When verifying a midx index with 0 objects, the > m->num_objects - 1 > overflows to 4294967295. I think this is underflow & wraparound, but I'll let it pass ;-) The patch looks good; will queue. Thanks. ``` ## Damien Robert, 2020-03-28 22:18 Subject: [PATCH 1/1] midx.c: fix an integer underflow Message-ID: <20200328221822.228469-1-damien.olivier.robert+git@gmail.com> URL: https://gitlist.dev/e/20200328221822.228469-1-damien.olivier.robert%2Bgit%40gmail.com In-Reply-To: <20200312173520.2401776-1-damien.olivier.robert+git@gmail.com> ``` When verifying a midx index with 0 objects, the m->num_objects - 1 underflows and wraps around to 4294967295. Fix this both by checking that the midx contains at least one oid, and also that we don't write any midx when there is no packfiles. Update the tests to check that `git multi-pack-index write` does not write an midx when there is no objects, and another to check that `git multi-pack-index verify` warns when it verifies an midx with no objects. For this last test, use t5319/no-objects.midx which was generated by an older version of git. Signed-off-by: Damien Robert --- Change since v4: use "$objdir" in tests rather than ".git/objects/" Use this opportunity to use the correct technical term in the commit message, as pointed out by Junio. midx.c | 15 +++++++++++++++ t/t5319-multi-pack-index.sh | 13 +++++++++---- t/t5319/no-objects.midx | Bin 0 -> 1116 bytes 3 files changed, 24 insertions(+), 4 deletions(-) create mode 100644 t/t5319/no-objects.midx diff --git a/midx.c b/midx.c index 1527e464a7..a520e26395 100644 --- a/midx.c +++ b/midx.c @@ -923,6 +923,12 @@ static int write_midx_internal(const char *object_dir, struct multi_pack_index * cur_chunk = 0; num_chunks = large_offsets_needed ? 5 : 4; + if (packs.nr - dropped_packs == 0) { + error(_("no pack files to index.")); + result = 1; + goto cleanup; + } + written = write_midx_header(f, num_chunks, packs.nr - dropped_packs); chunk_ids[cur_chunk] = MIDX_CHUNKID_PACKNAMES; @@ -1124,6 +1130,15 @@ int verify_midx_file(struct repository *r, const char *object_dir, unsigned flag i, oid_fanout1, oid_fanout2, i + 1); } + if (m->num_objects == 0) { + midx_report(_("the midx contains no oid")); + /* + * Remaining tests assume that we have objects, so we can + * return here. + */ + return verify_midx_error; + } + if (flags & MIDX_PROGRESS) progress = start_sparse_progress(_("Verifying OID order in multi-pack-index"), m->num_objects - 1); diff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh index 43a7a66c9d..22240fd30b 100755 --- a/t/t5319-multi-pack-index.sh +++ b/t/t5319-multi-pack-index.sh @@ -42,10 +42,15 @@ test_expect_success 'setup' ' EOF ' -test_expect_success 'write midx with no packs' ' - test_when_finished rm -f pack/multi-pack-index && - git multi-pack-index --object-dir=. write && - midx_read_expect 0 0 4 . +test_expect_success "don't write midx with no packs" ' + test_must_fail git multi-pack-index --object-dir=. write && + test_path_is_missing pack/multi-pack-index +' + +test_expect_success "Warn if a midx contains no oid" ' + cp "$TEST_DIRECTORY"/t5319/no-objects.midx $objdir/pack/multi-pack-index && + test_must_fail git multi-pack-index verify && + rm $objdir/pack/multi-pack-index ' generate_objects () { diff --git a/t/t5319/no-objects.midx b/t/t5319/no-objects.midx new file mode 100644 index 0000000000000000000000000000000000000000..e466b8e08654c29effb5248cb109d81cfbcfd2f4 GIT binary patch literal 1116 zcmebEbctYOWMKe-06#}xFoS`?!{5`z4T URL: https://gitlist.dev/e/20200328222342.jfxsk3gvujgqdt4f%40doriath In-Reply-To: ``` From Junio C Hamano, Thu 26 Mar 2020 at 16:27:16 (-0700) : > I think this is underflow & wraparound, but I'll let it pass ;-) Ah thanks for the 'wraparound' term, I was missing it, so that's why I wrote overflow instead since everybody understand what I meant. Mathematically I think of this as a truncating in the ring of 2-adic integers, but I did not want to write this in the commit message :) > The patch looks good; will queue. Thanks. I just sent a v5 with a microfix (which is not really important, so v5 can be dropped). Since pu is subject to be rewound anyway, I guess you prefer a new version rather than a patch on top of v4? -- Damien Robert http://www.normalesup.org/~robert/pro ``` ## Junio C Hamano, 2020-03-28 23:51 Subject: Re: [PATCH v4 1/1] midx.c: fix an integer overflow Message-ID: URL: https://gitlist.dev/e/xmqqlfnkca22.fsf%40gitster.c.googlers.com In-Reply-To: <20200328222342.jfxsk3gvujgqdt4f@doriath> ``` Damien Robert writes: > Mathematically I think of this as a truncating in the ring of 2-adic integers, > but I did not want to write this in the commit message :) ;-) >> The patch looks good; will queue. > > Thanks. I just sent a v5 with a microfix (which is not really important, so > v5 can be dropped). Since pu is subject to be rewound anyway, I guess you > prefer a new version rather than a patch on top of v4? OK, will replace. Thanks for your attention to the details. ```