{"thread":{"id":"61100","subject":"[PATCH] bugreport.c: fix a crash in `git bugreport` with `--no-suffix` option","startedAt":"2024-03-12T16:26:55Z","lastAt":"2024-03-16T01:56:36Z","messageCount":12,"participants":["barroit via GitGitGadget","Junio C Hamano","Jiamu Sun via GitGitGadget","Jiamu Sun","Taylor Blau"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"490493","messageId":"pull.1693.git.1710260812280.gitgitgadget@gmail.com","threadId":"61100","inReplyTo":null,"subject":"[PATCH] bugreport.c: fix a crash in `git bugreport` with `--no-suffix` option","fromName":"barroit via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-03-12T16:26:52Z","receivedAt":"2024-03-12T16:26:55Z","isPatch":true,"sender":{"key":"name:barroit","avatar":null},"body":"From: Jiamu Sun \n\nexecuting `git bugreport --no-suffix` led to a segmentation fault\ndue to strbuf_addftime() being called with a NULL option_suffix\nvariable. This occurs because negating the \"--[no-]suffix\" option\ncauses the parser to set option_suffix to NULL, which is not\nhandled prior to calling strbuf_addftime().\n\nSigned-off-by: Jiamu Sun \n---\n bugreport.c: fix a crash in git bugreport with --no-suffix option\n \n executing git bugreport --no-suffix led to a segmentation fault due to\n strbuf_addftime() being called with a NULL option_suffix variable. This\n occurs because negating the \"--[no-]suffix\" option causes the parser to\n set option_suffix to NULL, which is not handled prior to calling\n strbuf_addftime().\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1693%2Fbarroit%2Ffix-bugreport-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1693/barroit/fix-bugreport-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1693\n\n builtin/bugreport.c | 7 +++++--\n 1 file changed, 5 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/bugreport.c b/builtin/bugreport.c\nindex 3106e56a130..32281815b77 100644\n--- a/builtin/bugreport.c\n+++ b/builtin/bugreport.c\n@@ -138,8 +138,11 @@ int cmd_bugreport(int argc, const char **argv, const char *prefix)\n \tstrbuf_complete(&report_path, '/');\n \toutput_path_len = report_path.len;\n \n-\tstrbuf_addstr(&report_path, \"git-bugreport-\");\n-\tstrbuf_addftime(&report_path, option_suffix, localtime_r(&now, &tm), 0, 0);\n+\tstrbuf_addstr(&report_path, \"git-bugreport\");\n+\tif (option_suffix) {\n+\t\tstrbuf_addch(&report_path, '-');\n+\t\tstrbuf_addftime(&report_path, option_suffix, localtime_r(&now, &tm), 0, 0);\n+\t}\n \tstrbuf_addstr(&report_path, \".txt\");\n \n \tswitch (safe_create_leading_directories(report_path.buf)) {\n\nbase-commit: 945115026aa63df4ab849ab14a04da31623abece\n-- \ngitgitgadget\n"},{"id":"490548","messageId":"xmqqle6mdsw7.fsf@gitster.g","threadId":"61100","inReplyTo":"pull.1693.git.1710260812280.gitgitgadget@gmail.com","subject":"Re: [PATCH] bugreport.c: fix a crash in `git bugreport` with `--no-suffix` option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-03-13T15:59:52Z","receivedAt":"2024-03-13T15:59:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"barroit via GitGitGadget\" writes:\n\n> From: Jiamu Sun \n>\n> executing `git bugreport --no-suffix` led to a segmentation fault\n> due to strbuf_addftime() being called with a NULL option_suffix\n> variable. This occurs because negating the \"--[no-]suffix\" option\n> causes the parser to set option_suffix to NULL, which is not\n> handled prior to calling strbuf_addftime().\n>\n> Signed-off-by: Jiamu Sun \n> ---\n\n\"git blame\" points at 238b439d (bugreport: add tool to generate\ndebugging info, 2020-04-16) that is the very beginning of this tool,\nand the bug survived 4f6460df (builtin/bugreport.c: use thread-safe\nlocaltime_r(), 2020-11-30). Apparently neither commit considered\n\"--suffix=\" would invite users to say \"--no-suffix\" (authors\nof them CC'ed for their input).\n\nPerhaps we should update the documentation a bit while at it? Here\nis what I can find in its documentation.\n\n SYNOPSIS\n --------\n [verse]\n 'git bugreport' [(-o | --output-directory) ] [(-s | --suffix) ]\n [--diagnose[=]]\n\n -s ::\n --suffix ::\n Specify an alternate suffix for the bugreport name, to create a file\n named 'git-bugreport-'. This should take the form of a\n strftime(3) format string; the current local time will be used.\n\nThe above does not say that it is possible to ask the code not to\nuse suffix at all with \"--no-suffix\". If we want it to happen and\nbehave sensibly (which I think the code with your patch does, from\nmy cursory read), we probably should document it. At least two\ndevelopers, considered to be expert Git developers and consider\nthemselves to be expert Git users, did not even anticipate that\n\"--no-suffix\" will hit their code.\n\nAnother approach (with devil's advocate hat on) is obviously to use\nthe PARSE_OPT_NONEG bit so that people won't do what hurts them ;-),\nbut the fix is so trivial that it may be better to formally accept\n\"--no-suffix\" in this case.\n\nAn aside #leftoverbits is to find OPTION_STRING that is used without\nNONEG bit and make sure negating them with the \"--no-\" prefix will\nnot trigger a similar issue. All uses of OPT_STRING() that use a\nvariable that is initialized to a non-NULL string are suspect. Of\ncourse, this is #leftoverbits and must be kept outside the topic to\nfix this bug (i.e. this patch).\n\n> bugreport.c: fix a crash in git bugreport with --no-suffix option\n> \n> executing git bugreport --no-suffix led to a segmentation fault due to\n> strbuf_addftime() being called with a NULL option_suffix variable. This\n> occurs because negating the \"--[no-]suffix\" option causes the parser to\n> set option_suffix to NULL, which is not handled prior to calling\n> strbuf_addftime().\n>\n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1693%2Fbarroit%2Ffix-bugreport-v1\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1693/barroit/fix-bugreport-v1\n> Pull-Request: https://github.com/gitgitgadget/git/pull/1693\n>\n> builtin/bugreport.c | 7 +++++--\n> 1 file changed, 5 insertions(+), 2 deletions(-)\n>\n> diff --git a/builtin/bugreport.c b/builtin/bugreport.c\n> index 3106e56a130..32281815b77 100644\n> --- a/builtin/bugreport.c\n> +++ b/builtin/bugreport.c\n> @@ -138,8 +138,11 @@ int cmd_bugreport(int argc, const char **argv, const char *prefix)\n> \tstrbuf_complete(&report_path, '/');\n> \toutput_path_len = report_path.len;\n> \n> -\tstrbuf_addstr(&report_path, \"git-bugreport-\");\n> -\tstrbuf_addftime(&report_path, option_suffix, localtime_r(&now, &tm), 0, 0);\n> +\tstrbuf_addstr(&report_path, \"git-bugreport\");\n> +\tif (option_suffix) {\n> +\t\tstrbuf_addch(&report_path, '-');\n> +\t\tstrbuf_addftime(&report_path, option_suffix, localtime_r(&now, &tm), 0, 0);\n> +\t}\n> \tstrbuf_addstr(&report_path, \".txt\");\n> \n> \tswitch (safe_create_leading_directories(report_path.buf)) {\n>\n> base-commit: 945115026aa63df4ab849ab14a04da31623abece\n"},{"id":"490559","messageId":"xmqqmsr2c9jw.fsf@gitster.g","threadId":"61100","inReplyTo":"xmqqle6mdsw7.fsf@gitster.g","subject":"Re: [PATCH] bugreport.c: fix a crash in `git bugreport` with `--no-suffix` option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-03-13T17:42:59Z","receivedAt":"2024-03-13T17:43:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Perhaps we should update the documentation a bit while at it? Here\n> is what I can find in its documentation.\n> ...\n> The above does not say that it is possible to ask the code not to\n> use suffix at all with \"--no-suffix\". If we want it to happen and\n> behave sensibly (which I think the code with your patch does, from\n> my cursory read), we probably should document it. At least two\n> developers, considered to be expert Git developers and consider\n> themselves to be expert Git users, did not even anticipate that\n> \"--no-suffix\" will hit their code.\n\nAnd such a documentation update may look like this. Feel free to\nuse it in an updated version of the patch but please make sure it\nformats correctly (I didn't test it).\n\nThanks.\n\n Documentation/git-bugreport.txt | 6 +++++-\n 1 file changed, 5 insertions(+), 1 deletion(-)\n\ndiff --git c/Documentation/git-bugreport.txt w/Documentation/git-bugreport.txt\nindex ca626f7fc6..112658b3c3 100644\n--- c/Documentation/git-bugreport.txt\n+++ w/Documentation/git-bugreport.txt\n@@ -8,7 +8,8 @@ git-bugreport - Collect information for user to file a bug report\n SYNOPSIS\n --------\n [verse]\n-'git bugreport' [(-o | --output-directory) ] [(-s | --suffix) ]\n+'git bugreport' [(-o | --output-directory) ]\n+\t\t[(-s | --suffix) | --no-suffix]\n \t\t[--diagnose[=]]\n \n DESCRIPTION\n@@ -51,9 +52,12 @@ OPTIONS\n \n -s ::\n --suffix ::\n+--no-suffix::\n \tSpecify an alternate suffix for the bugreport name, to create a file\n \tnamed 'git-bugreport-'. This should take the form of a\n \tstrftime(3) format string; the current local time will be used.\n+\t`--no-suffix` disables the suffix and the file is just named\n+\t`git-bugreport` without any disambiguation measure.\n \n --no-diagnose::\n --diagnose[=]::\n"},{"id":"490604","messageId":"pull.1693.v2.git.1710388817.gitgitgadget@gmail.com","threadId":"61100","inReplyTo":"pull.1693.git.1710260812280.gitgitgadget@gmail.com","subject":"[PATCH v2 0/2] bugreport.c: fix a crash in git bugreport with --no-suffix option","fromName":"barroit via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-03-14T04:00:15Z","receivedAt":"2024-03-14T04:00:21Z","isPatch":true,"sender":{"key":"name:barroit","avatar":null},"body":"executing git bugreport --no-suffix led to a segmentation fault due to\nstrbuf_addftime() being called with a NULL option_suffix variable. This\noccurs because negating the \"--[no-]suffix\" option causes the parser to set\noption_suffix to NULL, which is not handled prior to calling\nstrbuf_addftime().\n\nJiamu Sun (2):\n bugreport.c: fix a crash in `git bugreport` with `--no-suffix` option\n doc: update doc file and usage for git-bugreport\n\n Documentation/git-bugreport.txt | 6 +++++-\n builtin/bugreport.c | 10 +++++++---\n 2 files changed, 12 insertions(+), 4 deletions(-)\n\n\nbase-commit: 945115026aa63df4ab849ab14a04da31623abece\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1693%2Fbarroit%2Ffix-bugreport-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1693/barroit/fix-bugreport-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/1693\n\nRange-diff vs v1:\n\n 1: 9c6f3f5203a = 1: 9c6f3f5203a bugreport.c: fix a crash in `git bugreport` with `--no-suffix` option\n -: ----------- > 2: 868cec05ed5 doc: update doc file and usage for git-bugreport\n\n-- \ngitgitgadget\n"},{"id":"490605","messageId":"9c6f3f5203ae26c501a5711e2610573130bfd550.1710388817.git.gitgitgadget@gmail.com","threadId":"61100","inReplyTo":"pull.1693.v2.git.1710388817.gitgitgadget@gmail.com","subject":"[PATCH v2 1/2] bugreport.c: fix a crash in `git bugreport` with `--no-suffix` option","fromName":"Jiamu Sun via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-03-14T04:00:16Z","receivedAt":"2024-03-14T04:00:21Z","isPatch":true,"sender":{"key":"39@barroit.sh","avatar":null},"body":"From: Jiamu Sun \n\nexecuting `git bugreport --no-suffix` led to a segmentation fault\ndue to strbuf_addftime() being called with a NULL option_suffix\nvariable. This occurs because negating the \"--[no-]suffix\" option\ncauses the parser to set option_suffix to NULL, which is not\nhandled prior to calling strbuf_addftime().\n\nSigned-off-by: Jiamu Sun \n---\n builtin/bugreport.c | 7 +++++--\n 1 file changed, 5 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/bugreport.c b/builtin/bugreport.c\nindex 3106e56a130..32281815b77 100644\n--- a/builtin/bugreport.c\n+++ b/builtin/bugreport.c\n@@ -138,8 +138,11 @@ int cmd_bugreport(int argc, const char **argv, const char *prefix)\n \tstrbuf_complete(&report_path, '/');\n \toutput_path_len = report_path.len;\n \n-\tstrbuf_addstr(&report_path, \"git-bugreport-\");\n-\tstrbuf_addftime(&report_path, option_suffix, localtime_r(&now, &tm), 0, 0);\n+\tstrbuf_addstr(&report_path, \"git-bugreport\");\n+\tif (option_suffix) {\n+\t\tstrbuf_addch(&report_path, '-');\n+\t\tstrbuf_addftime(&report_path, option_suffix, localtime_r(&now, &tm), 0, 0);\n+\t}\n \tstrbuf_addstr(&report_path, \".txt\");\n \n \tswitch (safe_create_leading_directories(report_path.buf)) {\n-- \ngitgitgadget\n\n"},{"id":"490606","messageId":"868cec05ed53e780660d98c08b5a306af3cc363e.1710388817.git.gitgitgadget@gmail.com","threadId":"61100","inReplyTo":"pull.1693.v2.git.1710388817.gitgitgadget@gmail.com","subject":"[PATCH v2 2/2] doc: update doc file and usage for git-bugreport","fromName":"Jiamu Sun via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-03-14T04:00:17Z","receivedAt":"2024-03-14T04:00:23Z","isPatch":true,"sender":{"key":"39@barroit.sh","avatar":null},"body":"From: Jiamu Sun \n\nChanges since v1:\n- Update documentation and usage string for `git bugreport` as\n suggested by Junio C Hamano\n\nSigned-off-by: Jiamu Sun \n---\n Documentation/git-bugreport.txt | 6 +++++-\n builtin/bugreport.c | 3 ++-\n 2 files changed, 7 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/git-bugreport.txt b/Documentation/git-bugreport.txt\nindex ca626f7fc68..112658b3c3b 100644\n--- a/Documentation/git-bugreport.txt\n+++ b/Documentation/git-bugreport.txt\n@@ -8,7 +8,8 @@ git-bugreport - Collect information for user to file a bug report\n SYNOPSIS\n --------\n [verse]\n-'git bugreport' [(-o | --output-directory) ] [(-s | --suffix) ]\n+'git bugreport' [(-o | --output-directory) ]\n+\t\t[(-s | --suffix) | --no-suffix]\n \t\t[--diagnose[=]]\n \n DESCRIPTION\n@@ -51,9 +52,12 @@ OPTIONS\n \n -s ::\n --suffix ::\n+--no-suffix::\n \tSpecify an alternate suffix for the bugreport name, to create a file\n \tnamed 'git-bugreport-'. This should take the form of a\n \tstrftime(3) format string; the current local time will be used.\n+\t`--no-suffix` disables the suffix and the file is just named\n+\t`git-bugreport` without any disambiguation measure.\n \n --no-diagnose::\n --diagnose[=]::\ndiff --git a/builtin/bugreport.c b/builtin/bugreport.c\nindex 32281815b77..25f860a0d97 100644\n--- a/builtin/bugreport.c\n+++ b/builtin/bugreport.c\n@@ -64,7 +64,8 @@ static void get_populated_hooks(struct strbuf *hook_info, int nongit)\n }\n \n static const char * const bugreport_usage[] = {\n-\tN_(\"git bugreport [(-o | --output-directory) ] [(-s | --suffix) ]\\n\"\n+\tN_(\"git bugreport [(-o | --output-directory) ]\\n\"\n+\t \" [(-s | --suffix) | --no-suffix]\\n\"\n \t \" [--diagnose[=]]\"),\n \tNULL\n };\n-- \ngitgitgadget\n"},{"id":"490622","messageId":"xmqqbk7gye0s.fsf@gitster.g","threadId":"61100","inReplyTo":"pull.1693.v2.git.1710388817.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 0/2] bugreport.c: fix a crash in git bugreport with --no-suffix option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-03-14T16:27:47Z","receivedAt":"2024-03-14T16:27:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"barroit via GitGitGadget\" writes:\n\n> executing git bugreport --no-suffix led to a segmentation fault due to\n> strbuf_addftime() being called with a NULL option_suffix variable. This\n> occurs because negating the \"--[no-]suffix\" option causes the parser to set\n> option_suffix to NULL, which is not handled prior to calling\n> strbuf_addftime().\n>\n> Jiamu Sun (2):\n> bugreport.c: fix a crash in `git bugreport` with `--no-suffix` option\n> doc: update doc file and usage for git-bugreport\n\nSquash them together into a single patch. As you didn't have any\nmeaningful log message in [2/2], unless there are other things that\nneed to be updated and v3 is needed, I can squash them into one\ncommit, though.\n\nThanks for updating.\n"},{"id":"490623","messageId":"xmqq4jd8ydqz.fsf@gitster.g","threadId":"61100","inReplyTo":"xmqqbk7gye0s.fsf@gitster.g","subject":"Re: [PATCH v2 0/2] bugreport.c: fix a crash in git bugreport with --no-suffix option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-03-14T16:33:40Z","receivedAt":"2024-03-14T16:33:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> \"barroit via GitGitGadget\" writes:\n>\n>> executing git bugreport --no-suffix led to a segmentation fault due to\n>> strbuf_addftime() being called with a NULL option_suffix variable. This\n>> occurs because negating the \"--[no-]suffix\" option causes the parser to set\n>> option_suffix to NULL, which is not handled prior to calling\n>> strbuf_addftime().\n>>\n>> Jiamu Sun (2):\n>> bugreport.c: fix a crash in `git bugreport` with `--no-suffix` option\n>> doc: update doc file and usage for git-bugreport\n>\n> Squash them together into a single patch. As you didn't have any\n> meaningful log message in [2/2], unless there are other things that\n> need to be updated and v3 is needed, I can squash them into one\n> commit, though.\n>\n> Thanks for updating.\n\nI forgot the two I CC'ed the review thread for the previous round to\nping them, so here it is.\n\nThanks.\n"},{"id":"490647","messageId":"20240314223406.79283-1-barroit@linux.com","threadId":"61100","inReplyTo":"pull.1693.v2.git.1710388817.gitgitgadget@gmail.com","subject":"[PATCH v3] bugreport.c: fix a crash in `git bugreport` with `--no-suffix` option","fromName":"Jiamu Sun","fromEmail":"baiorettohr@gmail.com","sentAt":"2024-03-14T22:34:06Z","receivedAt":"2024-03-14T22:34:38Z","isPatch":true,"sender":{"key":"baiorettohr@gmail.com","avatar":null},"body":"executing `git bugreport --no-suffix` led to a segmentation fault\ndue to strbuf_addftime() being called with a NULL option_suffix\nvariable. This occurs because negating the \"--[no-]suffix\" option\ncauses the parser to set option_suffix to NULL, which is not\nhandled prior to calling strbuf_addftime().\n\nBy adding a NULL check, the `--no-suffix` option is now available.\nUsing this option disables the suffix, and the file is just named\n`git-bugreport` without any disambiguation measure.\n\nSigned-off-by: Jiamu Sun \n---\nChanges since v2:\n- Squashed the previous patch series into a single patch for\n clarity\n\n Documentation/git-bugreport.txt | 6 +++++-\n builtin/bugreport.c | 10 +++++++---\n 2 files changed, 12 insertions(+), 4 deletions(-)\n\ndiff --git a/Documentation/git-bugreport.txt b/Documentation/git-bugreport.txt\nindex ca626f7fc6..112658b3c3 100644\n--- a/Documentation/git-bugreport.txt\n+++ b/Documentation/git-bugreport.txt\n@@ -8,7 +8,8 @@ git-bugreport - Collect information for user to file a bug report\n SYNOPSIS\n --------\n [verse]\n-'git bugreport' [(-o | --output-directory) ] [(-s | --suffix) ]\n+'git bugreport' [(-o | --output-directory) ]\n+\t\t[(-s | --suffix) | --no-suffix]\n \t\t[--diagnose[=]]\n \n DESCRIPTION\n@@ -51,9 +52,12 @@ OPTIONS\n \n -s ::\n --suffix ::\n+--no-suffix::\n \tSpecify an alternate suffix for the bugreport name, to create a file\n \tnamed 'git-bugreport-'. This should take the form of a\n \tstrftime(3) format string; the current local time will be used.\n+\t`--no-suffix` disables the suffix and the file is just named\n+\t`git-bugreport` without any disambiguation measure.\n \n --no-diagnose::\n --diagnose[=]::\ndiff --git a/builtin/bugreport.c b/builtin/bugreport.c\nindex 3106e56a13..25f860a0d9 100644\n--- a/builtin/bugreport.c\n+++ b/builtin/bugreport.c\n@@ -64,7 +64,8 @@ static void get_populated_hooks(struct strbuf *hook_info, int nongit)\n }\n \n static const char * const bugreport_usage[] = {\n-\tN_(\"git bugreport [(-o | --output-directory) ] [(-s | --suffix) ]\\n\"\n+\tN_(\"git bugreport [(-o | --output-directory) ]\\n\"\n+\t \" [(-s | --suffix) | --no-suffix]\\n\"\n \t \" [--diagnose[=]]\"),\n \tNULL\n };\n@@ -138,8 +139,11 @@ int cmd_bugreport(int argc, const char **argv, const char *prefix)\n \tstrbuf_complete(&report_path, '/');\n \toutput_path_len = report_path.len;\n \n-\tstrbuf_addstr(&report_path, \"git-bugreport-\");\n-\tstrbuf_addftime(&report_path, option_suffix, localtime_r(&now, &tm), 0, 0);\n+\tstrbuf_addstr(&report_path, \"git-bugreport\");\n+\tif (option_suffix) {\n+\t\tstrbuf_addch(&report_path, '-');\n+\t\tstrbuf_addftime(&report_path, option_suffix, localtime_r(&now, &tm), 0, 0);\n+\t}\n \tstrbuf_addstr(&report_path, \".txt\");\n \n \tswitch (safe_create_leading_directories(report_path.buf)) {\n-- \n2.44.GIT\n\n"},{"id":"490745","messageId":"20240315224224.27182-1-barroit@linux.com","threadId":"61100","inReplyTo":"xmqq4jd8ydqz.fsf@gitster.g","subject":"Re: [PATCH v3] bugreport.c: fix a crash in git bugreport with --no-suffix option","fromName":"Jiamu Sun","fromEmail":"baiorettohr@gmail.com","sentAt":"2024-03-15T22:42:24Z","receivedAt":"2024-03-15T22:43:24Z","isPatch":true,"sender":{"key":"baiorettohr@gmail.com","avatar":null},"body":"Jiamu Sun writes:\n\n> Junio C Hamano writes:\n>\n> I forgot the two I CC'ed the review thread for the previous round to\n> ping them, so here it is.\n>\n> Thanks.\n\nI've updated patch 3; this should hopefully be fine now.\n\nSorry for the trouble, and thanks for all your help, especially as\nit's my first PR.\n"},{"id":"490753","messageId":"ZfT8I3eoU0dBQMsv@nand.local","threadId":"61100","inReplyTo":"xmqqle6mdsw7.fsf@gitster.g","subject":"Re: [PATCH] bugreport.c: fix a crash in `git bugreport` with `--no-suffix` option","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-03-16T01:55:47Z","receivedAt":"2024-03-16T01:55:49Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, Mar 13, 2024 at 08:59:52AM -0700, Junio C Hamano wrote:\n> \"barroit via GitGitGadget\" writes:\n>\n> > From: Jiamu Sun \n> >\n> > executing `git bugreport --no-suffix` led to a segmentation fault\n> > due to strbuf_addftime() being called with a NULL option_suffix\n> > variable. This occurs because negating the \"--[no-]suffix\" option\n> > causes the parser to set option_suffix to NULL, which is not\n> > handled prior to calling strbuf_addftime().\n> >\n> > Signed-off-by: Jiamu Sun \n> > ---\n>\n> \"git blame\" points at 238b439d (bugreport: add tool to generate\n> debugging info, 2020-04-16) that is the very beginning of this tool,\n> and the bug survived 4f6460df (builtin/bugreport.c: use thread-safe\n> localtime_r(), 2020-11-30). Apparently neither commit considered\n> \"--suffix=\" would invite users to say \"--no-suffix\" (authors\n> of them CC'ed for their input).\n\nI can't speak for 238b439d, but at least in the case of 4f6460df, the\nconversion was purely about changing localtime() to localtime_r(), and\nnothing more.\n\nThe commit message indicates that I was blindly grepping around for\n'localtime\\(_.\\)\\?' without looking too much at the surrounding context.\n\nThanks,\nTaylor\n"},{"id":"490754","messageId":"ZfT8UiDM3DQhRX5x@nand.local","threadId":"61100","inReplyTo":"20240314223406.79283-1-barroit@linux.com","subject":"Re: [PATCH v3] bugreport.c: fix a crash in `git bugreport` with `--no-suffix` option","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-03-16T01:56:34Z","receivedAt":"2024-03-16T01:56:36Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Fri, Mar 15, 2024 at 07:34:06AM +0900, Jiamu Sun wrote:\n> executing `git bugreport --no-suffix` led to a segmentation fault\n> due to strbuf_addftime() being called with a NULL option_suffix\n> variable. This occurs because negating the \"--[no-]suffix\" option\n> causes the parser to set option_suffix to NULL, which is not\n> handled prior to calling strbuf_addftime().\n>\n> By adding a NULL check, the `--no-suffix` option is now available.\n> Using this option disables the suffix, and the file is just named\n> `git-bugreport` without any disambiguation measure.\n>\n> Signed-off-by: Jiamu Sun \n> ---\n\n Acked-by: Taylor Blau \n\nThanks,\nTaylor\n"}]}