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

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

[Damien Robert]

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(-)

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)

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

[Jeff King]

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

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

[Junio C Hamano]

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 ;-)

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

[Damien Robert]

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

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

[Damien Robert]

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

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

[Damien Robert]

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 <damien.olivier.robert+git@gmail.com>
---
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)

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

[Damien Robert]

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.

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

[Derrick Stolee]

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 <damien.olivier.robert+git@gmail.com>
> ---
> 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

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

[Damien Robert]

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.

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

[Damien Robert]

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 <damien.olivier.robert+git@gmail.com>
---
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)

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

[Jeff King]

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

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

[Junio C Hamano]

Jeff King <peff@peff.net> 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.

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

[Damien Robert]

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 <damien.olivier.robert+git@gmail.com>
---
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<doVY7Jn`@2EKSv;WfKnj_S5FKTWhQMeD
djEoT2!pSZW+;QcELdu7jD{8h)6W8x`1prin5MuxU

literal 0
HcmV?d00001

-- 
Patched on top of v2.26.0-51-ga7d14a4428 (git version 2.25.2)

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

[Junio C Hamano]

Damien Robert <damien.olivier.robert@gmail.com> 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.

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

[Damien Robert]

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 <damien.olivier.robert+git@gmail.com>
---
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<doVY7Jn`@2EKSv;WfKnj_S5FKTWhQMeD
djEoT2!pSZW+;QcELdu7jD{8h)6W8x`1prin5MuxU

literal 0
HcmV?d00001

-- 
Patched on top of v2.26.0-103-g3bab5d5625 (git version 2.26.0)

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

[Damien Robert]

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

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

[Junio C Hamano]

Damien Robert <damien.olivier.robert@gmail.com> 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.