Re: [PATCH v2 1/6] midx-write: only load initialized packs

> From: Derrick Stolee <stolee@gmail.com>
> 
> 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.