Re: [PATCH v2 1/8] midx: fix segfault with no packs and invalid preferred pack

From: Taylor Blau <me@ttaylorr.com>
Date: Apr 12, 2023, 17:56 UTC
Message-ID: <ZDbw6v8r89zciE3q@nand.local>
In-Reply-To: <5ecad306b42441fa7d4f50bdfb9c09ccce22b6c9.1681294715.git.ps@pks.im>

On Wed, Apr 12, 2023 at 12:22:31PM +0200, Patrick Steinhardt wrote:

> Fix this bug by exiting early in case we have determined that the MIDX
> wouldn't have any packfiles to index. While the request itself does not
> make much sense anyway, it is still preferable to exit gracefully than
> to abort.

Interesting. This reminded me quite a bit of eb57277ba3 (midx: prevent writing a .bitmap without any objects, 2022-02-09) which tackled a similar problem of trying to write a MIDX bitmap without any objects.

We may want to consider moving that conditional further up, since this makes the conditional added in eb57277ba3 dead code AFAICT. Here's a patch on top of this one that I think would do the trick.

It has the added benefit of sticking a:

    warning: unknown preferred pack: 'does-not-exist'

in the output before dying, which might be nice (though I doubt anybody will ever see it ;-)). The main difference is that we unset the bitmap related bits from `flags`, which avoids us trying to compute a preferred pack in the first place.

For it to work, though, we need to make sure that ctx.preferred_pack_idx is set to -1, and not zero-initialized, since we'll segfault otherwise when trying to read into an empty array.

--- 8< ---

diff --git a/midx.c b/midx.c
index 22ea7ffb75..dc4821eab8 100644
--- a/midx.c
+++ b/midx.c
@@ -1263,6 +1263,7 @@ static int write_midx_internal(const char *object_dir,
 	ctx.nr = 0;
 	ctx.alloc = ctx.m ? ctx.m->num_packs : 16;
 	ctx.info = NULL;
+	ctx.preferred_pack_idx = -1;
 	ALLOC_ARRAY(ctx.info, ctx.alloc);

 	if (ctx.m) {
@@ -1307,10 +1308,10 @@ static int write_midx_internal(const char *object_dir,
 	for_each_file_in_pack_dir(object_dir, add_pack_to_midx, &ctx);
 	stop_progress(&ctx.progress);

-	if (!ctx.nr) {
-		error(_("no pack files to index."));
-		result = 1;
-		goto cleanup;
+	if (!ctx.entries_nr) {
+		if (flags & MIDX_WRITE_BITMAP)
+			warning(_("refusing to write multi-pack .bitmap without any objects"));
+		flags &= ~(MIDX_WRITE_REV_INDEX | MIDX_WRITE_BITMAP);
 	}

 	if ((ctx.m && ctx.nr == ctx.m->num_packs) &&
@@ -1488,12 +1489,6 @@ static int write_midx_internal(const char *object_dir,
 		goto cleanup;
 	}

-	if (!ctx.entries_nr) {
-		if (flags & MIDX_WRITE_BITMAP)
-			warning(_("refusing to write multi-pack .bitmap without any objects"));
-		flags &= ~(MIDX_WRITE_REV_INDEX | MIDX_WRITE_BITMAP);
-	}
-
 	cf = init_chunkfile(f);

 	add_chunk(cf, MIDX_CHUNKID_PACKNAMES, pack_name_concat_len,
diff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh
index be7f3c1e1f..8a17361272 100755
--- a/t/t5319-multi-pack-index.sh
+++ b/t/t5319-multi-pack-index.sh
@@ -188,10 +188,9 @@ test_expect_success 'write with no objects and preferred pack' '
 	git init empty &&
 	test_must_fail git -C empty multi-pack-index write \
 		--stdin-packs --preferred-pack=does-not-exist </dev/null 2>err &&
-	cat >expect <<-EOF &&
-	error: no pack files to index.
-	EOF
-	test_cmp expect err
+		cat err &&
+	grep "warning: unknown preferred pack: .does-not-exist." err &&
+	grep "error: no pack files to index." err
 '

 test_expect_success 'write progress off for redirected stderr' '
--- >8 ---

Thanks,
Taylor