bugreport.c: fix a crash in `git bugreport` with `--no-suffix` option

From: Jiamu Sun <[email protected]>

executing git bugreport --no-suffix led to a segmentation fault due to strbuf_addftime() being called with a NULL option_suffix variable. This occurs because negating the "--[no-]suffix" option causes the parser to set option_suffix to NULL, which is not handled prior to calling strbuf_addftime().

---
bugreport.c: fix a crash in git bugreport with --no-suffix option

executing git bugreport --no-suffix led to a segmentation fault due to strbuf_addftime() being called with a NULL option_suffix variable. This occurs because negating the "--[no-]suffix" option causes the parser to set option_suffix to NULL, which is not handled prior to calling strbuf_addftime().

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1693%2Fbarroit%2Ffix-bugreport-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1693/barroit/fix-bugreport-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1693

 builtin/bugreport.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

index 3106e56a130..32281815b77 100644
@@ -138,8 +138,11 @@ int cmd_bugreport(int argc, const char **argv, const char *prefix)
 	strbuf_complete(&report_path, '/');
 	output_path_len = report_path.len;
 
-	strbuf_addstr(&report_path, "git-bugreport-");
-	strbuf_addftime(&report_path, option_suffix, localtime_r(&now, &tm), 0, 0);
+	strbuf_addstr(&report_path, "git-bugreport");
+	if (option_suffix) {
+		strbuf_addch(&report_path, '-');
+		strbuf_addftime(&report_path, option_suffix, localtime_r(&now, &tm), 0, 0);
+	}
 	strbuf_addstr(&report_path, ".txt");
 
 	switch (safe_create_leading_directories(report_path.buf)) {

base-commit: 945115026aa63df4ab849ab14a04da31623abece

gitgitgadget

"git blame" points at 238b439d (bugreport: add tool to generate debugging info, 2020-04-16) that is the very beginning of this tool, and the bug survived 4f6460df (builtin/bugreport.c: use thread-safe localtime_r(), 2020-11-30). Apparently neither commit considered "--suffix=<string>" would invite users to say "--no-suffix" (authors of them CC'ed for their input).

Perhaps we should update the documentation a bit while at it? Here is what I can find in its documentation.

    SYNOPSIS
    --------
    [verse]
    'git bugreport' [(-o | --output-directory) <path>] [(-s | --suffix) <format>]
                    [--diagnose[=<mode>]]

    -s <format>::
    --suffix <format>::
            Specify an alternate suffix for the bugreport name, to create a file
            named 'git-bugreport-<formatted-suffix>'. This should take the form of a
            strftime(3) format string; the current local time will be used.

The above does not say that it is possible to ask the code not to use suffix at all with "--no-suffix". If we want it to happen and behave sensibly (which I think the code with your patch does, from my cursory read), we probably should document it. At least two developers, considered to be expert Git developers and consider themselves to be expert Git users, did not even anticipate that "--no-suffix" will hit their code.

Another approach (with devil's advocate hat on) is obviously to use the PARSE_OPT_NONEG bit so that people won't do what hurts them ;-), but the fix is so trivial that it may be better to formally accept "--no-suffix" in this case.

An aside #leftoverbits is to find OPTION_STRING that is used without NONEG bit and make sure negating them with the "--no-" prefix will not trigger a similar issue. All uses of OPT_STRING() that use a variable that is initialized to a non-NULL string are suspect. Of course, this is #leftoverbits and must be kept outside the topic to fix this bug (i.e. this patch).

And such a documentation update may look like this. Feel free to use it in an updated version of the patch but please make sure it formats correctly (I didn't test it).

Thanks.

 Documentation/git-bugreport.txt | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

index ca626f7fc6..112658b3c3 100644
@@ -8,7 +8,8 @@ git-bugreport - Collect information for user to file a bug report
 SYNOPSIS
 --------
 [verse]
-'git bugreport' [(-o | --output-directory) <path>] [(-s | --suffix) <format>]
+'git bugreport' [(-o | --output-directory) <path>]
+		[(-s | --suffix) <format> | --no-suffix]
 		[--diagnose[=<mode>]]
 
 DESCRIPTION

@@ -51,9 +52,12 @@ OPTIONS
 
 -s <format>::
 --suffix <format>::
+--no-suffix::
 	Specify an alternate suffix for the bugreport name, to create a file
 	named 'git-bugreport-<formatted-suffix>'. This should take the form of a
 	strftime(3) format string; the current local time will be used.
+	`--no-suffix` disables the suffix and the file is just named
+	`git-bugreport` without any disambiguation measure.
 
 --no-diagnose::
 --diagnose[=<mode>]::

executing git bugreport --no-suffix led to a segmentation fault due to strbuf_addftime() being called with a NULL option_suffix variable. This occurs because negating the "--[no-]suffix" option causes the parser to set option_suffix to NULL, which is not handled prior to calling strbuf_addftime().

Jiamu Sun (2):
bugreport.c: fix a crash in git bugreport with --no-suffix option
doc: update doc file and usage for git-bugreport

 Documentation/git-bugreport.txt |  6 +++++-
 builtin/bugreport.c             | 10 +++++++---
 2 files changed, 12 insertions(+), 4 deletions(-)

base-commit: 945115026aa63df4ab849ab14a04da31623abece
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1693%2Fbarroit%2Ffix-bugreport-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1693/barroit/fix-bugreport-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1693

Range-diff vs v1:

 1:  9c6f3f5203a = 1:  9c6f3f5203a bugreport.c: fix a crash in `git bugreport` with `--no-suffix` option
 -:  ----------- > 2:  868cec05ed5 doc: update doc file and usage for git-bugreport

gitgitgadget

From: Jiamu Sun <[email protected]>

executing git bugreport --no-suffix led to a segmentation fault due to strbuf_addftime() being called with a NULL option_suffix variable. This occurs because negating the "--[no-]suffix" option causes the parser to set option_suffix to NULL, which is not handled prior to calling strbuf_addftime().

---
 builtin/bugreport.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

index 3106e56a130..32281815b77 100644
@@ -138,8 +138,11 @@ int cmd_bugreport(int argc, const char **argv, const char *prefix)
 	strbuf_complete(&report_path, '/');
 	output_path_len = report_path.len;
 
-	strbuf_addstr(&report_path, "git-bugreport-");
-	strbuf_addftime(&report_path, option_suffix, localtime_r(&now, &tm), 0, 0);
+	strbuf_addstr(&report_path, "git-bugreport");
+	if (option_suffix) {
+		strbuf_addch(&report_path, '-');
+		strbuf_addftime(&report_path, option_suffix, localtime_r(&now, &tm), 0, 0);
+	}
 	strbuf_addstr(&report_path, ".txt");
 
 	switch (safe_create_leading_directories(report_path.buf)) {

gitgitgadget

From: Jiamu Sun <[email protected]>

Changes since v1:
- Update documentation and usage string for git bugreport as suggested by Junio C Hamano

---
 Documentation/git-bugreport.txt | 6 +++++-
 builtin/bugreport.c             | 3 ++-
 2 files changed, 7 insertions(+), 2 deletions(-)

index ca626f7fc68..112658b3c3b 100644
@@ -8,7 +8,8 @@ git-bugreport - Collect information for user to file a bug report
 SYNOPSIS
 --------
 [verse]
-'git bugreport' [(-o | --output-directory) <path>] [(-s | --suffix) <format>]
+'git bugreport' [(-o | --output-directory) <path>]
+		[(-s | --suffix) <format> | --no-suffix]
 		[--diagnose[=<mode>]]
 
 DESCRIPTION

@@ -51,9 +52,12 @@ OPTIONS
 
 -s <format>::
 --suffix <format>::
+--no-suffix::
 	Specify an alternate suffix for the bugreport name, to create a file
 	named 'git-bugreport-<formatted-suffix>'. This should take the form of a
 	strftime(3) format string; the current local time will be used.
+	`--no-suffix` disables the suffix and the file is just named
+	`git-bugreport` without any disambiguation measure.
 
 --no-diagnose::
 --diagnose[=<mode>]::

index 32281815b77..25f860a0d97 100644
@@ -64,7 +64,8 @@ static void get_populated_hooks(struct strbuf *hook_info, int nongit)
 }
 
 static const char * const bugreport_usage[] = {
-	N_("git bugreport [(-o | --output-directory) <path>] [(-s | --suffix) <format>]\n"
+	N_("git bugreport [(-o | --output-directory) <path>]\n"
+	   "              [(-s | --suffix) <format> | --no-suffix]\n"
 	   "              [--diagnose[=<mode>]]"),
 	NULL
 };

gitgitgadget

Squash them together into a single patch. As you didn't have any meaningful log message in [2/2], unless there are other things that need to be updated and v3 is needed, I can squash them into one commit, though.

Thanks for updating.

I forgot the two I CC'ed the review thread for the previous round to ping them, so here it is.

Thanks.

executing git bugreport --no-suffix led to a segmentation fault due to strbuf_addftime() being called with a NULL option_suffix variable. This occurs because negating the "--[no-]suffix" option causes the parser to set option_suffix to NULL, which is not handled prior to calling strbuf_addftime().

By adding a NULL check, the --no-suffix option is now available. Using this option disables the suffix, and the file is just named git-bugreport without any disambiguation measure.

---
Changes since v2:
- Squashed the previous patch series into a single patch for clarity

 Documentation/git-bugreport.txt |  6 +++++-
 builtin/bugreport.c             | 10 +++++++---
 2 files changed, 12 insertions(+), 4 deletions(-)

index ca626f7fc6..112658b3c3 100644
@@ -8,7 +8,8 @@ git-bugreport - Collect information for user to file a bug report
 SYNOPSIS
 --------
 [verse]
-'git bugreport' [(-o | --output-directory) <path>] [(-s | --suffix) <format>]
+'git bugreport' [(-o | --output-directory) <path>]
+		[(-s | --suffix) <format> | --no-suffix]
 		[--diagnose[=<mode>]]
 
 DESCRIPTION

@@ -51,9 +52,12 @@ OPTIONS
 
 -s <format>::
 --suffix <format>::
+--no-suffix::
 	Specify an alternate suffix for the bugreport name, to create a file
 	named 'git-bugreport-<formatted-suffix>'. This should take the form of a
 	strftime(3) format string; the current local time will be used.
+	`--no-suffix` disables the suffix and the file is just named
+	`git-bugreport` without any disambiguation measure.
 
 --no-diagnose::
 --diagnose[=<mode>]::

index 3106e56a13..25f860a0d9 100644
@@ -64,7 +64,8 @@ static void get_populated_hooks(struct strbuf *hook_info, int nongit)
 }
 
 static const char * const bugreport_usage[] = {
-	N_("git bugreport [(-o | --output-directory) <path>] [(-s | --suffix) <format>]\n"
+	N_("git bugreport [(-o | --output-directory) <path>]\n"
+	   "              [(-s | --suffix) <format> | --no-suffix]\n"
 	   "              [--diagnose[=<mode>]]"),
 	NULL
 };

@@ -138,8 +139,11 @@ int cmd_bugreport(int argc, const char **argv, const char *prefix)
 	strbuf_complete(&report_path, '/');
 	output_path_len = report_path.len;
 
-	strbuf_addstr(&report_path, "git-bugreport-");
-	strbuf_addftime(&report_path, option_suffix, localtime_r(&now, &tm), 0, 0);
+	strbuf_addstr(&report_path, "git-bugreport");
+	if (option_suffix) {
+		strbuf_addch(&report_path, '-');
+		strbuf_addftime(&report_path, option_suffix, localtime_r(&now, &tm), 0, 0);
+	}
 	strbuf_addstr(&report_path, ".txt");
 
 	switch (safe_create_leading_directories(report_path.buf)) {

2.44.GIT

I've updated patch 3; this should hopefully be fine now.

Sorry for the trouble, and thanks for all your help, especially as it's my first PR.

I can't speak for 238b439d, but at least in the case of 4f6460df, the conversion was purely about changing localtime() to localtime_r(), and nothing more.

The commit message indicates that I was blindly grepping around for 'localtime\(_.\)\?' without looking too much at the surrounding context.

Thanks,
Taylor

Thanks,
Taylor