diff mbox series

[v3,01/10] midx: fix segfault with no packs and invalid preferred pack

Message ID dd8145beade440e5444130d3a3189b2c5d15a911.1681384405.git.ps@pks.im (mailing list archive)
State Accepted
Commit ceb96a160b05c3ec45c416851bac00b711418839
Headers show
Series repack: fix geometric repacking with gitalternates | expand

Commit Message

Patrick Steinhardt April 13, 2023, 11:16 a.m. UTC
When asked to write a multi-pack-index the user can specify a preferred
pack that is used as a tie breaker when multiple packs contain the same
objects. When this packfile cannot be found, we just pick the first pack
that is getting tracked by the newly written multi-pack-index as a
fallback.

Picking the fallback can fail in the case where we're asked to write a
multi-pack-index with no packfiles at all: picking the fallback value
will cause a segfault as we blindly index into the array of packfiles,
which would be empty.

Fix this bug by resetting the preferred packfile index to `-1` before
searching for the preferred pack. This fixes the segfault as we already
check for whether the index is `> - 1`. If it is not, we simply don't
pick a preferred packfile at all.

Helped-by: Taylor Blau <me@ttaylorr.com>
Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 midx.c                      |  6 +++---
 t/t5319-multi-pack-index.sh | 12 ++++++++++++
 2 files changed, 15 insertions(+), 3 deletions(-)

Comments

Derrick Stolee April 13, 2023, 1:49 p.m. UTC | #1
On 4/13/2023 7:16 AM, Patrick Steinhardt wrote:

> Fix this bug by resetting the preferred packfile index to `-1` before
> searching for the preferred pack. This fixes the segfault as we already
> check for whether the index is `> - 1`. If it is not, we simply don't
> pick a preferred packfile at all.

>  	if (preferred_pack_name) {
> -		int found = 0;
> +		ctx.preferred_pack_idx = -1;
> +

This patch looks good, but I did need to think about it for a bit,
so here are my thoughts (feel free to ignore):

I briefly considered recommending that we set this as the default in
an initializer macro, something like

  #define WRITE_MIDX_CONTEXT_INIT { .preferred_pack_idx = -1 }

but this struct is internal to the file and only constructed once
(at the start of write_midx_internal).

Further, outside the context of this patch we have this code:

	} else if (ctx.nr &&
		   (flags & (MIDX_WRITE_REV_INDEX | MIDX_WRITE_BITMAP))) {
		struct packed_git *oldest = ctx.info[ctx.preferred_pack_idx].p;
		ctx.preferred_pack_idx = 0;

I don't see a way that ctx.preferred_pack_idx can be anything but zero
here, but it's also not going to give a segfault because of the 'ctx.nr'
condition.

Thanks,
-Stolee
diff mbox series

Patch

diff --git a/midx.c b/midx.c
index 47989f7ea7..67eb617591 100644
--- a/midx.c
+++ b/midx.c
@@ -1328,17 +1328,17 @@  static int write_midx_internal(const char *object_dir,
 	}
 
 	if (preferred_pack_name) {
-		int found = 0;
+		ctx.preferred_pack_idx = -1;
+
 		for (i = 0; i < ctx.nr; i++) {
 			if (!cmp_idx_or_pack_name(preferred_pack_name,
 						  ctx.info[i].pack_name)) {
 				ctx.preferred_pack_idx = i;
-				found = 1;
 				break;
 			}
 		}
 
-		if (!found)
+		if (ctx.preferred_pack_idx == -1)
 			warning(_("unknown preferred pack: '%s'"),
 				preferred_pack_name);
 	} else if (ctx.nr &&
diff --git a/t/t5319-multi-pack-index.sh b/t/t5319-multi-pack-index.sh
index 499d5d4c78..0883c7c6bd 100755
--- a/t/t5319-multi-pack-index.sh
+++ b/t/t5319-multi-pack-index.sh
@@ -183,6 +183,18 @@  test_expect_success 'write midx with --stdin-packs' '
 
 compare_results_with_midx "mixed mode (one pack + extra)"
 
+test_expect_success 'write with no objects and preferred pack' '
+	test_when_finished "rm -rf empty" &&
+	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 &&
+	warning: unknown preferred pack: ${SQ}does-not-exist${SQ}
+	error: no pack files to index.
+	EOF
+	test_cmp expect err
+'
+
 test_expect_success 'write progress off for redirected stderr' '
 	git multi-pack-index --object-dir=$objdir write 2>err &&
 	test_line_count = 0 err