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

From: Derrick Stolee <stolee@gmail.com>
Date: Sep 5, 2025, 18:58 UTC
Message-ID: <745f5241-b745-481b-85e7-8cfe6ef73844@gmail.com>
In-Reply-To: <aLgVIPpW9QBFmgb7@pks.im>

On 9/3/2025 6:14 AM, Patrick Steinhardt wrote:

Show 18 quoted lines

> On Sat, Aug 30, 2025 at 09:23:22PM +0000, Derrick Stolee via GitGitGadget wrote:
>> 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.
> 
> I honestly don't quite understand why the conditions are necessary here.
> In other words, why do we need to be careful _not_ to open the
> packfiles?

My wording is poor. "We don't load packfiles unless one of these conditions holds" is more appropriate.

There are some test cases that want to keep things working even
when a .idx file disappears, I think. This is a reason to be
careful about open_pack_index(), but prepare_midx_pack() is
something we want to call always.

Show 29 quoted lines

>> 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.
> 
> Okay. So in other words we need to make sure to always prepare the
> MIDX'd packfiles, but we may not want to open them?

Yes. Always prepare. Don't always open (since that loads the .idx).

Show 17 quoted lines

>> 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.
> 
> Nit, feel free to ignore: this change feels like it would make for a
> nice separate commit.

True. I only included it since I was modifying the call anyway due to the changing parameters.

Thanks, -Stolee