threads / bug / 60365

Bug: git diagnose crashes with Segmentation fault outside of git repository

Subject: Bug: git diagnose crashes with Segmentation fault outside of git repository

## tl;dr

8 messages between Oct 14, 2023 and Oct 19, 2023.

replies: 7people: 6as markdown or json

ks1322 ks1322· Oct 14, 2023, 11:59 UTC · lore

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

What did you do before the bug happened? (Steps to reproduce your issue) Run `git diagnose` outside of any git repository

What did you expect to happen? (Expected behavior) No crash, reasonable diagnose output

What happened instead? (Actual behavior) `git diagnose` crashed with Segmentation fault

$ git diagnose Collecting diagnostic info

git version 2.41.0 cpu: x86_64 no commit associated with this build sizeof-long: 8 sizeof-size_t: 8 shell-path: /bin/sh Repository root: (null) Available space on '/tmp': 7.75 GiB (mount flags 0x6) Segmentation fault (core dumped)

What's different between what you expected and what actually happened? Segmentation fault, truncated output

Anything else you want to add: `git diagnose` does not crash when run within some git repository

Please review the rest of the bug report below. You can delete any lines you don't wish to share.

[System Info] git version: git version 2.41.0 cpu: x86_64 no commit associated with this build sizeof-long: 8 sizeof-size_t: 8 shell-path: /bin/sh uname: Linux 6.5.6-200.fc38.x86_64 #1 SMP PREEMPT_DYNAMIC Fri Oct 6 19:02:35 UTC 2023 x86_64 compiler info: gnuc: 13.1 libc info: glibc: 2.37 $SHELL (typically, interactive shell): /bin/bash

[Enabled Hooks] not run from a git repository - no hooks to show

Martin Ågren· Oct 14, 2023, 13:53 UTC · re: ks1322 ks1322 · lore

[PATCH] diagnose: require repository

When `git diagnose` is run from outside a repo, it begins collecting various information before eventually hitting a segmentation fault, leaving an incomplete zip file behind.

Switch from the gentle setup to requiring a git directory. Without a git repo, there isn't really much to diagnose.

We could possibly do a best-effort collection of information about the machine and then give up. That would roughly be today's behavior but with a controlled exit rather than a segfault. However, the purpose of this tool is largely to create a zip archive. Rather than creating an empty zip file or no zip file at all, and having to explain that behavior, it seems more helpful to bail out clearly and early with a succinct error message.

Reported-by: ks1322 ks1322 <ks1322@gmail.com>
Signed-off-by: Martin Ågren <martin.agren@gmail.com>
---
 Thanks for the report. This could be one way of fixing this.
 I haven't found anything in the original submission [1] discussing this
 "_GENTLY". I didn't see anything in the implementation or the tests
 suggesting that it was intentional to run outside a git repo.
 [1] https://lore.kernel.org/git/xmqqzgg1nz6v.fsf@gitster.g/t/#mc66904caab6bc79e57eaf5063df268b2725b6fcc
 t/t0092-diagnose.sh | 5 +++++
 git.c               | 2 +-
 2 files changed, 6 insertions(+), 1 deletion(-)
diff --git a/t/t0092-diagnose.sh b/t/t0092-diagnose.sh
index 133e5747d6..49671d35a2 100755
--- a/t/t0092-diagnose.sh
+++ b/t/t0092-diagnose.sh
@@ -5,6 +5,11 @@ test_description='git diagnose'
 TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
+test_expect_success 'nothing to diagnose without repo' '
+	nongit test_must_fail git diagnose 2>err &&
+	grep "not a git repository" err
+'
+
 test_expect_success UNZIP 'creates diagnostics zip archive' '
 	test_when_finished rm -rf report &&
 
