midx-write: only load initialized packs

The fill_packs_from_midx() method was refactored in fcb2205 (midx:
implement support for writing incremental MIDX chains, 2024-08-06) to
allow for preferred packfiles and incremental multi-pack-indexes.
However, this led to some conditions that can cause improperly
initialized memory in the context's list of packfiles.

The conditions caring about the preferred pack name or the incremental
flag are currently necessary to load a packfile. But the context is
still being populated with pack_info structs based on the packfile array
for the existing multi-pack-index even if prepare_midx_pack() isn't
called.

I'm not sure how to reproduce this situation in a test case (and it
would be great if I could) but some calls to the 'repack' or 'expire'
subcommand of 'git multi-pack-index' could segfault later due to these
improperly initialized pack_info structs. The current change does fix
the large, real-world case that I had observed with a debugger.

It is worth noting that there are test cases around the 'expire'
subcommand that will fail if we attempt to open the pack index during
this loop, or if we ignore packs that fail to open the pack index in
this loop. The removal of the calls to open_pack_index() reverts back to
the condition before this was refactored. The only instance where this
mattered was a check to see if the preferred packfile had objects or
not, so the initialization of the preferred pack was deferred to that
place.

To the best of my knowledge, the incremental feature does not require
the pack-indexes to be opened during this process.

Signed-off-by: Derrick Stolee <[email protected]>

Author: derrickstolee

midx-write.c

@@ -945,39 +945,21 @@ static struct multi_pack_index *lookup_multi_pack_index(struct repository *r,
 	return result;
 }
 
-static int fill_packs_from_midx(struct write_midx_context *ctx,
-				const char *preferred_pack_name, uint32_t flags)
+static int fill_packs_from_midx(struct write_midx_context *ctx)
 {
 	struct multi_pack_index *m;
 
 	for (m = ctx->m; m; m = m->base_midx) {
 		uint32_t i;
 
 		for (i = 0; i < m->num_packs; i++) {
-			ALLOC_GROW(ctx->info, ctx->nr + 1, ctx->alloc);
-
-			/*
-			 * If generating a reverse index, need to have
-			 * packed_git's loaded to compare their
-			 * mtimes and object count.
-			 *
-			 * If a preferred pack is specified, need to
-			 * have packed_git's loaded to ensure the chosen
-			 * preferred pack has a non-zero object count.
-			 */
-			if (flags & MIDX_WRITE_REV_INDEX ||
-			    preferred_pack_name) {
-				if (prepare_midx_pack(ctx->repo, m,
-						      m->num_packs_in_base + i)) {
-					error(_("could not load pack"));
-					return -1;
-				}
-
-				if (open_pack_index(m->packs[i]))
-					die(_("could not open index for %s"),
-					    m->packs[i]->pack_name);
+			if (prepare_midx_pack(ctx->repo, m,
+					      m->num_packs_in_base + i)) {
+				error(_("could not load pack"));
+				return -1;
 			}
 
+			ALLOC_GROW(ctx->info, ctx->nr + 1, ctx->alloc);
 			fill_pack_info(&ctx->info[ctx->nr++], m->packs[i],
 				       m->pack_names[i],
 				       m->num_packs_in_base + i);
@@ -1149,8 +1131,7 @@ static int write_midx_internal(struct repository *r, const char *object_dir,
 			ctx.num_multi_pack_indexes_before++;
 			m = m->base_midx;
 		}
-	} else if (ctx.m && fill_packs_from_midx(&ctx, preferred_pack_name,
-						 flags) < 0) {
+	} else if (ctx.m && fill_packs_from_midx(&ctx) < 0) {
 		result = 1;
 		goto cleanup;
 	}
@@ -1251,7 +1232,7 @@ static int write_midx_internal(struct repository *r, const char *object_dir,
 
 	if (ctx.preferred_pack_idx != NO_PREFERRED_PACK) {
 		struct packed_git *preferred = ctx.info[ctx.preferred_pack_idx].p;
-		if (!preferred->num_objects) {
+		if (open_pack_index(preferred) || !preferred->num_objects) {
 			error(_("cannot select preferred pack %s with no objects"),
 			      preferred->pack_name);
 			result = 1;