bug: Segfault with git diff

bug: Segfault with git diff

[jake roggenbuck]

> Thank you for filling out a Git bug report!
> Please answer the following questions to help us understand your issue.

I have found a segfault when running git diff.

> What did you do before the bug happened? (Steps to reproduce your issue)

1. After running `git diff`, git showed me that object files were empty.
2. I deleted the empty object files, and ran `git diff` again.
3. I continued deleting the empty files until `git diff` segfaulted.

> What did you expect to happen? (Expected behavior)
An error message of some type or a graceful exit.

> What happened instead? (Actual behavior)
A segmentation fault.

`Segmentation fault (core dumped)`

> What's different between what you expected and what actually happened?
Instead of closing gracefully, there was a segmentation fault.

> Anything else you want to add:
git log displays:
fatal: bad object HEAD

git branch displays:
fatal: missing object 7610511b1b4db888e8e6bb8d0ff158f932961345 for
refs/heads/main

Neither log nor branch causes the segfault.

[System Info]
git version:
git version 2.45.1
cpu: x86_64
no commit associated with this build
sizeof-long: 8
sizeof-size_t: 8
shell-path: /bin/sh
uname: Linux 6.7.3-arch1-1 #1 SMP PREEMPT_DYNAMIC Thu, 01 Feb 2024
10:30:35 +0000 x86_64
compiler info: gnuc: 14.1
libc info: glibc: 2.39
$SHELL (typically, interactive shell): /bin/bash


[Enabled Hooks]

Best,
Jake Roggenbuck

[PATCH 0/1] Exit on invalid diff status of diff_filepair

[Jake Roggenbuck]

Hi all,

Git showed that object files were empty, so I manually removed them.
After removing a few object files, I ran `git diff` and I got a segfault.

I have since narrowed down the source of the error to a usage of
`diff_filepair`. I also noticed that the `status` field of `diff_filepair`
does not get set and stays a zero value. My fix is to check for that invalid
status character and exit gracefully with an error message.

I modeled the behavior of this patch after what `git log` does in the same
situation. `git log` will exit with a message that reads `bad object HEAD`.

I initially found this segfault when using git while working on a school
assignment. I am really interested in helping fix this segfault.
I'd be happy to hear any feedback.

Thank you!


Jake Roggenbuck (1):
  Exit on invalid diff status of diff_filepair

 diff.c | 4 ++++
 1 file changed, 4 insertions(+)

-- 
2.47.0

[PATCH 1/1] Exit on invalid diff status of diff_filepair

[Jake Roggenbuck]

Add a check for the invalid status of `0` for `diff_filepair` when certain
object files are missing. When these object files are missing, 'git log'
returns 'fatal: bad object HEAD' but 'git diff' segfaults.

Normally, the `diff_filepair` status should be a character, but when object
files are removed, status becomes a zero character which isn't listed as one
of the possible status letters in `Documentation/diff-format.txt`.

This patch checks for that invalid status character and gracefully exits with
an error message.

Signed-off-by: Jake Roggenbuck <jakeroggenbuck2@gmail.com>
---
 diff.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/diff.c b/diff.c
index 6c96154fed..aeab1ac445 100644
--- a/diff.c
+++ b/diff.c
@@ -7018,6 +7018,10 @@ void diff_queued_diff_prefetch(void *repository)
 
 	for (i = 0; i < q->nr; i++) {
 		struct diff_filepair *p = q->queue[i];
+
+		if (!p->status)
+			die("invalid diff status");
+
 		diff_add_if_missing(repo, &to_fetch, p->one);
 		diff_add_if_missing(repo, &to_fetch, p->two);
 	}
-- 
2.47.0

Re: [PATCH 1/1] Exit on invalid diff status of diff_filepair

[Jake Roggenbuck]

Hi all,

Has anyone gotten a chance to take a look at this simple fix?
When git is built of the main branch, it still segfaults when there is an invalid diff_filepair.

    git (main) $ make
    <make output omitted>
    git (main) $ cd ~/Repos/ECS50-3/hw3-skeleton-broken/
    hw3-skeleton-broken (main) $ ~/Build/git/git diff
    Segmentation fault (core dumped)
    hw3-skeleton-broken (main) $

Let me know if you have any feedback or suggestions.

Signed-off-by: Jake Roggenbuck <jakeroggenbuck2@gmail.com>

Re: [PATCH 1/1] Exit on invalid diff status of diff_filepair

[Jake Roggenbuck]

Re: [PATCH 1/1] Exit on invalid diff status of diff_filepair

[Eric Sunshine]

On Wed, Apr 30, 2025 at 2:53 PM Jake Roggenbuck
<jakeroggenbuck2@gmail.com> wrote:
> Has anyone gotten a chance to take a look at this simple fix?
> When git is built of the main branch, it still segfaults when there is an invalid diff_filepair.
>
>     git (main) $ make
>     <make output omitted>
>     git (main) $ cd ~/Repos/ECS50-3/hw3-skeleton-broken/
>     hw3-skeleton-broken (main) $ ~/Build/git/git diff
>     Segmentation fault (core dumped)
>     hw3-skeleton-broken (main) $
>
> Let me know if you have any feedback or suggestions.

Do you have a reproduction recipe which demonstrates the problem which
your patch fixes? Including the recipe in the patch's commit message
would help reviewers better understand the circumstances under which
the crash occurs since the descriptions provided by both the original
problem report[1] and the submitted patch[2] seem rather nebulous[3].

More importantly, if you have a reproduction recipe, then it can be
used as the basis for creating a test which should accompany the patch
(and which should be added to one of the `t/t40xx-*.sh` files). We can
help you convert the reproduction recipe into a test if desired.

FOOTNOTES

[1]: https://lore.kernel.org/git/CAEUC8gmgq_yViedLGHOeSyvR9rQK+O-8Fh9wzds=2+326ngUjw@mail.gmail.com/

[2]: https://lore.kernel.org/git/20250108060151.7218-2-jakeroggenbuck2@gmail.com/

[3]: It is not clear, for instance, what an "object file" is in the
provided description. Are you referring to one of the files under
`.git/objects/`?

Re: [PATCH 1/1] Exit on invalid diff status of diff_filepair

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

>>     git (main) $ cd ~/Repos/ECS50-3/hw3-skeleton-broken/
>>     hw3-skeleton-broken (main) $ ~/Build/git/git diff
>>     Segmentation fault (core dumped)
>>     hw3-skeleton-broken (main) $
>>
>> Let me know if you have any feedback or suggestions.
>
> Do you have a reproduction recipe which demonstrates the problem which
> your patch fixes? Including the recipe in the patch's commit message
> would help reviewers better understand the circumstances under which
> the crash occurs since the descriptions provided by both the original
> problem report[1] and the submitted patch[2] seem rather nebulous[3].
>
> More importantly, if you have a reproduction recipe, then it can be
> used as the basis for creating a test which should accompany the patch
> (and which should be added to one of the `t/t40xx-*.sh` files). We can
> help you convert the reproduction recipe into a test if desired.

Nicely put.

It is a bug _elsewhere_ in the code for the .status member to be
unassigned when the control reaches that point, so a patch to exit
after that happens is not all that interesting.  Instead of exiting
there, we would want to see a patch against the place where it
should have set the .status member but fails to do so.

Maybe with a corrupted repository, some of the blob objects we read
for comparison may fail to load and the function may be taking an
early return instead of complaining and dying upon unreadable blob
(I am not saying that is the only or even a likely case; just giving
an example situation for illustrate the point), in which case we
would want to fix _that_ code to complain and die properly.

Thanks.