# Bug: git diagnose crashes with Segmentation fault outside of git repository 8 messages from 2023-10-14 to 2023-10-19. Participants: ks1322 ks1322, Martin Ågren, Kristoffer Haugsbakk, Junio C Hamano, Christian Couder, Victoria Dye. Thread: https://gitlist.dev/t/60365 ## ks1322 ks1322, 2023-10-14 11:59 Subject: Bug: git diagnose crashes with Segmentation fault outside of git repository Message-ID: URL: https://gitlist.dev/e/CAKFQ_Q9WjF9i-Rx2jdCw-adPVQrWNfNKrDY-em8Rpa5RNLXz4A%40mail.gmail.com ``` 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, 2023-10-14 13:53 Subject: [PATCH] diagnose: require repository Message-ID: <20231014135302.13095-1-martin.agren@gmail.com> URL: https://gitlist.dev/e/20231014135302.13095-1-martin.agren%40gmail.com In-Reply-To: ``` 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 Signed-off-by: Martin Ågren --- 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, 2023-10-14 14:56 Subject: Re: [PATCH] diagnose: require repository Message-ID: URL: https://gitlist.dev/e/cfc411f9-26b4-4af2-8c50-29fbe3dcbe43%40app.fastmail.com In-Reply-To: <20231014135302.13095-1-martin.agren@gmail.com> ``` 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, 2023-10-14 17:15 Subject: Re: [PATCH] diagnose: require repository Message-ID: URL: https://gitlist.dev/e/xmqq5y39unvc.fsf%40gitster.g In-Reply-To: <20231014135302.13095-1-martin.agren@gmail.com> ``` Martin Ågren 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; ``` ## Christian Couder, 2023-10-14 17:22 Subject: Re: Bug: git diagnose crashes with Segmentation fault outside of git repository Message-ID: URL: https://gitlist.dev/e/CAP8UFD3z5xKfvao2Q87tK7y5P%2B2LSKW_Hv-dBvFb%3DJ3gm7-Arg%40mail.gmail.com In-Reply-To: ``` On Sat, Oct 14, 2023 at 2:00 PM ks1322 ks1322 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. ``` ## Martin Ågren, 2023-10-19 13:18 Subject: Re: [PATCH] diagnose: require repository Message-ID: URL: https://gitlist.dev/e/CAN0heSqmZ7QXJbet2Tp%3DYYCjBLToOHtNy%2Bn%3Dzcf29XYaukYN0w%40mail.gmail.com In-Reply-To: ``` On Sat, 14 Oct 2023 at 19:15, Junio C Hamano wrote: > > Martin Ågren 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 ``` ## Junio C Hamano, 2023-10-19 18:09 Subject: Re: [PATCH] diagnose: require repository Message-ID: URL: https://gitlist.dev/e/xmqqo7guwkla.fsf%40gitster.g In-Reply-To: ``` Martin Ågren 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. ``` ## Victoria Dye, 2023-10-19 18:16 Subject: Re: [PATCH] diagnose: require repository Message-ID: <01569dd1-f807-d56d-a123-5e8f3c930503@github.com> URL: https://gitlist.dev/e/01569dd1-f807-d56d-a123-5e8f3c930503%40github.com In-Reply-To: ``` 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 ```