git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / Atom feed
* [PATCH 1/1] midx.c: fix an integer overflow
@ 2020-02-28 16:24 Damien Robert
  2020-02-28 18:55 ` Jeff King
  2020-03-12 17:35 ` [PATCH v2 " Damien Robert
  0 siblings, 2 replies; 17+ messages in thread
From: Damien Robert @ 2020-02-28 16:24 UTC (permalink / raw)
  To: git; +Cc: Damien Robert, Junio C Hamano, Derrick Stolee, William Baker

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)


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 1/1] midx.c: fix an integer overflow
  2020-02-28 16:24 [PATCH 1/1] midx.c: fix an integer overflow Damien Robert
@ 2020-02-28 18:55 ` Jeff King
  2020-02-28 20:39   ` Junio C Hamano
  2020-02-29 15:38   ` Damien Robert
  2020-03-12 17:35 ` [PATCH v2 " Damien Robert
  1 sibling, 2 replies; 17+ messages in thread
From: Jeff King @ 2020-02-28 18:55 UTC (permalink / raw)
  To: Damien Robert
  Cc: git, Damien Robert, Junio C Hamano, Derrick Stolee, William Baker

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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 1/1] midx.c: fix an integer overflow
  2020-02-28 18:55 ` Jeff King
@ 2020-02-28 20:39   ` Junio C Hamano
  2020-02-29 17:15     ` Damien Robert
  2020-02-29 15:38   ` Damien Robert
  1 sibling, 1 reply; 17+ messages in thread
From: Junio C Hamano @ 2020-02-28 20:39 UTC (permalink / raw)
  To: Jeff King
  Cc: Damien Robert, git, Damien Robert, Derrick Stolee, William Baker

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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 1/1] midx.c: fix an integer overflow
  2020-02-28 18:55 ` Jeff King
  2020-02-28 20:39   ` Junio C Hamano
@ 2020-02-29 15:38   ` Damien Robert
  1 sibling, 0 replies; 17+ messages in thread
From: Damien Robert @ 2020-02-29 15:38 UTC (permalink / raw)
  To: Jeff King; +Cc: git, Junio C Hamano, Derrick Stolee, William Baker

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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH 1/1] midx.c: fix an integer overflow
  2020-02-28 20:39   ` Junio C Hamano
@ 2020-02-29 17:15     ` Damien Robert
  0 siblings, 0 replies; 17+ messages in thread
From: Damien Robert @ 2020-02-29 17:15 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Jeff King, git, Derrick Stolee, William Baker

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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* [PATCH v2 1/1] midx.c: fix an integer overflow
  2020-02-28 16:24 [PATCH 1/1] midx.c: fix an integer overflow Damien Robert
  2020-02-28 18:55 ` Jeff King
