From: Derrick Stolee Date: Sat, 30 Aug 2025 14:33:39 GMT Subject: Re: [PATCH 1/5] midx-write: only load initialized packs Message-ID: In-Reply-To: On 8/28/25 9:20 PM, Taylor Blau wrote: > On Thu, Aug 28, 2025 at 05:39:51PM +0000, Derrick Stolee via GitGitGadget wrote: >> From: Derrick Stolee >> >> 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. > > Thanks for looking at this one. On the surface this looks not great, but > I am having a hard time coming up with a smaller test case that > exercises this behavior. > > I can get what you wrote below to fail on my machine pretty reliable > when building with SANITIZE=address (even without --stress). All of the > spots that read from the pack_info array and access the actual > packed_git structs are guarded by either writing a MIDX bitmap or having > a non-empty preferred pack. I'm glad you're able to reproduce it. My --stress runs had about a 50% hit rate. >> 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. This test has a very >> minor check at the end confirming only one packfile remaining. The >> failing nature of this test actually relies on auto-GC cleaning up some >> packfiles during the creation of the commits, as tests setting gc.auto >> to zero make the packfile count match the number of added commits but >> also avoids hitting the memory issue. > > Hmm. Is this portion of the commit message out-of-date? I can't see the > check you're referring to that ensures there is only one pack remaining, > nor can I see the spot where we disable gc.auto. You're right. When I added more robustness around the packfile count by removing gc.auto, the test stopped failing pre-fix. Then, I forgot to remove mention of those test updates. >> 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 it > > s/it// ? Thanks. >> 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. > > Oops ;-). > >> 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). > > I think we need to do this in at least one other spot, but see below. Interesting! >> + if (prepare_midx_pack(ctx->repo, m, >> + m->num_packs_in_base + i)) { >> + error(_("could not load pack")); >> + return 1; > > Looks good, though I agree with Junio's comment in his separate reply > that we could probably just turn this into "return error(...)" while > we're at it. Can do. >> @@ -1223,6 +1204,11 @@ static int write_midx_internal(struct repository *r, const char *object_dir, >> >> if (ctx.preferred_pack_idx > -1) { >> struct packed_git *preferred = ctx.info[ctx.preferred_pack_idx].p; >> + >> + if (open_pack_index(preferred)) >> + die(_("failed to open preferred pack %s"), >> + ctx.info[ctx.preferred_pack_idx].pack_name); > > This makes sense, but I think we need to apply similar treatment in the > "else if" arm of the if-statement immediately above this one too. That > portion of the code handles the case where we're writing a MIDX bitmap > but didn't provide a preferred pack. > > When that's the case, we loop through to try and find the oldest pack > that contains at least one object. If we don't call open_pack_index() > all of those ->num_objects fields will still be zero'd, so we'll only > find the oldest pack. > > That may actually produce wrong behavior if we have duplicate objects > that aren't uniformly resolved in favor of the earliest pack in lex > order. I'd have to think about it a little more to be sure, though. I see. In this case, we need to open_pack_index() before relying on oldest->num_objects, which only needs to happen for the first pack and any packfile that wins via mtime preference. It also seems like we can _warn_ on failures to open packfiles in those cases, since it isn't fatal if some packfiles fail to open. Thanks, -Stolee