{"thread":{"id":"60365","subject":"Bug: git diagnose crashes with Segmentation fault outside of git repository","startedAt":"2023-10-14T11:59:55Z","lastAt":"2023-10-19T18:16:42Z","messageCount":8,"participants":["ks1322 ks1322","Martin Ågren","Kristoffer Haugsbakk","Junio C Hamano","Christian Couder","Victoria Dye"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"483243","messageId":"CAKFQ_Q9WjF9i-Rx2jdCw-adPVQrWNfNKrDY-em8Rpa5RNLXz4A@mail.gmail.com","threadId":"60365","inReplyTo":null,"subject":"Bug: git diagnose crashes with Segmentation fault outside of git repository","fromName":"ks1322 ks1322","fromEmail":"ks1322@gmail.com","sentAt":"2023-10-14T11:59:42Z","receivedAt":"2023-10-14T11:59:55Z","isPatch":false,"sender":{"key":"ks1322@gmail.com","avatar":null},"body":"Thank you for filling out a Git bug report!\nPlease answer the following questions to help us understand your issue.\n\nWhat did you do before the bug happened? (Steps to reproduce your issue)\nRun `git diagnose` outside of any git repository\n\nWhat did you expect to happen? (Expected behavior)\nNo crash, reasonable diagnose output\n\nWhat happened instead? (Actual behavior)\n`git diagnose` crashed with Segmentation fault\n\n$ git diagnose\nCollecting diagnostic info\n\ngit version 2.41.0\ncpu: x86_64\nno commit associated with this build\nsizeof-long: 8\nsizeof-size_t: 8\nshell-path: /bin/sh\nRepository root: (null)\nAvailable space on '/tmp': 7.75 GiB (mount flags 0x6)\nSegmentation fault (core dumped)\n\nWhat's different between what you expected and what actually happened?\nSegmentation fault, truncated output\n\nAnything else you want to add:\n`git diagnose` does not crash when run within some git repository\n\nPlease review the rest of the bug report below.\nYou can delete any lines you don't wish to share.\n\n\n[System Info]\ngit version:\ngit version 2.41.0\ncpu: x86_64\nno commit associated with this build\nsizeof-long: 8\nsizeof-size_t: 8\nshell-path: /bin/sh\nuname: Linux 6.5.6-200.fc38.x86_64 #1 SMP PREEMPT_DYNAMIC Fri Oct 6\n19:02:35 UTC 2023 x86_64\ncompiler info: gnuc: 13.1\nlibc info: glibc: 2.37\n$SHELL (typically, interactive shell): /bin/bash\n\n\n[Enabled Hooks]\nnot run from a git repository - no hooks to show\n"},{"id":"483244","messageId":"20231014135302.13095-1-martin.agren@gmail.com","threadId":"60365","inReplyTo":"CAKFQ_Q9WjF9i-Rx2jdCw-adPVQrWNfNKrDY-em8Rpa5RNLXz4A@mail.gmail.com","subject":"[PATCH] diagnose: require repository","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2023-10-14T13:53:01Z","receivedAt":"2023-10-14T13:53:36Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"When `git diagnose` is run from outside a repo, it begins collecting\nvarious information before eventually hitting a segmentation fault,\nleaving an incomplete zip file behind.\n\nSwitch from the gentle setup to requiring a git directory. Without a git\nrepo, there isn't really much to diagnose.\n\nWe could possibly do a best-effort collection of information about the\nmachine and then give up. That would roughly be today's behavior but\nwith a controlled exit rather than a segfault. However, the purpose of\nthis tool is largely to create a zip archive. Rather than creating an\nempty zip file or no zip file at all, and having to explain that\nbehavior, it seems more helpful to bail out clearly and early with a\nsuccinct error message.\n\nReported-by: ks1322 ks1322 \nSigned-off-by: Martin Ågren \n---\n Thanks for the report. This could be one way of fixing this.\n\n I haven't found anything in the original submission [1] discussing this\n \"_GENTLY\". I didn't see anything in the implementation or the tests\n suggesting that it was intentional to run outside a git repo.\n\n [1] https://lore.kernel.org/git/xmqqzgg1nz6v.fsf@gitster.g/t/#mc66904caab6bc79e57eaf5063df268b2725b6fcc\n\n t/t0092-diagnose.sh | 5 +++++\n git.c | 2 +-\n 2 files changed, 6 insertions(+), 1 deletion(-)\n\ndiff --git a/t/t0092-diagnose.sh b/t/t0092-diagnose.sh\nindex 133e5747d6..49671d35a2 100755\n--- a/t/t0092-diagnose.sh\n+++ b/t/t0092-diagnose.sh\n@@ -5,6 +5,11 @@ test_description='git diagnose'\n TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n+test_expect_success 'nothing to diagnose without repo' '\n+\tnongit test_must_fail git diagnose 2>err &&\n+\tgrep \"not a git repository\" err\n+'\n+\n test_expect_success UNZIP 'creates diagnostics zip archive' '\n \ttest_when_finished rm -rf report &&\n \ndiff --git a/git.c b/git.c\nindex c67e44dd82..ff04a74bbd 100644\n--- a/git.c\n+++ b/git.c\n@@ -525,7 +525,7 @@ static struct cmd_struct commands[] = {\n \t{ \"credential-cache--daemon\", cmd_credential_cache_daemon },\n \t{ \"credential-store\", cmd_credential_store },\n \t{ \"describe\", cmd_describe, RUN_SETUP },\n-\t{ \"diagnose\", cmd_diagnose, RUN_SETUP_GENTLY },\n+\t{ \"diagnose\", cmd_diagnose, RUN_SETUP },\n \t{ \"diff\", cmd_diff, NO_PARSEOPT },\n \t{ \"diff-files\", cmd_diff_files, RUN_SETUP | NEED_WORK_TREE | NO_PARSEOPT },\n \t{ \"diff-index\", cmd_diff_index, RUN_SETUP | NO_PARSEOPT },\n-- \n2.42.0.399.g85a82e71e0\n\n"},{"id":"483245","messageId":"cfc411f9-26b4-4af2-8c50-29fbe3dcbe43@app.fastmail.com","threadId":"60365","inReplyTo":"20231014135302.13095-1-martin.agren@gmail.com","subject":"Re: [PATCH] diagnose: require repository","fromName":"Kristoffer Haugsbakk","fromEmail":"code@khaugsbakk.name","sentAt":"2023-10-14T14:56:02Z","receivedAt":"2023-10-14T14:56:43Z","isPatch":true,"sender":{"key":"code@khaugsbakk.name","avatar":"https://avatars.githubusercontent.com/u/2229597?v=4"},"body":"On Sat, Oct 14, 2023, at 15:53, Martin Ågren wrote:\n> Switch from the gentle setup to requiring a git directory. Without a git\n> repo, there isn't really much to diagnose.\n\nThis patch works for me. Thanks.\n\n-- \nKristoffer Haugsbakk\n"},{"id":"483252","messageId":"xmqq5y39unvc.fsf@gitster.g","threadId":"60365","inReplyTo":"20231014135302.13095-1-martin.agren@gmail.com","subject":"Re: [PATCH] diagnose: require repository","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-10-14T17:15:19Z","receivedAt":"2023-10-14T17:15:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Martin Ågren writes:\n\n> When `git diagnose` is run from outside a repo, it begins collecting\n> various information before eventually hitting a segmentation fault,\n> leaving an incomplete zip file behind.\n>\n> Switch from the gentle setup to requiring a git directory. Without a git\n> repo, there isn't really much to diagnose.\n>\n> We could possibly do a best-effort collection of information about the\n> machine and then give up. That would roughly be today's behavior but\n> with a controlled exit rather than a segfault. However, the purpose of\n> this tool is largely to create a zip archive. Rather than creating an\n> empty zip file or no zip file at all, and having to explain that\n> behavior, it seems more helpful to bail out clearly and early with a\n> succinct error message.\n\nWithout having thought things through, offhand I agree with your \"no\nrepository? there is nothing worth tarring up then\" assessment.\n\nBecause \"git bugreport --diag\" unconditionally spawns \"git\ndiagnose\", the former may also want to be extra careful, perhaps\nlike the attached patch.\n\n builtin/bugreport.c | 5 +++++\n 1 file changed, 5 insertions(+)\n\ndiff --git c/builtin/bugreport.c w/builtin/bugreport.c\nindex d2ae5c305d..ac9e05fcf7 100644\n--- c/builtin/bugreport.c\n+++ w/builtin/bugreport.c\n@@ -146,6 +146,11 @@ int cmd_bugreport(int argc, const char **argv, const char *prefix)\n \t\t report_path.buf);\n \t}\n \n+\tif (!startup_info->have_repository && diagnose != DIAGNOSE_NONE) {\n+\t\twarning(_(\"no repository--diagnostic output disabled\"));\n+\t\tdiagnose = DIAGNOSE_NONE;\n+\t}\n+\n \t/* Prepare diagnostics, if requested */\n \tif (diagnose != DIAGNOSE_NONE) {\n \t\tstruct strbuf zip_path = STRBUF_INIT;\n"},{"id":"483253","messageId":"CAP8UFD3z5xKfvao2Q87tK7y5P+2LSKW_Hv-dBvFb=J3gm7-Arg@mail.gmail.com","threadId":"60365","inReplyTo":"CAKFQ_Q9WjF9i-Rx2jdCw-adPVQrWNfNKrDY-em8Rpa5RNLXz4A@mail.gmail.com","subject":"Re: Bug: git diagnose crashes with Segmentation fault outside of git repository","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2023-10-14T17:22:38Z","receivedAt":"2023-10-14T17:22:53Z","isPatch":false,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Sat, Oct 14, 2023 at 2:00 PM ks1322 ks1322 wrote:\n>\n> Thank you for filling out a Git bug report!\n> Please answer the following questions to help us understand your issue.\n>\n> What did you do before the bug happened? (Steps to reproduce your issue)\n> Run `git diagnose` outside of any git repository\n>\n> What did you expect to happen? (Expected behavior)\n> No crash, reasonable diagnose output\n>\n> What happened instead? (Actual behavior)\n> `git diagnose` crashed with Segmentation fault\n>\n> $ git diagnose\n> Collecting diagnostic info\n>\n> git version 2.41.0\n> cpu: x86_64\n> no commit associated with this build\n> sizeof-long: 8\n> sizeof-size_t: 8\n> shell-path: /bin/sh\n> Repository root: (null)\n> Available space on '/tmp': 7.75 GiB (mount flags 0x6)\n> Segmentation fault (core dumped)\n\nYeah, this is a valid bug report that I can reproduce. It looks like\nsuch bugs have existed in this command since it was created last year.\n\nBest,\nChristian.\n"},{"id":"483486","messageId":"CAN0heSqmZ7QXJbet2Tp=YYCjBLToOHtNy+n=zcf29XYaukYN0w@mail.gmail.com","threadId":"60365","inReplyTo":"xmqq5y39unvc.fsf@gitster.g","subject":"Re: [PATCH] diagnose: require repository","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2023-10-19T13:18:45Z","receivedAt":"2023-10-19T13:18:58Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On Sat, 14 Oct 2023 at 19:15, Junio C Hamano wrote:\n>\n> Martin Ågren writes:\n>\n> > Switch from the gentle setup to requiring a git directory. Without a git\n> > repo, there isn't really much to diagnose.\n> >\n> > We could possibly do a best-effort collection of information about the\n> > machine and then give up. That would roughly be today's behavior but\n> > with a controlled exit rather than a segfault. However, the purpose of\n> > this tool is largely to create a zip archive. Rather than creating an\n> > empty zip file or no zip file at all, and having to explain that\n\nCorrecting myself: The zip archive would actually contain\n`diagnostics.log` with some general info about the machine and Git\nbuild.\n\n> > behavior, it seems more helpful to bail out clearly and early with a\n> > succinct error message.\n>\n> Without having thought things through, offhand I agree with your \"no\n> repository? there is nothing worth tarring up then\" assessment.\n>\n> Because \"git bugreport --diag\" unconditionally spawns \"git\n> diagnose\", the former may also want to be extra careful, perhaps\n> like the attached patch.\n\nGood point. TBH, I had no idea about `git bugreport --diagnose`.\n\n> + if (!startup_info->have_repository && diagnose != DIAGNOSE_NONE) {\n> + warning(_(\"no repository--diagnostic output disabled\"));\n> + diagnose = DIAGNOSE_NONE;\n> + }\n> +\n\nWhen the user explicitly provides that option, it seems unfortunate to\nme to drop it. Yes, we'd warn, but `git bugreport` then pops a text\neditor, so you would only see the warning after finishing up the report.\n(Maybe. By the time you quit your editor, you might not consider\nchecking the terminal for warnings and such.)\n\nSo I'm inclined to instead just die if we see the option outside a repo.\nIf `diagnose` the command fundamentally requires a repo (as with my\npatch) it seems surprising to me to not have `--diagnose` the option\nbehave the same.\n\nMartin\n"},{"id":"483513","messageId":"xmqqo7guwkla.fsf@gitster.g","threadId":"60365","inReplyTo":"CAN0heSqmZ7QXJbet2Tp=YYCjBLToOHtNy+n=zcf29XYaukYN0w@mail.gmail.com","subject":"Re: [PATCH] diagnose: require repository","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-10-19T18:09:05Z","receivedAt":"2023-10-19T18:09:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Martin Ågren writes:\n\n> Correcting myself: The zip archive would actually contain\n> `diagnostics.log` with some general info about the machine and Git\n> build.\n\nSo it could contain some useful information without a specific\nrepository, perhaps.\n\n> Good point. TBH, I had no idea about `git bugreport --diagnose`.\n\nYou are not alone ;-) I didn't, either. Before responding to your\npatch, that is.\n\n>> + if (!startup_info->have_repository && diagnose != DIAGNOSE_NONE) {\n>> + warning(_(\"no repository--diagnostic output disabled\"));\n>> + diagnose = DIAGNOSE_NONE;\n>> + }\n>> +\n>\n> When the user explicitly provides that option, it seems unfortunate to\n> me to drop it. Yes, we'd warn, but `git bugreport` then pops a text\n> editor, so you would only see the warning after finishing up the report.\n> (Maybe. By the time you quit your editor, you might not consider\n> checking the terminal for warnings and such.)\n>\n> So I'm inclined to instead just die if we see the option outside a repo.\n> If `diagnose` the command fundamentally requires a repo (as with my\n> patch) it seems surprising to me to not have `--diagnose` the option\n> behave the same.\n\nI have no strong opinion. Victoria is on Cc: already, whose name\nappears a lot more often than mine in the shortlog for \"diagnose\"\nstuff, so I'll defer to her area expertise.\n\nThanks.\n"},{"id":"483514","messageId":"01569dd1-f807-d56d-a123-5e8f3c930503@github.com","threadId":"60365","inReplyTo":"CAN0heSqmZ7QXJbet2Tp=YYCjBLToOHtNy+n=zcf29XYaukYN0w@mail.gmail.com","subject":"Re: [PATCH] diagnose: require repository","fromName":"Victoria Dye","fromEmail":"vdye@github.com","sentAt":"2023-10-19T18:16:38Z","receivedAt":"2023-10-19T18:16:42Z","isPatch":true,"sender":{"key":"vdye@github.com","avatar":"https://avatars.githubusercontent.com/u/3619353?v=4"},"body":"Martin Ågren wrote:\n>>> behavior, it seems more helpful to bail out clearly and early with a\n>>> succinct error message.\n>>\n>> Without having thought things through, offhand I agree with your \"no\n>> repository? there is nothing worth tarring up then\" assessment.\n>>\n>> Because \"git bugreport --diag\" unconditionally spawns \"git\n>> diagnose\", the former may also want to be extra careful, perhaps\n>> like the attached patch.\n> \n> Good point. TBH, I had no idea about `git bugreport --diagnose`.\n> \n>> + if (!startup_info->have_repository && diagnose != DIAGNOSE_NONE) {\n>> + warning(_(\"no repository--diagnostic output disabled\"));\n>> + diagnose = DIAGNOSE_NONE;\n>> + }\n>> +\n> \n> When the user explicitly provides that option, it seems unfortunate to\n> me to drop it. Yes, we'd warn, but `git bugreport` then pops a text\n> editor, so you would only see the warning after finishing up the report.\n> (Maybe. By the time you quit your editor, you might not consider\n> checking the terminal for warnings and such.)\n> \n> So I'm inclined to instead just die if we see the option outside a repo.\n> If `diagnose` the command fundamentally requires a repo (as with my\n> patch) it seems surprising to me to not have `--diagnose` the option\n> behave the same.\n\nI agree - it was an oversight on my part to not firmly require the existence\nof a repository with 'git diagnose', and the same applies to 'bugreport\n--diagnose'.\n\nFor reference, there is one other usage of 'git diagnose' (in 'scalar\ndiagnose'). However, it's already guarded by 'setup_git_directory()' so it\nshouldn't need to be updated.\n\n> \n> Martin\n\n"}]}