From: Derrick Stolee <stolee@gmail•com>
To: Taylor Blau <me@ttaylorr•com>,
Derrick Stolee via GitGitGadget <gitgitgadget@gmail•com>
Cc: git@vger•kernel.org, gitster@pobox•com
Subject: Re: [PATCH 1/5] midx-write: only load initialized packs
Date: Sat, 30 Aug 2025 10:33:39 -0400 [thread overview]
Message-ID: <f161727d-7688-4f62-9eb4-67d1b9033503@gmail.com> (raw)
In-Reply-To: <aLEAaNsm8LE2M3TE@nand.local>
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 <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.
>
> 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
next prev parent reply other threads:[~2025-08-30 14:33 UTC|newest]
Thread overview: 47+ messages / expand[flat|nested] mbox.gz Atom feed top
2025-08-28 17:39 [PATCH 0/5] midx-write: fix segfault and do several cleanups Derrick Stolee via GitGitGadget
2025-08-28 17:39 ` [PATCH 1/5] midx-write: only load initialized packs Derrick Stolee via GitGitGadget
2025-08-28 20:19 ` Junio C Hamano
2025-08-29 1:20 ` Taylor Blau
2025-08-30 14:33 ` Derrick Stolee [this message]
2025-08-28 17:39 ` [PATCH 2/5] midx-write: put failing response value back Derrick Stolee via GitGitGadget
2025-08-28 20:45 ` Junio C Hamano
2025-08-29 1:26 ` Taylor Blau
2025-08-28 17:39 ` [PATCH 3/5] midx-write: use cleanup when incremental midx fails Derrick Stolee via GitGitGadget
2025-08-28 20:51 ` Junio C Hamano
2025-08-29 1:29 ` Taylor Blau
2025-08-30 14:44 ` Derrick Stolee
2025-08-28 17:39 ` [PATCH 4/5] midx-write: use uint32_t for preferred_pack_idx Derrick Stolee via GitGitGadget
2025-08-28 20:58 ` Junio C Hamano
2025-08-29 1:35 ` Taylor Blau
2025-08-28 17:39 ` [PATCH 5/5] midx-write: reenable signed comparison errors Derrick Stolee via GitGitGadget
2025-08-28 21:01 ` Junio C Hamano
2025-08-29 1:35 ` Taylor Blau
2025-08-29 1:36 ` [PATCH 0/5] midx-write: fix segfault and do several cleanups Taylor Blau
2025-08-30 21:23 ` [PATCH v2 0/6] " Derrick Stolee via GitGitGadget
2025-08-30 21:23 ` [PATCH v2 1/6] midx-write: only load initialized packs Derrick Stolee via GitGitGadget
2025-09-03 10:14 ` Patrick Steinhardt
2025-09-05 18:58 ` Derrick Stolee
2025-08-30 21:23 ` [PATCH v2 2/6] midx-write: put failing response value back Derrick Stolee via GitGitGadget
2025-09-03 10:15 ` Patrick Steinhardt
2025-09-05 19:03 ` Derrick Stolee
2025-08-30 21:23 ` [PATCH v2 3/6] midx-write: use cleanup when incremental midx fails Derrick Stolee via GitGitGadget
2025-09-03 10:15 ` Patrick Steinhardt
2025-08-30 21:23 ` [PATCH v2 4/6] midx-write: use uint32_t for preferred_pack_idx Derrick Stolee via GitGitGadget
2025-09-03 10:15 ` Patrick Steinhardt
2025-09-05 19:05 ` Derrick Stolee
2025-08-30 21:23 ` [PATCH v2 5/6] midx-write: reenable signed comparison errors Derrick Stolee via GitGitGadget
2025-09-03 10:15 ` Patrick Steinhardt
2025-08-30 21:23 ` [PATCH v2 6/6] midx-write: simplify error cases Derrick Stolee via GitGitGadget
2025-09-03 10:15 ` Patrick Steinhardt
2025-09-03 18:43 ` Junio C Hamano
2025-09-05 19:26 ` [PATCH v3 0/6] midx-write: fix segfault and do several cleanups Derrick Stolee via GitGitGadget
2025-09-05 19:26 ` [PATCH v3 1/6] midx-write: only load initialized packs Derrick Stolee via GitGitGadget
2025-09-05 19:26 ` [PATCH v3 2/6] midx-write: put failing response value back Derrick Stolee via GitGitGadget
2025-09-05 19:26 ` [PATCH v3 3/6] midx-write: use cleanup when incremental midx fails Derrick Stolee via GitGitGadget
2025-09-05 19:26 ` [PATCH v3 4/6] midx-write: use uint32_t for preferred_pack_idx Derrick Stolee via GitGitGadget
2025-09-05 19:26 ` [PATCH v3 5/6] midx-write: reenable signed comparison errors Derrick Stolee via GitGitGadget
2025-09-05 19:26 ` [PATCH v3 6/6] midx-write: simplify error cases Derrick Stolee via GitGitGadget
2025-09-05 19:38 ` [PATCH v3 0/6] midx-write: fix segfault and do several cleanups Junio C Hamano
2025-09-05 19:57 ` Derrick Stolee
2025-09-11 23:13 ` Taylor Blau
2025-09-11 23:44 ` Junio C Hamano
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=f161727d-7688-4f62-9eb4-67d1b9033503@gmail.com \
--to=stolee@gmail$(echo .)com \
--cc=git@vger$(echo .)kernel.org \
--cc=gitgitgadget@gmail$(echo .)com \
--cc=gitster@pobox$(echo .)com \
--cc=me@ttaylorr$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox