Re: [PATCH] bugreport.c: fix a crash in `git bugreport` with `--no-suffix` option

[Junio C Hamano <gitster@pobox.com>]

"barroit via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Jiamu Sun <barroit@linux.com>
>
> 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().
>
> Signed-off-by: Jiamu Sun <barroit@linux.com>
> ---

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

>     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(-)
>
> diff --git a/builtin/bugreport.c b/builtin/bugreport.c
> index 3106e56a130..32281815b77 100644
> --- a/builtin/bugreport.c
> +++ b/builtin/bugreport.c
> @@ -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