@matheusbsbs / matheus-bagliano / commits / c9388d9012

midx-write: only load initialized packs

The fill_packs_from_midx() method was refactored in fcb2205b77 (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. Add a new test that breaks under --stress when compiled with SANITIZE=address. The chosen number of 100 packfiles was selected to get the --stress output to fail about 50% of the time, while 50 packfiles could not get a failure in most --stress runs. The test case is marked as EXPENSIVE not only because of the number of packfiles it creates, but because some CI environments were reporting errors during the test that I could not reproduce, specifically around being unable to open the packfiles or their pack-indexes. When it fails under SANITIZE=address, it provides the following error: AddressSanitizer:DEADLYSIGNAL ================================================================= ==3263517==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000027 ==3263517==The signal is caused by a READ memory access. ==3263517==Hint: address points to the zero page. #0 0x562d5d82d1fb in close_pack_windows packfile.c:299 #1 0x562d5d82d3ab in close_pack packfile.c:354 #2 0x562d5d7bfdb4 in write_midx_internal midx-write.c:1490 #3 0x562d5d7c7aec in midx_repack midx-write.c:1795 #4 0x562d5d46fff6 in cmd_multi_pack_index builtin/multi-pack-index.c:305 ... This failure stack trace is disconnected from the real fix because the bad pointers are accessed later when closing the packfiles from the context. There are a few different aspects to this fix that are worth noting: 1. We return to the previous behavior of fill_packs_from_midx to not rely on the incremental flag or existence of a preferred pack. 2. The behavior to scan all layers of an incremental midx is kept, so this is not a full revert of the change. 3. We skip allocating more room in the pack_info array if the pack fails prepare_midx_pack(). 4. The method has always returned 0 for success and 1 for failure, but the condition checking for error added a check for a negative result for failure, so that is now updated. 5. The call to open_pack_index() is removed, but this is needed later in the case of a preferred pack. That call is moved to immediately before its result is needed (checking for the object count). Signed-off-by: Derrick Stolee <stolee@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Derrick Stolee committed Sep 5, 2025 at 19:26 UTC c9388d9012a71c6ef83ec94bd72eeb93f53caee4

2 files changed +36 -27

midx-write.c

+19 -27

@@ -938,8 +938,7 @@ cleanup:

938 return result;

939 }

940

941 -static int fill_packs_from_midx(struct write_midx_context *ctx,

942 - const char *preferred_pack_name, uint32_t flags)

941 +static int fill_packs_from_midx(struct write_midx_context *ctx)

942 {

943 struct multi_pack_index *m;

944

@@ -947,30 +946,11 @@ static int fill_packs_from_midx(struct write_midx_context *ctx,

946 uint32_t i;

947

948 for (i = 0; i < m->num_packs; i++) {

950 - ALLOC_GROW(ctx->info, ctx->nr + 1, ctx->alloc);

951 -

952 - /*

953 - * If generating a reverse index, need to have

954 - * packed_git's loaded to compare their

955 - * mtimes and object count.

956 - *

957 - * If a preferred pack is specified, need to

958 - * have packed_git's loaded to ensure the chosen

959 - * preferred pack has a non-zero object count.

960 - */

961 - if (flags & MIDX_WRITE_REV_INDEX ||

962 - preferred_pack_name) {

963 - if (prepare_midx_pack(ctx->repo, m,

964 - m->num_packs_in_base + i)) {

965 - error(_("could not load pack"));

966 - return 1;

967 - }

968 -

969 - if (open_pack_index(m->packs[i]))

970 - die(_("could not open index for %s"),

971 - m->packs[i]->pack_name);

972 - }

949 + if (prepare_midx_pack(ctx->repo, m,

950 + m->num_packs_in_base + i))

951 + return error(_("could not load pack"));

952

953 + ALLOC_GROW(ctx->info, ctx->nr + 1, ctx->alloc);

954 fill_pack_info(&ctx->info[ctx->nr++], m->packs[i],

955 m->pack_names[i],

956 m->num_packs_in_base + i);

@@ -1141,8 +1121,7 @@ static int write_midx_internal(struct repository *r, const char *object_dir,

1121 ctx.num_multi_pack_indexes_before++;

1122 m = m->base_midx;

1123 }

1144 - } else if (ctx.m && fill_packs_from_midx(&ctx, preferred_pack_name,

1145 - flags) < 0) {

1124 + } else if (ctx.m && fill_packs_from_midx(&ctx)) {

1125 goto cleanup;

1126 }

1127

@@ -1204,6 +1183,13 @@ static int write_midx_internal(struct repository *r, const char *object_dir,

1183 struct packed_git *oldest = ctx.info[ctx.preferred_pack_idx].p;

1184 ctx.preferred_pack_idx = 0;

1185

1186 + /*

1187 + * Attempt opening the pack index to populate num_objects.

1188 + * Ignore failiures as they can be expected and are not

1189 + * fatal during this selection time.

1190 + */

1191 + open_pack_index(oldest);

1192 +

1193 if (packs_to_drop && packs_to_drop->nr)

1194 BUG("cannot write a MIDX bitmap during expiration");

1195

@@ -1218,6 +1204,7 @@ static int write_midx_internal(struct repository *r, const char *object_dir,

1204

1205 if (!oldest->num_objects || p->mtime < oldest->mtime) {

1206 oldest = p;

1207 + open_pack_index(oldest);

1208 ctx.preferred_pack_idx = i;

1209 }

1210 }

@@ -1241,6 +1228,11 @@ static int write_midx_internal(struct repository *r, const char *object_dir,

1228

1229 if (ctx.preferred_pack_idx > -1) {

1230 struct packed_git *preferred = ctx.info[ctx.preferred_pack_idx].p;

1231 +

1232 + if (open_pack_index(preferred))

1233 + die(_("failed to open preferred pack %s"),

1234 + ctx.info[ctx.preferred_pack_idx].pack_name);

1235 +

1236 if (!preferred->num_objects) {

1237 error(_("cannot select preferred pack %s with no objects"),

1238 preferred->pack_name);

t/t5319-multi-pack-index.sh

+17

@@ -989,6 +989,23 @@ test_expect_success 'repack --batch-size=0 repacks everything' '

989 )

990 '

991

992 +test_expect_success EXPENSIVE 'repack/expire with many packs' '

993 + cp -r dup many &&

994 + (

995 + cd many &&

996 +

997 + for i in $(test_seq 1 100)

998 + do

999 + test_commit extra$i &&

1000 + git maintenance run --task=loose-objects || return 1

1001 + done &&

1002 +

1003 + git multi-pack-index write &&

1004 + git multi-pack-index repack &&

1005 + git multi-pack-index expire

1006 + )

1007 +'

1008 +

1009 test_expect_success 'repack --batch-size=<large> repacks everything' '

1010 (

1011 cd dup2 &&