diff --git a/git.c b/git.c
index c67e44dd82..ff04a74bbd 100644
--- a/git.c
+++ b/git.c
@@ -525,7 +525,7 @@ static struct cmd_struct commands[] = {
 	{ "credential-cache--daemon", cmd_credential_cache_daemon },
 	{ "credential-store", cmd_credential_store },
 	{ "describe", cmd_describe, RUN_SETUP },
-	{ "diagnose", cmd_diagnose, RUN_SETUP_GENTLY },
+	{ "diagnose", cmd_diagnose, RUN_SETUP },
 	{ "diff", cmd_diff, NO_PARSEOPT },
 	{ "diff-files", cmd_diff_files, RUN_SETUP | NEED_WORK_TREE | NO_PARSEOPT },
 	{ "diff-index", cmd_diff_index, RUN_SETUP | NO_PARSEOPT },

-- 
2.42.0.399.g85a82e71e0
Kristoffer Haugsbakk· Oct 14, 2023, 14:56 UTC · re: Martin Ågren · lore

Re: [PATCH] diagnose: require repository

On Sat, Oct 14, 2023, at 15:53, Martin Ågren wrote:
> Switch from the gentle setup to requiring a git directory. Without a git
> repo, there isn't really much to diagnose.
This patch works for me. Thanks.
-- 
Kristoffer Haugsbakk
Junio C Hamano· Oct 14, 2023, 17:15 UTC · re: Martin Ågren · lore

Re: [PATCH] diagnose: require repository

Martin Ågren <martin.agren@gmail.com> writes:
Show 14 quoted lines
> When `git diagnose` is run from outside a repo, it begins collecting
> various information before eventually hitting a segmentation fault,
> leaving an incomplete zip file behind.
>
> Switch from the gentle setup to requiring a git directory. Without a git
> repo, there isn't really much to diagnose.
>
> We could possibly do a best-effort collection of information about the
> machine and then give up. That would roughly be today's behavior but
> with a controlled exit rather than a segfault. However, the purpose of
> this tool is largely to create a zip archive. Rather than creating an
> empty zip file or no zip file at all, and having to explain that
> behavior, it seems more helpful to bail out clearly and early with a
> succinct error message.

Without having thought things through, offhand I agree with your "no repository? there is nothing worth tarring up then" assessment.

Because "git bugreport --diag" unconditionally spawns "git diagnose", the former may also want to be extra careful, perhaps like the attached patch.

 builtin/bugreport.c | 5 +++++
 1 file changed, 5 insertions(+)
diff --git c/builtin/bugreport.c w/builtin/bugreport.c
index d2ae5c305d..ac9e05fcf7 100644
--- c/builtin/bugreport.c
+++ w/builtin/bugreport.c
@@ -146,6 +146,11 @@ int cmd_bugreport(int argc, const char **argv, const char *prefix)
 		    report_path.buf);
 	}
 
+	if (!startup_info->have_repository && diagnose != DIAGNOSE_NONE) {
+		warning(_("no repository--diagnostic output disabled"));
+		diagnose = DIAGNOSE_NONE;
+	}
+
 	/* Prepare diagnostics, if requested */
 	if (diagnose != DIAGNOSE_NONE) {
 		struct strbuf zip_path = STRBUF_INIT;
Martin Ågren· Oct 19, 2023, 13:18 UTC · re: Junio C Hamano · lore

Re: [PATCH] diagnose: require repository

On Sat, 14 Oct 2023 at 19:15, Junio C Hamano <gitster@pobox.com> wrote:
Show 11 quoted lines
>
> Martin Ågren <martin.agren@gmail.com> writes:
>
> > Switch from the gentle setup to requiring a git directory. Without a git
> > repo, there isn't really much to diagnose.
> >
> > We could possibly do a best-effort collection of information about the
> > machine and then give up. That would roughly be today's behavior but
> > with a controlled exit rather than a segfault. However, the purpose of
> > this tool is largely to create a zip archive. Rather than creating an
> > empty zip file or no zip file at all, and having to explain that

Correcting myself: The zip archive would actually contain `diagnostics.log` with some general info about the machine and Git build.

Show 9 quoted lines
> > behavior, it seems more helpful to bail out clearly and early with a
> > succinct error message.
>
> Without having thought things through, offhand I agree with your "no
> repository?  there is nothing worth tarring up then" assessment.
>
> Because "git bugreport --diag" unconditionally spawns "git
> diagnose", the former may also want to be extra careful, perhaps
> like the attached patch.
Good point. TBH, I had no idea about `git bugreport --diagnose`.
Show 5 quoted lines
> +       if (!startup_info->have_repository && diagnose != DIAGNOSE_NONE) {
> +               warning(_("no repository--diagnostic output disabled"));
> +               diagnose = DIAGNOSE_NONE;
> +       }
> +

When the user explicitly provides that option, it seems unfortunate to me to drop it. Yes, we'd warn, but `git bugreport` then pops a text editor, so you would only see the warning after finishing up the report. (Maybe. By the time you quit your editor, you might not consider checking the terminal for warnings and such.)

So I'm inclined to instead just die if we see the option outside a repo. If `diagnose` the command fundamentally requires a repo (as with my patch) it seems surprising to me to not have `--diagnose` the option behave the same.

Martin
Junio C Hamano· Oct 19, 2023, 18:09 UTC · re: Martin Ågren · lore

Re: [PATCH] diagnose: require repository

Martin Ågren <martin.agren@gmail.com> writes:
> Correcting myself: The zip archive would actually contain
> `diagnostics.log` with some general info about the machine and Git
> build.

So it could contain some useful information without a specific repository, perhaps.

> Good point. TBH, I had no idea about `git bugreport --diagnose`.

You are not alone ;-) I didn't, either. Before responding to your patch, that is.

