{"thread":{"id":"52908","subject":"[PATCH 1/1] midx.c: fix an integer overflow","startedAt":"2020-02-28T16:25:58Z","lastAt":"2020-03-28T23:51:54Z","messageCount":17,"participants":["Damien Robert","Jeff King","Junio C Hamano","Derrick Stolee"],"isPatch":true,"patchVersion":1,"patchTotal":1},"messages":[{"id":"392669","messageId":"20200228162450.1720795-1-damien.olivier.robert+git@gmail.com","threadId":"52908","inReplyTo":null,"subject":"[PATCH 1/1] midx.c: fix an integer overflow","fromName":"Damien Robert","fromEmail":"damien.olivier.robert@gmail.com","sentAt":"2020-02-28T16:24:49Z","receivedAt":"2020-02-28T16:25:58Z","isPatch":true,"sender":{"key":"damien.olivier.robert@gmail.com","avatar":null},"body":"When verifying a midx index with 0 objects, the\n m->num_objects - 1\noverflows to 4294967295.\n\nFix this.\n\nSigned-off-by: Damien Robert \n---\n midx.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/midx.c b/midx.c\nindex 37ec28623a..6ffe013089 100644\n--- a/midx.c\n+++ b/midx.c\n@@ -1127,7 +1127,7 @@ int verify_midx_file(struct repository *r, const char *object_dir, unsigned flag\n \tif (flags & MIDX_PROGRESS)\n \t\tprogress = start_sparse_progress(_(\"Verifying OID order in multi-pack-index\"),\n \t\t\t\t\t\t m->num_objects - 1);\n-\tfor (i = 0; i < m->num_objects - 1; i++) {\n+\tfor (i = 0; i + 1 < m->num_objects; i++) {\n \t\tstruct object_id oid1, oid2;\n \n \t\tnth_midxed_object_oid(&oid1, m, i);\n-- \nPatched on top of v2.25.1-379-gd22418c625 (git version 2.25.1)\n\n"},{"id":"392681","messageId":"20200228185525.GB1408759@coredump.intra.peff.net","threadId":"52908","inReplyTo":"20200228162450.1720795-1-damien.olivier.robert+git@gmail.com","subject":"Re: [PATCH 1/1] midx.c: fix an integer overflow","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-02-28T18:55:25Z","receivedAt":"2020-02-28T18:55:27Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Feb 28, 2020 at 05:24:49PM +0100, Damien Robert wrote:\n\n> When verifying a midx index with 0 objects, the\n> m->num_objects - 1\n> overflows to 4294967295.\n> \n> Fix this.\n\nMakes sense. Such a midx shouldn't be generated in the first place, but\nwe should handle it robustly if we do see one.\n\n> diff --git a/midx.c b/midx.c\n> index 37ec28623a..6ffe013089 100644\n> --- a/midx.c\n> +++ b/midx.c\n> @@ -1127,7 +1127,7 @@ int verify_midx_file(struct repository *r, const char *object_dir, unsigned flag\n> \tif (flags & MIDX_PROGRESS)\n> \t\tprogress = start_sparse_progress(_(\"Verifying OID order in multi-pack-index\"),\n> \t\t\t\t\t\t m->num_objects - 1);\n> -\tfor (i = 0; i < m->num_objects - 1; i++) {\n> +\tfor (i = 0; i + 1 < m->num_objects; i++) {\n> \t\tstruct object_id oid1, oid2;\n> \n> \t\tnth_midxed_object_oid(&oid1, m, i);\n[...] nth_midxed_object_oid(&oid2, m, i + 1);\n\nPerhaps it would be simpler as:\n\n for (i = 1; i < m->num_objects; i++) {\n ...\n\t nth_midxed_object_oid(&oid1, m, i - 1);\n\t nth_midxed_object_oid(&oid2, m, i);\n\t ...\n }\n\nThough I almost wonder if we should be catching \"m->num_objects == 0\"\nearly and declaring the midx to be bogus (it's not _technically_ wrong,\nbut I'd have to suspect a bug in anything that generated a 0-object midx\nfile).\n\n-Peff\n"},{"id":"392688","messageId":"xmqqzhd2cup5.fsf@gitster-ct.c.googlers.com","threadId":"52908","inReplyTo":"20200228185525.GB1408759@coredump.intra.peff.net","subject":"Re: [PATCH 1/1] midx.c: fix an integer overflow","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-02-28T20:39:50Z","receivedAt":"2020-02-28T20:39:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n>> -\tfor (i = 0; i < m->num_objects - 1; i++) {\n>> +\tfor (i = 0; i + 1 < m->num_objects; i++) {\n>> \t\tstruct object_id oid1, oid2;\n>> \n>> \t\tnth_midxed_object_oid(&oid1, m, i);\n> [...] nth_midxed_object_oid(&oid2, m, i + 1);\n>\n> Perhaps it would be simpler as:\n>\n> for (i = 1; i < m->num_objects; i++) {\n> ...\n> \t nth_midxed_object_oid(&oid1, m, i - 1);\n> \t nth_midxed_object_oid(&oid2, m, i);\n> \t ...\n> }\n\n\"Count up while i+1 is smaller than...\" looked extremely unnatural and\nit was hard to grok, at least to me. This\n\n\tfor (i = 0; i < m->num_objects - 1; i++) {\n\nmight have been more palatable, but yours is much better.\n\n> Though I almost wonder if we should be catching \"m->num_objects == 0\"\n> early and declaring the midx to be bogus (it's not _technically_ wrong,\n> but I'd have to suspect a bug in anything that generated a 0-object midx\n> file).\n\nThat, too ;-)\n"},{"id":"392710","messageId":"20200229153855.o2s2lv4qiltej4ej@doriath","threadId":"52908","inReplyTo":"20200228185525.GB1408759@coredump.intra.peff.net","subject":"Re: [PATCH 1/1] midx.c: fix an integer overflow","fromName":"Damien Robert","fromEmail":"damien.olivier.robert@gmail.com","sentAt":"2020-02-29T15:38:55Z","receivedAt":"2020-02-29T15:39:03Z","isPatch":true,"sender":{"key":"damien.olivier.robert@gmail.com","avatar":null},"body":"From Jeff King, Fri 28 Feb 2020 at 13:55:25 (-0500) :\n> Makes sense. Such a midx shouldn't be generated in the first place, but\n> we should handle it robustly if we do see one.\n\nThis midx was actually written by `git multi-pack-index write`, when there\nis no pack files in the store.\n\n> for (i = 1; i < m->num_objects; i++) {\n> ...\n> \t nth_midxed_object_oid(&oid1, m, i - 1);\n> \t nth_midxed_object_oid(&oid2, m, i);\n> \t ...\n> }\n\nWe could, but this mean that we have to shift all values of i by one in the\nbody. My patch has a smaller diff :)\n\n> Though I almost wonder if we should be catching \"m->num_objects == 0\"\n> early and declaring the midx to be bogus\n\nThis is probably the best solution. Should I also catch m->num_objects == 1?\nHaving a midx with only one pack does not make much sense either.\n\n> (it's not _technically_ wrong, but I'd have to suspect a bug in anything\n> that generated a 0-object midx file).\n\nSo mid.c:926 calls write_midx_header unconditionally. \n\twritten = write_midx_header(f, num_chunks, packs.nr - dropped_packs);\nMaybe we could check that packs.nr - dropped_packds is > 0 first.\n\nSo I can reroll.\n- I'll add a warning to verify if there is no pack in the midx. What about\n when there is one pack?\n- Should I update write_midx_internal to not write anything if there is no\n pack? What about if there is only one pack?\n- Should I add tests?\n\n-- \nDamien\n"},{"id":"392715","messageId":"20200229154241.gxwb4u7l5u6iaveg@doriath","threadId":"52908","inReplyTo":"xmqqzhd2cup5.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 1/1] midx.c: fix an integer overflow","fromName":"Damien Robert","fromEmail":"damien.olivier.robert@gmail.com","sentAt":"2020-02-29T17:15:40Z","receivedAt":"2020-02-29T17:15:59Z","isPatch":true,"sender":{"key":"damien.olivier.robert@gmail.com","avatar":null},"body":"From Junio C Hamano, Fri 28 Feb 2020 at 12:39:50 (-0800) :\n> \"Count up while i+1 is smaller than...\" looked extremely unnatural and\n> it was hard to grok, at least to me. This\n\n> \tfor (i = 0; i < m->num_objects - 1; i++) {\n> \n> might have been more palatable, but yours is much better.\n\nThis is probably a question of taste. The\n\tfor (i = 0; i < m->num_objects - 1; i++) {\nlooks like someone who forgot to use <= instead of < to me (until the body\nof the for explain that we are actually iterating over two consecutive\nobjects), while\n\tfor (i = 0; i + 1 < m->num_objects; i++) {\nmakes it clear that we are iterating over two objects (and has the\nadvantage of not overflowing :))\n\n> > Though I almost wonder if we should be catching \"m->num_objects == 0\"\n> > early and declaring the midx to be bogus (it's not _technically_ wrong,\n> > but I'd have to suspect a bug in anything that generated a 0-object midx\n> > file).\n> That, too ;-)\n\nYeah I'll go for that solution in my reroll.\n\n-- \nDamien Robert\nhttp://www.normalesup.org/~robert/pro\n"},{"id":"393145","messageId":"20200312173520.2401776-1-damien.olivier.robert+git@gmail.com","threadId":"52908","inReplyTo":"20200228162450.1720795-1-damien.olivier.robert+git@gmail.com","subject":"[PATCH v2 1/1] midx.c: fix an integer overflow","fromName":"Damien Robert","fromEmail":"damien.olivier.robert@gmail.com","sentAt":"2020-03-12T17:35:20Z","receivedAt":"2020-03-12T17:35:57Z","isPatch":true,"sender":{"key":"damien.olivier.robert@gmail.com","avatar":null},"body":"When verifying a midx index with 0 objects, the\n m->num_objects - 1\noverflows to 4294967295.\n\nFix this both by checking that the midx contains at least one oid,\nand also that we don't write any midx when there is no packfiles.\n\nSigned-off-by: Damien Robert \n---\nShould I add a test? It is a bit troublesome to generate a zero object midx\nfile since this patch prevents it from using 'midx write'...\n\n midx.c | 35 +++++++++++++++++++++++------------\n 1 file changed, 23 insertions(+), 12 deletions(-)\n\ndiff --git a/midx.c b/midx.c\nindex 1527e464a7..2cece7f9ea 100644\n--- a/midx.c\n+++ b/midx.c\n@@ -923,6 +923,12 @@ static int write_midx_internal(const char *object_dir, struct multi_pack_index *\n \tcur_chunk = 0;\n \tnum_chunks = large_offsets_needed ? 5 : 4;\n \n+\tif (packs.nr - dropped_packs == 0) {\n+\t\terror(_(\"no pack files to index.\"));\n+\t\tresult = 1;\n+\t\tgoto cleanup;\n+\t}\n+\n \twritten = write_midx_header(f, num_chunks, packs.nr - dropped_packs);\n \n \tchunk_ids[cur_chunk] = MIDX_CHUNKID_PACKNAMES;\n@@ -1124,22 +1130,27 @@ int verify_midx_file(struct repository *r, const char *object_dir, unsigned flag\n \t\t\t\t i, oid_fanout1, oid_fanout2, i + 1);\n \t}\n \n-\tif (flags & MIDX_PROGRESS)\n-\t\tprogress = start_sparse_progress(_(\"Verifying OID order in multi-pack-index\"),\n-\t\t\t\t\t\t m->num_objects - 1);\n-\tfor (i = 0; i < m->num_objects - 1; i++) {\n-\t\tstruct object_id oid1, oid2;\n+\tif (m->num_objects == 0)\n+\t\tmidx_report(_(\"Warning: the midx contains no oid.\"));\n+\telse\n+\t{\n+\t\tif (flags & MIDX_PROGRESS)\n+\t\t\tprogress = start_sparse_progress(_(\"Verifying OID order in multi-pack-index\"),\n+\t\t\t\t\t\t\t m->num_objects - 1);\n+\t\tfor (i = 0; i < m->num_objects - 1; i++) {\n+\t\t\tstruct object_id oid1, oid2;\n \n-\t\tnth_midxed_object_oid(&oid1, m, i);\n-\t\tnth_midxed_object_oid(&oid2, m, i + 1);\n+\t\t\tnth_midxed_object_oid(&oid1, m, i);\n+\t\t\tnth_midxed_object_oid(&oid2, m, i + 1);\n \n-\t\tif (oidcmp(&oid1, &oid2) >= 0)\n-\t\t\tmidx_report(_(\"oid lookup out of order: oid[%d] = %s >= %s = oid[%d]\"),\n-\t\t\t\t i, oid_to_hex(&oid1), oid_to_hex(&oid2), i + 1);\n+\t\t\tif (oidcmp(&oid1, &oid2) >= 0)\n+\t\t\t\tmidx_report(_(\"oid lookup out of order: oid[%d] = %s >= %s = oid[%d]\"),\n+\t\t\t\t\t i, oid_to_hex(&oid1), oid_to_hex(&oid2), i + 1);\n \n-\t\tmidx_display_sparse_progress(progress, i + 1);\n+\t\t\tmidx_display_sparse_progress(progress, i + 1);\n+\t\t}\n+\t\tstop_progress(&progress);\n \t}\n-\tstop_progress(&progress);\n \n \t/*\n \t * Create an array mapping each object to its packfile id. Sort it\n-- \nPatched on top of v2.26.0-rc1-6-ga56d361f66 (git version 2.25.1)\n\n"},{"id":"393154","messageId":"20200312182446.6zqjtudkptsvr6uh@feanor","threadId":"52908","inReplyTo":"20200312173520.2401776-1-damien.olivier.robert+git@gmail.com","subject":"Re: [PATCH v2 1/1] midx.c: fix an integer overflow","fromName":"Damien Robert","fromEmail":"damien.olivier.robert@gmail.com","sentAt":"2020-03-12T18:24:46Z","receivedAt":"2020-03-12T18:24:56Z","isPatch":true,"sender":{"key":"damien.olivier.robert@gmail.com","avatar":null},"body":"From Damien Robert, Thu 12 Mar 2020 at 18:35:20 (+0100) :\n> When verifying a midx index with 0 objects, the\n> m->num_objects - 1\n> overflows to 4294967295.\n> \n> Fix this both by checking that the midx contains at least one oid,\n> and also that we don't write any midx when there is no packfiles.\n\nI forgot to add: previously I was wondering about a warning when in 'write'\nwhen we only index one pack file, but this could make sense in the case of\na 'midx repack'. So I only warn if there is no objects.\n"},{"id":"393155","messageId":"a538c497-a79d-43be-3d00-c7a619acc4e6@gmail.com","threadId":"52908","inReplyTo":"20200312173520.2401776-1-damien.olivier.robert+git@gmail.com","subject":"Re: [PATCH v2 1/1] midx.c: fix an integer overflow","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2020-03-12T18:28:11Z","receivedAt":"2020-03-12T18:28:18Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 3/12/2020 1:35 PM, Damien Robert wrote:\n> When verifying a midx index with 0 objects, the\n> m->num_objects - 1\n> overflows to 4294967295.\n> \n> Fix this both by checking that the midx contains at least one oid,\n> and also that we don't write any midx when there is no packfiles.\n> \n> Signed-off-by: Damien Robert \n> ---\n> Should I add a test? It is a bit troublesome to generate a zero object midx\n> file since this patch prevents it from using 'midx write'...\n\nI'm glad that your patch makes it impossible to generate a zero-object\nmulti-pack-index, and that makes a test hard to implement. I'm not sure\nwhat history Git has for storing explicit binary content into the test\nsuite. There really is only one \"empty\" multi-pack-index, but it is\nunfortunately still a bit big for a test case to write explicitly due\nto the 256-word fanout table.\n\nI _think_ the t/tXXXX directories are used for this kind of data storage,\nso you could generate an empty multi-pack-index from an older version of\nGit then store it there. Please wait for someone else on-list to say that\nthis is a good idea, though. It may not be worth the pain of a binary file\nin the patch.\n\n> midx.c | 35 +++++++++++++++++++++++------------\n> 1 file changed, 23 insertions(+), 12 deletions(-)\n> \n> diff --git a/midx.c b/midx.c\n> index 1527e464a7..2cece7f9ea 100644\n> --- a/midx.c\n> +++ b/midx.c\n> @@ -923,6 +923,12 @@ static int write_midx_internal(const char *object_dir, struct multi_pack_index *\n> \tcur_chunk = 0;\n> \tnum_chunks = large_offsets_needed ? 5 : 4;\n> \n> +\tif (packs.nr - dropped_packs == 0) {\n> +\t\terror(_(\"no pack files to index.\"));\n\nnit: I would use \"pack-files\" here. Second best is \"packfiles\".\n\n> +\t\tresult = 1;\n> +\t\tgoto cleanup;\n> +\t}\n> +\n> \twritten = write_midx_header(f, num_chunks, packs.nr - dropped_packs);\n> \n> \tchunk_ids[cur_chunk] = MIDX_CHUNKID_PACKNAMES;\n> @@ -1124,22 +1130,27 @@ int verify_midx_file(struct repository *r, const char *object_dir, unsigned flag\n> \t\t\t\t i, oid_fanout1, oid_fanout2, i + 1);\n> \t}\n> \n> -\tif (flags & MIDX_PROGRESS)\n> -\t\tprogress = start_sparse_progress(_(\"Verifying OID order in multi-pack-index\"),\n> -\t\t\t\t\t\t m->num_objects - 1);\n> -\tfor (i = 0; i < m->num_objects - 1; i++) {\n> -\t\tstruct object_id oid1, oid2;\n> +\tif (m->num_objects == 0)\n> +\t\tmidx_report(_(\"Warning: the midx contains no oid.\"));\n\nShould this \"Warning: \" be here? The other calls to midx_report() do not have such prefix.\n\nIt could be valuable to add \"warning: %s\\n\" to the fprintf inside midx_report(), but that should be done as its own patch.\n\nAlso, 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.\n\n> +\telse\n> +\t{\n> +\t\tif (flags & MIDX_PROGRESS)\n> +\t\t\tprogress = start_sparse_progress(_(\"Verifying OID order in multi-pack-index\"),\n> +\t\t\t\t\t\t\t m->num_objects - 1);\n> +\t\tfor (i = 0; i < m->num_objects - 1; i++) {\n> +\t\t\tstruct object_id oid1, oid2;\n> \n> -\t\tnth_midxed_object_oid(&oid1, m, i);\n> -\t\tnth_midxed_object_oid(&oid2, m, i + 1);\n> +\t\t\tnth_midxed_object_oid(&oid1, m, i);\n> +\t\t\tnth_midxed_object_oid(&oid2, m, i + 1);\n> \n> -\t\tif (oidcmp(&oid1, &oid2) >= 0)\n> -\t\t\tmidx_report(_(\"oid lookup out of order: oid[%d] = %s >= %s = oid[%d]\"),\n> -\t\t\t\t i, oid_to_hex(&oid1), oid_to_hex(&oid2), i + 1);\n> +\t\t\tif (oidcmp(&oid1, &oid2) >= 0)\n> +\t\t\t\tmidx_report(_(\"oid lookup out of order: oid[%d] = %s >= %s = oid[%d]\"),\n> +\t\t\t\t\t i, oid_to_hex(&oid1), oid_to_hex(&oid2), i + 1);\n> \n> -\t\tmidx_display_sparse_progress(progress, i + 1);\n> +\t\t\tmidx_display_sparse_progress(progress, i + 1);\n> +\t\t}\n> +\t\tstop_progress(&progress);\n> \t}\n> -\tstop_progress(&progress);\n> \n> \t/*\n> \t * Create an array mapping each object to its packfile id. Sort it\n> \n\nThanks for digging into this!\n\n-Stolee\n"},{"id":"393178","messageId":"20200312214123.sqja3fvuiaaspxwp@doriath","threadId":"52908","inReplyTo":"a538c497-a79d-43be-3d00-c7a619acc4e6@gmail.com","subject":"Re: [PATCH v2 1/1] midx.c: fix an integer overflow","fromName":"Damien Robert","fromEmail":"damien.olivier.robert@gmail.com","sentAt":"2020-03-12T21:41:23Z","receivedAt":"2020-03-12T21:41:30Z","isPatch":true,"sender":{"key":"damien.olivier.robert@gmail.com","avatar":null},"body":"From Derrick Stolee, Thu 12 Mar 2020 at 14:28:11 (-0400) :\n> I _think_ the t/tXXXX directories are used for this kind of data storage,\n> so you could generate an empty multi-pack-index from an older version of\n> Git then store it there.\n\nYes I anticipated that and have one available on hand :)\nIt weights 1116 characters.\n\n> > -\tif (flags & MIDX_PROGRESS)\n> > -\t\tprogress = start_sparse_progress(_(\"Verifying OID order in multi-pack-index\"),\n> > -\t\t\t\t\t\t m->num_objects - 1);\n> > -\tfor (i = 0; i < m->num_objects - 1; i++) {\n> > -\t\tstruct object_id oid1, oid2;\n> > +\tif (m->num_objects == 0)\n> > +\t\tmidx_report(_(\"Warning: the midx contains no oid.\"));\n> \n> Should this \"Warning: \" be here? The other calls to midx_report() do not have such prefix.\n\nRight, I agree it should not.\n\n> 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.\n\nAgreed: we don't want to run the other checks anyway if we don't have any\nobjects.\n\nThat'll be for v3 once I get advice on what to do for tests.\n"},{"id":"393835","messageId":"20200323222515.779477-1-damien.olivier.robert+git@gmail.com","threadId":"52908","inReplyTo":"20200312173520.2401776-1-damien.olivier.robert+git@gmail.com","subject":"[PATCH v3 1/1] midx.c: fix an integer overflow","fromName":"Damien Robert","fromEmail":"damien.olivier.robert@gmail.com","sentAt":"2020-03-23T22:25:15Z","receivedAt":"2020-03-23T22:25:32Z","isPatch":true,"sender":{"key":"damien.olivier.robert@gmail.com","avatar":null},"body":"When verifying a midx index with 0 objects, the\n m->num_objects - 1\noverflows to 4294967295.\n\nFix this both by checking that the midx contains at least one oid,\nand also that we don't write any midx when there is no packfiles.\n\nUpdate the tests so that we check that `git multi-pack-index write` does\nnot write an midx when there is no object.\n\nSigned-off-by: Damien Robert \n---\nSince I did not receive any guidelines, I did not upload an midx with no\nobject to check in the tests. I just modified the current tests to check\nthat we don't produce an midx if there is no objects.\n\n midx.c | 13 +++++++++++++\n t/t5319-multi-pack-index.sh | 7 +++----\n 2 files changed, 16 insertions(+), 4 deletions(-)\n\ndiff --git a/midx.c b/midx.c\nindex 1527e464a7..018acc7e76 100644\n--- a/midx.c\n+++ b/midx.c\n@@ -923,6 +923,12 @@ static int write_midx_internal(const char *object_dir, struct multi_pack_index *\n \tcur_chunk = 0;\n \tnum_chunks = large_offsets_needed ? 5 : 4;\n \n+\tif (packs.nr - dropped_packs == 0) {\n+\t\terror(_(\"no pack files to index.\"));\n+\t\tresult = 1;\n+\t\tgoto cleanup;\n+\t}\n+\n \twritten = write_midx_header(f, num_chunks, packs.nr - dropped_packs);\n \n \tchunk_ids[cur_chunk] = MIDX_CHUNKID_PACKNAMES;\n@@ -1124,6 +1130,13 @@ int verify_midx_file(struct repository *r, const char *object_dir, unsigned flag\n \t\t\t\t i, oid_fanout1, oid_fanout2, i + 1);\n \t}\n \n+\tif (m->num_objects == 0) {\n+\t\tmidx_report(_(\"the midx contains no oid\"));\n+\t\t// remaining tests assume that we have objects, so we can\n+\t\t// return here\n+\t\treturn verify_midx_error;\n+\t}\n+\n \tif (flags & MIDX_PROGRESS)\n \t\tprogress = start_sparse_progress(_(\"Verifying OID order in multi-pack-index\"),\n \t\t\t\t\t\t m->num_objects - 1);\ndiff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh\nindex 43a7a66c9d..d90dfce268 100755\n--- a/t/t5319-multi-pack-index.sh\n+++ b/t/t5319-multi-pack-index.sh\n@@ -42,10 +42,9 @@ test_expect_success 'setup' '\n \tEOF\n '\n \n-test_expect_success 'write midx with no packs' '\n-\ttest_when_finished rm -f pack/multi-pack-index &&\n-\tgit multi-pack-index --object-dir=. write &&\n-\tmidx_read_expect 0 0 4 .\n+test_expect_success \"don't write midx with no packs\" '\n+\ttest_must_fail git multi-pack-index --object-dir=. write &&\n+\ttest_path_is_missing pack/multi-pack-index\n '\n \n generate_objects () {\n-- \nPatched on top of v2.26.0 (git version 2.25.1)\n\n"},{"id":"393854","messageId":"20200324060125.GA610977@coredump.intra.peff.net","threadId":"52908","inReplyTo":"20200323222515.779477-1-damien.olivier.robert+git@gmail.com","subject":"Re: [PATCH v3 1/1] midx.c: fix an integer overflow","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-03-24T06:01:25Z","receivedAt":"2020-03-24T06:01:29Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Mar 23, 2020 at 11:25:15PM +0100, Damien Robert wrote:\n\n> When verifying a midx index with 0 objects, the\n> m->num_objects - 1\n> overflows to 4294967295.\n> \n> Fix this both by checking that the midx contains at least one oid,\n> and also that we don't write any midx when there is no packfiles.\n> \n> Update the tests so that we check that `git multi-pack-index write` does\n> not write an midx when there is no object.\n\nThanks, both sides of this make sense.\n\n> ---\n> Since I did not receive any guidelines, I did not upload an midx with no\n> object to check in the tests. I just modified the current tests to check\n> that we don't produce an midx if there is no objects.\n\nI'd be OK with just this, but adding a binary t/t5319/zero-objs.midx\nwould be fine by me, too.\n\nOne minor style nit:\n\n> @@ -1124,6 +1130,13 @@ int verify_midx_file(struct repository *r, const char *object_dir, unsigned flag\n> \t\t\t\t i, oid_fanout1, oid_fanout2, i + 1);\n> \t}\n> \n> +\tif (m->num_objects == 0) {\n> +\t\tmidx_report(_(\"the midx contains no oid\"));\n> +\t\t// remaining tests assume that we have objects, so we can\n> +\t\t// return here\n> +\t\treturn verify_midx_error;\n> +\t}\n\nWe prefer /**/ for comments, like:\n\n /*\n * Remaining tests assume that we have objects, so we can\n * return here.\n */\n\n-Peff\n"},{"id":"393886","messageId":"xmqqtv2d1td4.fsf@gitster.c.googlers.com","threadId":"52908","inReplyTo":"20200324060125.GA610977@coredump.intra.peff.net","subject":"Re: [PATCH v3 1/1] midx.c: fix an integer overflow","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-03-24T18:48:55Z","receivedAt":"2020-03-24T18:48:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> I'd be OK with just this, but adding a binary t/t5319/zero-objs.midx\n> would be fine by me, too.\n\nYup, that sounds like a simple way to make sure we won't regress.\n\n> One minor style nit:\n>\n>> @@ -1124,6 +1130,13 @@ int verify_midx_file(struct repository *r, const char *object_dir, unsigned flag\n>> \t\t\t\t i, oid_fanout1, oid_fanout2, i + 1);\n>> \t}\n>> \n>> +\tif (m->num_objects == 0) {\n>> +\t\tmidx_report(_(\"the midx contains no oid\"));\n>> +\t\t// remaining tests assume that we have objects, so we can\n>> +\t\t// return here\n>> +\t\treturn verify_midx_error;\n>> +\t}\n>\n> We prefer /**/ for comments, like:\n>\n> /*\n> * Remaining tests assume that we have objects, so we can\n> * return here.\n> */\n\nThanks for catching it.\n\n"},{"id":"394123","messageId":"20200326213534.399377-1-damien.olivier.robert+git@gmail.com","threadId":"52908","inReplyTo":"20200312173520.2401776-1-damien.olivier.robert+git@gmail.com","subject":"[PATCH v4 1/1] midx.c: fix an integer overflow","fromName":"Damien Robert","fromEmail":"damien.olivier.robert@gmail.com","sentAt":"2020-03-26T21:35:34Z","receivedAt":"2020-03-26T21:36:08Z","isPatch":true,"sender":{"key":"damien.olivier.robert@gmail.com","avatar":null},"body":"When verifying a midx index with 0 objects, the\n m->num_objects - 1\noverflows to 4294967295.\n\nFix this both by checking that the midx contains at least one oid,\nand also that we don't write any midx when there is no packfiles.\n\nUpdate the tests to check that `git multi-pack-index write` does\nnot write an midx when there is no objects, and another to check\nthat `git multi-pack-index verify` warns when it verifies an midx with no\nobjects. For this last test, use t5319/no-objects.midx which was\ngenerated by an older version of git.\n\nSigned-off-by: Damien Robert \n---\nFollowing the recommandations I uploaded an empty midx to check we don't\nregress.\n\n midx.c | 15 +++++++++++++++\n t/t5319-multi-pack-index.sh | 13 +++++++++----\n t/t5319/no-objects.midx | Bin 0 -> 1116 bytes\n 3 files changed, 24 insertions(+), 4 deletions(-)\n create mode 100644 t/t5319/no-objects.midx\n\ndiff --git a/midx.c b/midx.c\nindex 1527e464a7..a520e26395 100644\n--- a/midx.c\n+++ b/midx.c\n@@ -923,6 +923,12 @@ static int write_midx_internal(const char *object_dir, struct multi_pack_index *\n \tcur_chunk = 0;\n \tnum_chunks = large_offsets_needed ? 5 : 4;\n \n+\tif (packs.nr - dropped_packs == 0) {\n+\t\terror(_(\"no pack files to index.\"));\n+\t\tresult = 1;\n+\t\tgoto cleanup;\n+\t}\n+\n \twritten = write_midx_header(f, num_chunks, packs.nr - dropped_packs);\n \n \tchunk_ids[cur_chunk] = MIDX_CHUNKID_PACKNAMES;\n@@ -1124,6 +1130,15 @@ int verify_midx_file(struct repository *r, const char *object_dir, unsigned flag\n \t\t\t\t i, oid_fanout1, oid_fanout2, i + 1);\n \t}\n \n+\tif (m->num_objects == 0) {\n+\t\tmidx_report(_(\"the midx contains no oid\"));\n+\t\t/*\n+\t\t * Remaining tests assume that we have objects, so we can\n+\t\t * return here.\n+\t\t */\n+\t\treturn verify_midx_error;\n+\t}\n+\n \tif (flags & MIDX_PROGRESS)\n \t\tprogress = start_sparse_progress(_(\"Verifying OID order in multi-pack-index\"),\n \t\t\t\t\t\t m->num_objects - 1);\ndiff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh\nindex 43a7a66c9d..10c35d445d 100755\n--- a/t/t5319-multi-pack-index.sh\n+++ b/t/t5319-multi-pack-index.sh\n@@ -42,10 +42,15 @@ test_expect_success 'setup' '\n \tEOF\n '\n \n-test_expect_success 'write midx with no packs' '\n-\ttest_when_finished rm -f pack/multi-pack-index &&\n-\tgit multi-pack-index --object-dir=. write &&\n-\tmidx_read_expect 0 0 4 .\n+test_expect_success \"don't write midx with no packs\" '\n+\ttest_must_fail git multi-pack-index --object-dir=. write &&\n+\ttest_path_is_missing pack/multi-pack-index\n+'\n+\n+test_expect_success \"Warn if a midx contains no oid\" '\n+\tcp \"$TEST_DIRECTORY\"/t5319/no-objects.midx .git/objects/pack/multi-pack-index &&\n+\ttest_must_fail git multi-pack-index verify &&\n+\trm .git/objects/pack/multi-pack-index\n '\n \n generate_objects () {\ndiff --git a/t/t5319/no-objects.midx b/t/t5319/no-objects.midx\nnew file mode 100644\nindex 0000000000000000000000000000000000000000..e466b8e08654c29effb5248cb109d81cfbcfd2f4\nGIT binary patch\nliteral 1116\nzcmebEbctYOWMKe-06#}xFoS`?!{5`z4T writes:\n\n> When verifying a midx index with 0 objects, the\n> m->num_objects - 1\n> overflows to 4294967295.\n\nI think this is underflow & wraparound, but I'll let it pass ;-)\n\nThe patch looks good; will queue.\n\nThanks.\n"},{"id":"394252","messageId":"20200328221822.228469-1-damien.olivier.robert+git@gmail.com","threadId":"52908","inReplyTo":"20200312173520.2401776-1-damien.olivier.robert+git@gmail.com","subject":"[PATCH 1/1] midx.c: fix an integer underflow","fromName":"Damien Robert","fromEmail":"damien.olivier.robert@gmail.com","sentAt":"2020-03-28T22:18:22Z","receivedAt":"2020-03-28T22:18:36Z","isPatch":true,"sender":{"key":"damien.olivier.robert@gmail.com","avatar":null},"body":"When verifying a midx index with 0 objects, the\n m->num_objects - 1\nunderflows and wraps around to 4294967295.\n\nFix this both by checking that the midx contains at least one oid,\nand also that we don't write any midx when there is no packfiles.\n\nUpdate the tests to check that `git multi-pack-index write` does\nnot write an midx when there is no objects, and another to check\nthat `git multi-pack-index verify` warns when it verifies an midx with no\nobjects. For this last test, use t5319/no-objects.midx which was\ngenerated by an older version of git.\n\nSigned-off-by: Damien Robert \n---\nChange since v4: use \"$objdir\" in tests rather than \".git/objects/\"\nUse this opportunity to use the correct technical term in the commit\nmessage, as pointed out by Junio.\n\n midx.c | 15 +++++++++++++++\n t/t5319-multi-pack-index.sh | 13 +++++++++----\n t/t5319/no-objects.midx | Bin 0 -> 1116 bytes\n 3 files changed, 24 insertions(+), 4 deletions(-)\n create mode 100644 t/t5319/no-objects.midx\n\ndiff --git a/midx.c b/midx.c\nindex 1527e464a7..a520e26395 100644\n--- a/midx.c\n+++ b/midx.c\n@@ -923,6 +923,12 @@ static int write_midx_internal(const char *object_dir, struct multi_pack_index *\n \tcur_chunk = 0;\n \tnum_chunks = large_offsets_needed ? 5 : 4;\n \n+\tif (packs.nr - dropped_packs == 0) {\n+\t\terror(_(\"no pack files to index.\"));\n+\t\tresult = 1;\n+\t\tgoto cleanup;\n+\t}\n+\n \twritten = write_midx_header(f, num_chunks, packs.nr - dropped_packs);\n \n \tchunk_ids[cur_chunk] = MIDX_CHUNKID_PACKNAMES;\n@@ -1124,6 +1130,15 @@ int verify_midx_file(struct repository *r, const char *object_dir, unsigned flag\n \t\t\t\t i, oid_fanout1, oid_fanout2, i + 1);\n \t}\n \n+\tif (m->num_objects == 0) {\n+\t\tmidx_report(_(\"the midx contains no oid\"));\n+\t\t/*\n+\t\t * Remaining tests assume that we have objects, so we can\n+\t\t * return here.\n+\t\t */\n+\t\treturn verify_midx_error;\n+\t}\n+\n \tif (flags & MIDX_PROGRESS)\n \t\tprogress = start_sparse_progress(_(\"Verifying OID order in multi-pack-index\"),\n \t\t\t\t\t\t m->num_objects - 1);\ndiff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh\nindex 43a7a66c9d..22240fd30b 100755\n--- a/t/t5319-multi-pack-index.sh\n+++ b/t/t5319-multi-pack-index.sh\n@@ -42,10 +42,15 @@ test_expect_success 'setup' '\n \tEOF\n '\n \n-test_expect_success 'write midx with no packs' '\n-\ttest_when_finished rm -f pack/multi-pack-index &&\n-\tgit multi-pack-index --object-dir=. write &&\n-\tmidx_read_expect 0 0 4 .\n+test_expect_success \"don't write midx with no packs\" '\n+\ttest_must_fail git multi-pack-index --object-dir=. write &&\n+\ttest_path_is_missing pack/multi-pack-index\n+'\n+\n+test_expect_success \"Warn if a midx contains no oid\" '\n+\tcp \"$TEST_DIRECTORY\"/t5319/no-objects.midx $objdir/pack/multi-pack-index &&\n+\ttest_must_fail git multi-pack-index verify &&\n+\trm $objdir/pack/multi-pack-index\n '\n \n generate_objects () {\ndiff --git a/t/t5319/no-objects.midx b/t/t5319/no-objects.midx\nnew file mode 100644\nindex 0000000000000000000000000000000000000000..e466b8e08654c29effb5248cb109d81cfbcfd2f4\nGIT binary patch\nliteral 1116\nzcmebEbctYOWMKe-06#}xFoS`?!{5`z4T I think this is underflow & wraparound, but I'll let it pass ;-)\n\nAh thanks for the 'wraparound' term, I was missing it, so that's why I\nwrote overflow instead since everybody understand what I meant.\n\nMathematically I think of this as a truncating in the ring of 2-adic integers,\nbut I did not want to write this in the commit message :)\n\n> The patch looks good; will queue.\n\nThanks. I just sent a v5 with a microfix (which is not really important, so\nv5 can be dropped). Since pu is subject to be rewound anyway, I guess you\nprefer a new version rather than a patch on top of v4?\n\n-- \nDamien Robert\nhttp://www.normalesup.org/~robert/pro\n"},{"id":"394256","messageId":"xmqqlfnkca22.fsf@gitster.c.googlers.com","threadId":"52908","inReplyTo":"20200328222342.jfxsk3gvujgqdt4f@doriath","subject":"Re: [PATCH v4 1/1] midx.c: fix an integer overflow","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-03-28T23:51:49Z","receivedAt":"2020-03-28T23:51:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Damien Robert writes:\n\n> Mathematically I think of this as a truncating in the ring of 2-adic integers,\n> but I did not want to write this in the commit message :)\n\n;-)\n\n>> The patch looks good; will queue.\n>\n> Thanks. I just sent a v5 with a microfix (which is not really important, so\n> v5 can be dropped). Since pu is subject to be rewound anyway, I guess you\n> prefer a new version rather than a patch on top of v4?\n\nOK, will replace. Thanks for your attention to the details.\n"}]}