Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 1 commit into from

Conversation

barroit
Copy link

@barroit barroit commented Mar 12, 2024

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

cc: Jiamu Sun baiorettohr@gmail.com
cc: Taylor Blau me@ttaylorr.com

Copy link

gitgitgadget bot commented Mar 12, 2024

Welcome to GitGitGadget

Hi @barroit, and welcome to GitGitGadget, the GitHub App to send patch series to the Git mailing list from GitHub Pull Requests.

Please make sure that either:

  • Your Pull Request has a good description, if it consists of multiple commits, as it will be used as cover letter.
  • Your Pull Request description is empty, if it consists of a single commit, as the commit message should be descriptive enough by itself.

You can CC potential reviewers by adding a footer to the PR description with the following syntax:

CC: Revi Ewer <revi.ewer@example.com>, Ill Takalook <ill.takalook@example.net>

Also, it is a good idea to review the commit messages one last time, as the Git project expects them in a quite specific form:

  • the lines should not exceed 76 columns,
  • the first line should be like a header and typically start with a prefix like "tests:" or "revisions:" to state which subsystem the change is about, and
  • the commit messages' body should be describing the "why?" of the change.
  • Finally, the commit messages should end in a Signed-off-by: line matching the commits' author.

It is in general a good idea to await the automated test ("Checks") in this Pull Request before contributing the patches, e.g. to avoid trivial issues such as unportable code.

Contributing the patches

Before you can contribute the patches, your GitHub username needs to be added to the list of permitted users. Any already-permitted user can do that, by adding a comment to your PR of the form /allow. A good way to find other contributors is to locate recent pull requests where someone has been /allowed:

Both the person who commented /allow and the PR author are able to /allow you.

An alternative is the channel #git-devel on the Libera Chat IRC network:

<newcontributor> I've just created my first PR, could someone please /allow me? https://github.com/gitgitgadget/git/pull/12345
<veteran> newcontributor: it is done
<newcontributor> thanks!

Once on the list of permitted usernames, you can contribute the patches to the Git mailing list by adding a PR comment /submit.

If you want to see what email(s) would be sent for a /submit request, add a PR comment /preview to have the email(s) sent to you. You must have a public GitHub email address for this. Note that any reviewers CC'd via the list in the PR description will not actually be sent emails.

After you submit, GitGitGadget will respond with another comment that contains the link to the cover letter mail in the Git mailing list archive. Please make sure to monitor the discussion in that thread and to address comments and suggestions (while the comments and suggestions will be mirrored into the PR by GitGitGadget, you will still want to reply via mail).

If you do not want to subscribe to the Git mailing list just to be able to respond to a mail, you can download the mbox from the Git mailing list archive (click the (raw) link), then import it into your mail program. If you use GMail, you can do this via:

curl -g --user "<EMailAddress>:<Password>" \
    --url "imaps://imap.gmail.com/INBOX" -T /path/to/raw.txt

To iterate on your change, i.e. send a revised patch or patch series, you will first want to (force-)push to the same branch. You probably also want to modify your Pull Request description (or title). It is a good idea to summarize the revision by adding something like this to the cover letter (read: by editing the first comment on the PR, i.e. the PR description):

Changes since v1:
- Fixed a typo in the commit message (found by ...)
- Added a code comment to ... as suggested by ...
...

To send a new iteration, just add another PR comment with the contents: /submit.

Need help?

New contributors who want advice are encouraged to join git-mentoring@googlegroups.com, where volunteers who regularly contribute to Git are willing to answer newbie questions, give advice, or otherwise provide mentoring to interested contributors. You must join in order to post or view messages, but anyone can join.

You may also be able to find help in real time in the developer IRC channel, #git-devel on Libera Chat. Remember that IRC does not support offline messaging, so if you send someone a private message and log out, they cannot respond to you. The scrollback of #git-devel is archived, though.

@Ikke
Copy link

Ikke commented Mar 12, 2024

/allow

Copy link

gitgitgadget bot commented Mar 12, 2024

User barroit is now allowed to use GitGitGadget.

@barroit
Copy link
Author

barroit commented Mar 12, 2024

/preview

Copy link

gitgitgadget bot commented Mar 12, 2024

Preview email sent as pull.1693.git.1710260038446.gitgitgadget@gmail.com

@barroit
Copy link
Author

barroit commented Mar 12, 2024

/submit

Copy link

gitgitgadget bot commented Mar 12, 2024

Submitted as pull.1693.git.1710260812280.gitgitgadget@gmail.com

To fetch this version into FETCH_HEAD:

git fetch https://github.com/gitgitgadget/git/ pr-1693/barroit/fix-bugreport-v1

To fetch this version to local tag pr-1693/barroit/fix-bugreport-v1:

git fetch --no-tags https://github.com/gitgitgadget/git/ tag pr-1693/barroit/fix-bugreport-v1

Copy link

gitgitgadget bot commented Mar 13, 2024

On the Git mailing list, Junio C Hamano wrote (reply to this):

"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

Copy link

gitgitgadget bot commented Mar 13, 2024

On the Git mailing list, Junio C Hamano wrote (reply to this):

Junio C Hamano <gitster@pobox.com> writes:

> Perhaps we should update the documentation a bit while at it?  Here
> is what I can find in its documentation.
> ...
> 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.

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(-)

