[2.51.0] midx-write: fix segfault and other cleanups #787
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
I added a few more cleanups that I've seen in this file. I'm still looking for a smoking gun. I may reorganize these patches before sending them upstream. |
|
692232b is the smoking gun. I'll work on an upstream version tomorrow. @dscho @mjcheetham could you please give this a review? If you want, I can pull out everything except the "real" fix (the final commit) to avoid extra patches in the fork. |
dscho
previously approved these changes
Aug 28, 2025
There was a problem hiding this comment.
Thank you for working on this!
I had to kick the CI build twice until it passed (and couldn't make head nor tails of the failure because of a bug introduced in git/git that prevents failures to be shown in containerized builds), and I have a couple of questions/suggestions, but otherwise this looks ready to go to me!
| @@ -24,6 +22,7 @@ | |||
| #define BITMAP_POS_UNKNOWN (~((uint32_t)0)) | |||
| #define MIDX_CHUNK_FANOUT_SIZE (sizeof(uint32_t) * 256) | |||
| #define MIDX_CHUNK_LARGE_OFFSET_WIDTH (sizeof(uint64_t)) | |||
| #define NO_PREFERRED_PACK (~((uint32_t)0)) | |||
There was a problem hiding this comment.
Do we need to take precautions against an actual preferred pack with the number 2^32-1?
There was a problem hiding this comment.
Probably not, but at least it previously would have broken at 2^31 and works a bit longer now.
|
Making note that I have gitgitgadget#1965 prepared for upstream. It includes a test case that verifies this fix for the memory bug. |
derrickstolee
changed the title
derrickstolee
force-pushed
the
midx-write-free-and-null
branch
from
692232b to
6705ed1
Compare
|
Sorry to cause noise with the new version, but this matches the GGG version to minimize differences in future integration things. |
The fill_packs_from_midx() method was refactored in fcb2205 (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. 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. 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 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 <[email protected]>
This instance of setting the result to 1 before going to cleanup was accidentally removed in fcb2205 (midx: implement support for writing incremental MIDX chains, 2024-08-06). Signed-off-by: Derrick Stolee <[email protected]>
The incremental mode of writing a multi-pack-index has a few extra conditions that could lead to failure, but these are currently short-ciruiting with 'return -1' instead of setting the method's 'result' variable and going to the cleanup tag. Replace these returns with gotos to avoid memory issues when exiting early due to error conditions. Unfortunately, these error conditions are difficult to reproduce with test cases, which is perhaps one reason why the memory loss was not caught by existing test cases in memory tracking modes. Signed-off-by: Derrick Stolee <[email protected]>
midx-write.c has the DISABLE_SIGN_COMPARE_WARNINGS macro defined for a few reasons, but the biggest one is the use of a signed preferred_pack_idx member inside the write_midx_context struct. The code currently uses -1 to indicate an unset preferred pack but pack int ids are normally handled as uint32_t. There are also a few loops that search for the preferred pack by name and those iterators will need updates to uint32_t in the next change. For now, replace the use of -1 with a 'NO_PREFERRED_PACK' macro and an equality check. The macro stores the max value of a uint32_t, so we cannot store a preferred pack that appears last in a list of 2^32 total packs, but that's expected to be unreasonable already. This improves the range from 2^31 already. There are some careful things to worry about with initializing the preferred pack in the struct and using that value when searching for a preferred pack that was already incorrect but accidentally working when the index was initialized to zero. Signed-off-by: Derrick Stolee <[email protected]>
Remove the remaining signed comparison warnings in midx-write.c so that they can be enforced as errors in the future. After the previous change, the remaining errors are due to iterator variables named 'i'. The strategy here involves defining the variable within the for loop syntax to make sure we use the appropriate bitness for the loop sentinel. This matters in at least one method where the variable was compared to uint32_t in some loops and size_t in others. While adjusting these loops, there were some where the loop boundary was checking against a uint32_t value _plus one_. These were replaced with non-strict comparisons, but also the value is checked to not be UINT32_MAX. Since the value is the number of incremental multi-pack- indexes, this is not a meaningful restriction. The new die() is about defensive programming more than it being realistically possible. Signed-off-by: Derrick Stolee <[email protected]>
derrickstolee
force-pushed
the
midx-write-free-and-null
branch
from
6705ed1 to
abe22e5
Compare
derrickstolee
changed the title
Much appreciated! |
dscho
approved these changes
Sep 2, 2025
derrickstolee
added a commit
that referenced
this pull request
Sep 2, 2025
This is a copy of #787 but for the 2.50.1 branch.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Users have reported crashes during background maintenance. Their memory dumps appear to have an access violation in close_pack(), but it doesn't make sense based on the current implementation.
I finally figured out the problem after wading through a bunch of messiness in the midx-write.c file. This definitely fixes the segfault, but it also does some of that cleanup.
This is also going upstream in gitgitgadget#1965, but hopefully this version can be fast-tracked to microsoft/git for our users who need it quickly.