@ 2020-03-12 17:35 ` " Damien Robert
  2020-03-12 18:24   ` Damien Robert
                     ` (4 more replies)
  1 sibling, 5 replies; 17+ messages in thread
From: Damien Robert @ 2020-03-12 17:35 UTC (permalink / raw)
  To: Jeff King, git, Junio C Hamano, Derrick Stolee, William Baker
  Cc: 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)


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v2 1/1] midx.c: fix an integer overflow
  2020-03-12 17:35 ` [PATCH v2 " Damien Robert
@ 2020-03-12 18:24   ` Damien Robert
  2020-03-12 18:28   ` Derrick Stolee
                     ` (3 subsequent siblings)
  4 siblings, 0 replies; 17+ messages in thread
From: Damien Robert @ 2020-03-12 18:24 UTC (permalink / raw)
  To: Jeff King, git, Junio C Hamano, Derrick Stolee, William Baker

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.

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v2 1/1] midx.c: fix an integer overflow
  2020-03-12 17:35 ` [PATCH v2 " Damien Robert
  2020-03-12 18:24   ` Damien Robert
@ 2020-03-12 18:28   ` Derrick Stolee
  2020-03-12 21:41     ` Damien Robert
  2020-03-23 22:25   ` [PATCH v3 " Damien Robert
                     ` (2 subsequent siblings)
  4 siblings, 1 reply; 17+ messages in thread
From: Derrick Stolee @ 2020-03-12 18:28 UTC (permalink / raw)
  To: Damien Robert, Jeff King, git, Junio C Hamano, Derrick Stolee,
	William Baker
  Cc: Damien Robert

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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v2 1/1] midx.c: fix an integer overflow
  2020-03-12 18:28   ` Derrick Stolee
@ 2020-03-12 21:41     ` Damien Robert
  0 siblings, 0 replies; 17+ messages in thread
From: Damien Robert @ 2020-03-12 21:41 UTC (permalink / raw)
  To: Derrick Stolee
  Cc: Jeff King, git, Junio C Hamano, Derrick Stolee, William Baker

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.

^ permalink raw reply	[flat|nested] 17+ messages in thread

* [PATCH v3 1/1] midx.c: fix an integer overflow
  2020-03-12 17:35 ` [PATCH v2 " Damien Robert
  2020-03-12 18:24   ` Damien Robert
  2020-03-12 18:28   ` Derrick Stolee
@ 2020-03-23 22:25   ` " Damien Robert
  2020-03-24  6:01     ` Jeff King
  2020-03-26 21:35   ` [PATCH v4 " Damien Robert
  2020-03-28 22:18   ` [PATCH 1/1] midx.c: fix an integer underflow Damien Robert
  4 siblings, 1 reply; 17+ messages in thread
From: Damien Robert @ 2020-03-23 22:25 UTC (permalink / raw)
  To: Jeff King, git, Junio C Hamano, Derrick Stolee, William Baker
  Cc: 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)


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v3 1/1] midx.c: fix an integer overflow
  2020-03-23 22:25   ` [PATCH v3 " Damien Robert
@ 2020-03-24  6:01     ` Jeff King
  2020-03-24 18:48       ` Junio C Hamano
  0 siblings, 1 reply; 17+ messages in thread
From: Jeff King @ 2020-03-24  6:01 UTC (permalink / raw)
  To: Damien Robert
  Cc: git, Junio C Hamano, Derrick Stolee, William Baker, Damien Robert

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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v3 1/1] midx.c: fix an integer overflow
  2020-03-24  6:01     ` Jeff King
@ 2020-03-24 18:48       ` Junio C Hamano
  0 siblings, 0 replies; 17+ messages in thread
From: Junio C Hamano @ 2020-03-24 18:48 UTC (permalink / raw)
  To: Jeff King
  Cc: Damien Robert, git, Derrick Stolee, William Baker, Damien Robert

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.


^ permalink raw reply	[flat|nested] 17+ messages in thread

* [PATCH v4 1/1] midx.c: fix an integer overflow
  2020-03-12 17:35 ` [PATCH v2 " Damien Robert
                     ` (2 preceding siblings ...)
  2020-03-23 22:25   ` [PATCH v3 " Damien Robert
@ 2020-03-26 21:35   ` " Damien Robert
  2020-03-26 23:27     ` Junio C Hamano
  2020-03-28 22:18   ` [PATCH 1/1] midx.c: fix an integer underflow Damien Robert
  4 siblings, 1 reply; 17+ messages in thread
From: Damien Robert @ 2020-03-26 21:35 UTC (permalink / raw)
  To: Jeff King, git, Junio C Hamano, Derrick Stolee, William Baker
  Cc: 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)


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v4 1/1] midx.c: fix an integer overflow
  2020-03-26 21:35   ` [PATCH v4 " Damien Robert
@ 2020-03-26 23:27     ` Junio C Hamano
  2020-03-28 22:23       ` Damien Robert
  0 siblings, 1 reply; 17+ messages in thread
From: Junio C Hamano @ 2020-03-26 23:27 UTC (permalink / raw)
  To: Damien Robert
  Cc: Jeff King, git, Derrick Stolee, William Baker, Damien Robert

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.

^ permalink raw reply	[flat|nested] 17+ messages in thread

* [PATCH 1/1] midx.c: fix an integer underflow
  2020-03-12 17:35 ` [PATCH v2 " Damien Robert
                     ` (3 preceding siblings ...)
  2020-03-26 21:35   ` [PATCH v4 " Damien Robert
@ 2020-03-28 22:18   ` Damien Robert
  4 siblings, 0 replies; 17+ messages in thread
From: Damien Robert @ 2020-03-28 22:18 UTC (permalink / raw)
  To: Jeff King, git, Junio C Hamano, Derrick Stolee, William Baker
  Cc: 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)


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v4 1/1] midx.c: fix an integer overflow
  2020-03-26 23:27     ` Junio C Hamano
@ 2020-03-28 22:23       ` Damien Robert
  2020-03-28 23:51         ` Junio C Hamano
  0 siblings, 1 reply; 17+ messages in thread
From: Damien Robert @ 2020-03-28 22:23 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Jeff King, git, Derrick Stolee, William Baker

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

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH v4 1/1] midx.c: fix an integer overflow
  2020-03-28 22:23       ` Damien Robert
@ 2020-03-28 23:51         ` Junio C Hamano
  0 siblings, 0 replies; 17+ messages in thread
From: Junio C Hamano @ 2020-03-28 23:51 UTC (permalink / raw)
  To: Damien Robert; +Cc: Jeff King, git, Derrick Stolee, William Baker

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.

^ permalink raw reply	[flat|nested] 17+ messages in thread

end of thread, back to index

Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-02-28 16:24 [PATCH 1/1] midx.c: fix an integer overflow Damien Robert
2020-02-28 18:55 ` Jeff King
2020-02-28 20:39   ` Junio C Hamano
2020-02-29 17:15     ` Damien Robert
2020-02-29 15:38   ` Damien Robert
2020-03-12 17:35 ` [PATCH v2 " Damien Robert
2020-03-12 18:24   ` Damien Robert
2020-03-12 18:28   ` Derrick Stolee
2020-03-12 21:41     ` Damien Robert
2020-03-23 22:25   ` [PATCH v3 " Damien Robert
2020-03-24  6:01     ` Jeff King
2020-03-24 18:48       ` Junio C Hamano
2020-03-26 21:35   ` [PATCH v4 " Damien Robert
2020-03-26 23:27     ` Junio C Hamano
2020-03-28 22:23       ` Damien Robert
2020-03-28 23:51         ` Junio C Hamano
2020-03-28 22:18   ` [PATCH 1/1] midx.c: fix an integer underflow Damien Robert

git@vger.kernel.org list mirror (unofficial, one of many)

Archives are clonable:
	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

Example config snippet for mirrors

Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.io/gmane.comp.version-control.git

 note: .onion URLs require Tor: https://www.torproject.org/

AGPL code for this site: git clone https://public-inbox.org/public-inbox.git