diff --git c/Documentation/git-bugreport.txt w/Documentation/git-bugreport.txt
index ca626f7fc6..112658b3c3 100644
--- c/Documentation/git-bugreport.txt
+++ w/Documentation/git-bugreport.txt
@@ -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>]::

Copy link

gitgitgadget bot commented Mar 14, 2024

This branch is now known as js/bugreport-no-suffix-fix.

Copy link

gitgitgadget bot commented Mar 14, 2024

This patch series was integrated into seen via git@b748578.

@gitgitgadget gitgitgadget bot added the seen label Mar 14, 2024
@barroit
Copy link
Author

barroit commented Mar 14, 2024

/preview

Copy link

gitgitgadget bot commented Mar 14, 2024

Preview email sent as pull.1693.v2.git.1710388601.gitgitgadget@gmail.com

@barroit
Copy link
Author

barroit commented Mar 14, 2024

/submit

Copy link

gitgitgadget bot commented Mar 14, 2024

Submitted as pull.1693.v2.git.1710388817.gitgitgadget@gmail.com

To fetch this version into FETCH_HEAD:

git fetch https://github.com/gitgitgadget/git/ pr-1693/barroit/fix-bugreport-v2

To fetch this version to local tag pr-1693/barroit/fix-bugreport-v2:

git fetch --no-tags https://github.com/gitgitgadget/git/ tag pr-1693/barroit/fix-bugreport-v2

@briantracy
Copy link

Hello, I am a new git contributor. Could you please /allow my PR: #1692

Copy link

gitgitgadget bot commented Mar 14, 2024

On the Git mailing list, Junio C Hamano wrote (reply to this):

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

> 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

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.

Copy link

gitgitgadget bot commented Mar 14, 2024

On the Git mailing list, Junio C Hamano wrote (reply to this):

Junio C Hamano <gitster@pobox.com> writes:

> "barroit via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
>> 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
>
> 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.

@barroit
Copy link
Author

barroit commented Mar 14, 2024

/preview

Copy link

gitgitgadget bot commented Mar 14, 2024

Preview email sent as pull.1693.v3.git.1710441878359.gitgitgadget@gmail.com

@barroit
Copy link
Author

barroit commented Mar 14, 2024

/preview

Copy link

gitgitgadget bot commented Mar 14, 2024

Preview email sent as pull.1693.v3.git.1710443912487.gitgitgadget@gmail.com

Copy link

gitgitgadget bot commented Mar 14, 2024

This patch series was integrated into seen via git@ed37692.

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.

Signed-off-by: Jiamu Sun <barroit@linux.com>
Copy link

gitgitgadget bot commented Mar 14, 2024

This patch series was integrated into seen via git@aa777c3.

Copy link

gitgitgadget bot commented Mar 14, 2024

On the Git mailing list, Jiamu Sun wrote (reply to this):

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.

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

diff --git a/Documentation/git-bugreport.txt b/Documentation/git-bugreport.txt
index ca626f7fc6..112658b3c3 100644
--- a/Documentation/git-bugreport.txt
+++ b/Documentation/git-bugreport.txt
@@ -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>]::
diff --git a/builtin/bugreport.c b/builtin/bugreport.c
index 3106e56a13..25f860a0d9 100644
--- a/builtin/bugreport.c
+++ b/builtin/bugreport.c
@@ -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

Copy link

gitgitgadget bot commented Mar 14, 2024

User Jiamu Sun <baiorettohr@gmail.com> has been added to the cc: list.

Copy link

gitgitgadget bot commented Mar 15, 2024

On the Git mailing list, Jiamu Sun wrote (reply to this):

Jiamu Sun <barroit@linux.com> writes:

> Junio C Hamano <gitster@pobox.com> writes:
>
> I forgot the two I CC'ed the review thread for the previous round to
> ping them, so here it is.
>
> Thanks.

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.

Copy link

gitgitgadget bot commented Mar 15, 2024

This patch series was integrated into seen via git@be18540.

Copy link

gitgitgadget bot commented Mar 16, 2024

There was a status update in the "New Topics" section about the branch js/bugreport-no-suffix-fix on the Git mailing list:

"git bugreport --no-suffix" was not supported and instead
segfaulted, which has been corrected.

Will merge to 'next'?
source: <9c6f3f5203ae26c501a5711e2610573130bfd550.1710388817.git.gitgitgadget@gmail.com>

Copy link

gitgitgadget bot commented Mar 16, 2024

On the Git mailing list, Taylor Blau wrote (reply to this):

On Wed, Mar 13, 2024 at 08:59:52AM -0700, Junio C Hamano wrote:
> "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).

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

Copy link

gitgitgadget bot commented Mar 16, 2024

User Taylor Blau <me@ttaylorr.com> has been added to the cc: list.

Copy link

gitgitgadget bot commented Mar 16, 2024

On the Git mailing list, Taylor Blau wrote (reply to this):

On Fri, Mar 15, 2024 at 07:34:06AM +0900, Jiamu Sun wrote:
> 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.
>
> Signed-off-by: Jiamu Sun <barroit@linux.com>
> ---

    Acked-by: Taylor Blau <me@ttaylorr.com>

Thanks,
Taylor

Copy link

gitgitgadget bot commented Mar 16, 2024

This patch series was integrated into seen via git@28e6dc5.

@barroit barroit closed this Mar 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants