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

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

[ks1322 ks1322]

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

[PATCH] diagnose: require repository

[Martin Ågren]

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

Re: [PATCH] diagnose: require repository

[Kristoffer Haugsbakk]

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

Re: [PATCH] diagnose: require repository

[Junio C Hamano]

Martin Ågren <martin.agren@gmail.com> writes:

> 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;

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

[Christian Couder]

On Sat, Oct 14, 2023 at 2:00 PM ks1322 ks1322 <ks1322@gmail.com> wrote:
>
> 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.

Re: [PATCH] diagnose: require repository

[Martin Ågren]

On Sat, 14 Oct 2023 at 19:15, Junio C Hamano <gitster@pobox.com> wrote:
>
> 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.

> > 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.

Martin

Re: [PATCH] diagnose: require repository

[Junio C Hamano]

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.

>> +       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.

Re: [PATCH] diagnose: require repository

[Victoria Dye]

Martin Ågren wrote:
>>> 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