diff: avoid segfault with freed entries #2027
Open
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
derrickstolee
force-pushed
the
diff-segfault
branch
from
0988373 to
9d3a73f
Compare
1 task
When computing a diff in a partial clone, there is a chance that we could trigger a prefetch of missing objects at the same time as we are freeing entries from the global diff queue. This is difficult to reproduce, as we need to have some objects be freed from the queue before triggering the prefetch of missing objects. There is a new test in t4067 that does trigger the segmentation fault that results in this case. The fix is to set the queue pointer to NULL after it is freed, and then to be careful about NULL values in the prefetch. The more elaborate explanation is that within diffcore_std(), we may skip the initial prefetch due to the output format (--name-only in the test) and go straight to diffcore_skip_stat_unmatch(). In that method, the index entries that have been invalidated by path changes show up as entries but may be deleted because they are not actually content diffs and only newer timestamps than expected. As those entries are deleted, later entries are checked with diff_filespec_check_stat_unmatch(), which uses diff_queued_diff_prefetch() as the missing_object_cb in its diff options. That can trigger downloading missing objects if the appropriate scenario occurs to trigger a call to diff_popoulate_filespec(). It's finally within that callback to diff_queued_diff_prefetch() that the segfault occurs. The test was hard to find because it required some real differences, some not-different files that had a newer modified time, and the order of those files alphabetically was important to trigger the deletion before the prefetch was triggered. I briefly considered a "lock" member for the diff queue, but it was a much larger diff and introduced many more possible error scenarios. Signed-off-by: Derrick Stolee <stolee@gmail.com>
derrickstolee
force-pushed
the
diff-segfault
branch
from
9d3a73f to
98e06c8
Compare
Author
|
/submit |
|
Submitted as pull.2027.git.1767044697712.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
|
On the Git mailing list, Junio C Hamano wrote (reply to this): "Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:
> The more elaborate explanation is that within diffcore_std(), we may
> skip the initial prefetch due to the output format (--name-only in the
> test) and go straight to diffcore_skip_stat_unmatch().
That's very interesting. We have code to fetch on-demand when it
turns out that the initial prefetch shouldn't have been skipped and
we need contents, so in that sense, the condition to skip the
initial prefetch does not have to be precise, but we may want to see
if we can have a single helper function that exactly tells us if we
need to look at the contents or not. I think we had a few changes
that made the definition of "diff status is based on contents" in
the past few releases, not for the purpose of this prefetch skipping
but to set the exit status. |
|
On the Git mailing list, "Kristoffer Haugsbakk" wrote (reply to this): On Mon, Dec 29, 2025, at 22:44, Derrick Stolee via GitGitGadget wrote:
> From: Derrick Stolee <stolee@gmail.com>
>
>[snip]
> The more elaborate explanation is that within diffcore_std(), we may
> skip the initial prefetch due to the output format (--name-only in the
> test) and go straight to diffcore_skip_stat_unmatch(). In that method,
> the index entries that have been invalidated by path changes show up as
> entries but may be deleted because they are not actually content diffs
> and only newer timestamps than expected. As those entries are deleted,
> later entries are checked with diff_filespec_check_stat_unmatch(), which
> uses diff_queued_diff_prefetch() as the missing_object_cb in its diff
> options. That can trigger downloading missing objects if the appropriate
> scenario occurs to trigger a call to diff_popoulate_filespec(). It's
s/diff_popoulate_filespec/diff_populate_filespec/
> finally within that callback to diff_queued_diff_prefetch() that the
> segfault occurs.
>
>[snip] |
|
User |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
I found this segfault in the wild in a pipeline that was using the microsoft/git fork, but the error can be reproduced without that fork. It requires partial clone, though.
Thanks,
-Stolee
cc: gitster@pobox.com
cc: "Kristoffer Haugsbakk" kristofferhaugsbakk@fastmail.com