Show 16 quoted lines
>> +       if (!startup_info->have_repository && diagnose != DIAGNOSE_NONE) {
>> +               warning(_("no repository--diagnostic output disabled"));
>> +               diagnose = DIAGNOSE_NONE;
>> +       }
>> +
>
> When the user explicitly provides that option, it seems unfortunate to
> me to drop it. Yes, we'd warn, but `git bugreport` then pops a text
> editor, so you would only see the warning after finishing up the report.
> (Maybe. By the time you quit your editor, you might not consider
> checking the terminal for warnings and such.)
>
> So I'm inclined to instead just die if we see the option outside a repo.
> If `diagnose` the command fundamentally requires a repo (as with my
> patch) it seems surprising to me to not have `--diagnose` the option
> behave the same.

I have no strong opinion. Victoria is on Cc: already, whose name appears a lot more often than mine in the shortlog for "diagnose" stuff, so I'll defer to her area expertise.

Thanks.
Victoria Dye· Oct 19, 2023, 18:16 UTC · re: Martin Ågren · lore

Re: [PATCH] diagnose: require repository

Martin Ågren wrote:
Show 28 quoted lines
>>> behavior, it seems more helpful to bail out clearly and early with a
>>> succinct error message.
>>
>> Without having thought things through, offhand I agree with your "no
>> repository?  there is nothing worth tarring up then" assessment.
>>
>> Because "git bugreport --diag" unconditionally spawns "git
>> diagnose", the former may also want to be extra careful, perhaps
>> like the attached patch.
> 
> Good point. TBH, I had no idea about `git bugreport --diagnose`.
> 
>> +       if (!startup_info->have_repository && diagnose != DIAGNOSE_NONE) {
>> +               warning(_("no repository--diagnostic output disabled"));
>> +               diagnose = DIAGNOSE_NONE;
>> +       }
>> +
> 
> When the user explicitly provides that option, it seems unfortunate to
> me to drop it. Yes, we'd warn, but `git bugreport` then pops a text
> editor, so you would only see the warning after finishing up the report.
> (Maybe. By the time you quit your editor, you might not consider
> checking the terminal for warnings and such.)
> 
> So I'm inclined to instead just die if we see the option outside a repo.
> If `diagnose` the command fundamentally requires a repo (as with my
> patch) it seems surprising to me to not have `--diagnose` the option
> behave the same.

I agree - it was an oversight on my part to not firmly require the existence of a repository with 'git diagnose', and the same applies to 'bugreport --diagnose'.

For reference, there is one other usage of 'git diagnose' (in 'scalar diagnose'). However, it's already guarded by 'setup_git_directory()' so it shouldn't need to be updated.

> 
> Martin
Christian Couder· Oct 14, 2023, 17:22 UTC · re: ks1322 ks1322 · lore

Re: Bug: git diagnose crashes with Segmentation fault outside of git repository

On Sat, Oct 14, 2023 at 2:00 PM ks1322 ks1322 <ks1322@gmail.com> wrote:
Show 25 quoted lines
>
> Thank you for filling out a Git bug report!
> Please answer the following questions to help us understand your issue.
>
> What did you do before the bug happened? (Steps to reproduce your issue)
> Run `git diagnose` outside of any git repository
>
> What did you expect to happen? (Expected behavior)
> No crash, reasonable diagnose output
>
> What happened instead? (Actual behavior)
> `git diagnose` crashed with Segmentation fault
>
> $ git diagnose
> Collecting diagnostic info
>
> git version 2.41.0
> cpu: x86_64
> no commit associated with this build
> sizeof-long: 8
> sizeof-size_t: 8
> shell-path: /bin/sh
> Repository root: (null)
> Available space on '/tmp': 7.75 GiB (mount flags 0x6)
> Segmentation fault (core dumped)

Yeah, this is a valid bug report that I can reproduce. It looks like such bugs have existed in this command since it was created last year.

Best, Christian.

← back to recent threads