show-branch: fix SEGFAULT on --current + --reflog

[p1-gdd]

If run show-branch with --current and --reflog simultaneously, a
SEGFAULT appears.

The bug is that we read over the end of the reflog_msg array after
having append_one_rev() for the current branch without supplying a
convenient message to it.

It seems that it has been introduced in:
Commit 1aa68d6 (show-branch: --current includes the current branch., 2006-01-11)

Signed-off-by: Gregory DAVID gregory.david@p1sec.com
Thanks-to: Ævar Arnfjörð Bjarmason avarab@gmail.com
CC: Ævar Arnfjörð Bjarmason avarab@gmail.com, Phillip Wood phillip.wood123@gmail.com
cc: Gregory David gregory.david@p1sec.com

[gitgitgadget]

Welcome to GitGitGadget

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

Please make sure that your Pull Request has a good description, as it will be used as cover letter. 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:

• Search: is:pr is:open "/allow"

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.

[gitgitgadget]

There are issues in commit 4bd3dcb:
show-branch: fix SEGFAULT when --currentand--reflog together
Lines in the body of the commit messages should be wrapped between 60 and 76 characters.

[Ikke]

/allow

[gitgitgadget]

User p1-gdd is now allowed to use GitGitGadget.

WARNING: p1-gdd has no public email address set on GitHub;
GitGitGadget needs an email address to Cc: you on your contribution, so that you receive any feedback on the Git mailing list. Go to https://github.com/settings/profile to make your preferred email public to let GitGitGadget know which email address to use.

[gitgitgadget]

There are issues in commit 9c59ea8:
show-branch: fix SEGFAULT on both --current & --reflog
Lines in the body of the commit messages should be wrapped between 60 and 76 characters.

[p1-gdd]

/submit

[gitgitgadget]

Submitted as pull.1222.git.1650634704191.gitgitgadget@gmail.com

To fetch this version into FETCH_HEAD:

git fetch https://github.com/gitgitgadget/git/ pr-1222/p1-gdd/fix/show-branch-segfault-v1

To fetch this version to local tag pr-1222/p1-gdd/fix/show-branch-segfault-v1:

git fetch --no-tags https://github.com/gitgitgadget/git/ tag pr-1222/p1-gdd/fix/show-branch-segfault-v1

[p1-gdd]

The test fails on Ubuntu latest because the default branch name has changed to main.

[gitgitgadget]

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

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

> From: Gregory DAVID <gregory.david@p1sec.com>
>
> If run `show-branch` with `--current` and `--reflog` simultaneously, a
> SEGFAULT appears.

Thanks for noticing.  As I said elsewhere, "--current" was invented
for the sole purpose of being used together with branches and
individual commits, not in "--reflog" or "--merge-base" modes.

And as I also said elsewhere, I do not think of a good reason why a
user would want to use these two together.

Can you tell us a bit more about what you were trying to achieve
when you used them together?

While waiting for your (and others) valid use cases I probably have
missed (I said "do not think of" above, not "I think there cannot
be"), let's speculate what sensible meaning the combination could
have.

As is clear from an existing error when two branches are given:

    $ git show-branch --reflog master maint
    fatal: --reflog option needs one branch name

the "--reflog" mode is not even prepared to work with more than one
branch.  It is to show reflog entries taken from one branch (it
could be HEAD)'s reflog.

But "--current" is about "Among the branches I listed on the command
line, the current branch may or may not be included. If not, please
pretend as if I had the current branch there, too".

So, if we said

    $ git show-branch --reflog --current maint

while we are on 'master' branch, that is the same as saying

    $ git show-branch --reflog master maint

which should get a usage error, and if we are on 'maint' branch,
then maint is already there, so there is no point in giving
'--current' to begin with.

Because

    $ git show-branch --reflog

defaults to showing the reflog entries from current branch,

    $ git show-branch --reflog --current 

hoping that it would show the reflog entries of the current branch
is indeed a plausible interpretation, but even in this case, it is
not necessary to give "--current".

So, unless there is a reason why it makes sense to enumerate recent
reflog entries from a branch *and* the tip of the current branch at
the same time, I am very much inclined to make it clear that
"--reflog" and "--current" are mutually incompatible by making it an
error to give both.

In addition, we _could_ allow a command line with "--reflog --current"
and nothing else on it, and ignore "--current" only in that case, to
"support" the "plausible interpretation" above, but I do not think
it is worth it.

> It seems that it has been introduced in: Commit 1aa68d6735
> (show-branch: --current includes the current branch., 2006-01-11)

Yes, the commit should have noticed the invalid combination of
options were given and errored out.  Since omission of such a check
lead to a segfaulting bug without producing any useful output, it
is safe to make it an error to give these options at the same time.

Thanks.

[gitgitgadget]

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

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

>> It seems that it has been introduced in: Commit 1aa68d6735
>> (show-branch: --current includes the current branch., 2006-01-11)
>
> Yes, the commit should have noticed the invalid combination of
> options were given and errored out.  Since omission of such a check
> lead to a segfaulting bug without producing any useful output, it
> is safe to make it an error to give these options at the same time.

Actually, no, the commit couldn't have been the culprit.  Back when
1aa58d6735 was written, "--reflog" option did not even exist.

If we want to find a commit to blame, it is 76a44c5c (show-branch
--reflog: show the reflog message at the top., 2007-01-19).  That
commit should have caught invalid combination.  It did attempt to
catch some invalid combinations (like --more in the reflog mode),
but apparently forgot to notice that --current does not make sense
to be used in the new mode it was adding.

Thanks.

[gitgitgadget]

On the Git mailing list, Gregory David wrote (reply to this):

You are right Junio! They are mutually exclusives and your last
suggestion's patch is totally coherent, instead of mine.

My attempt about this combination was driven by a missunderstanding of
the command and the goal to find all ancestors' branches, sorted
reverse-chronologicaly, for a given commit.

I'm happy to have contributed to the git project by discovering the
segfault. Happy, also, that together, we achieve a better fix than I done.

Hope this would help all of us, as it will ensure a more stable use in
our devs.

Sincerely.

On 22/04/2022 17:26, Junio C Hamano wrote:
> "gdd via GitGitGadget" <gitgitgadget@gmail.com> writes:
> 
>> From: Gregory DAVID <gregory.david@p1sec.com>
>>
>> If run `show-branch` with `--current` and `--reflog` simultaneously, a
>> SEGFAULT appears.
> 
> Thanks for noticing.  As I said elsewhere, "--current" was invented
> for the sole purpose of being used together with branches and
> individual commits, not in "--reflog" or "--merge-base" modes.
> 
> And as I also said elsewhere, I do not think of a good reason why a
> user would want to use these two together.
> 
> Can you tell us a bit more about what you were trying to achieve
> when you used them together?
> 
> While waiting for your (and others) valid use cases I probably have
> missed (I said "do not think of" above, not "I think there cannot
> be"), let's speculate what sensible meaning the combination could
> have.
> 
> As is clear from an existing error when two branches are given:
> 
>     $ git show-branch --reflog master maint
>     fatal: --reflog option needs one branch name
> 
> the "--reflog" mode is not even prepared to work with more than one
> branch.  It is to show reflog entries taken from one branch (it
> could be HEAD)'s reflog.
> 
> But "--current" is about "Among the branches I listed on the command
> line, the current branch may or may not be included. If not, please
> pretend as if I had the current branch there, too".
> 
> So, if we said
> 
>     $ git show-branch --reflog --current maint
> 
> while we are on 'master' branch, that is the same as saying
> 
>     $ git show-branch --reflog master maint
> 
> which should get a usage error, and if we are on 'maint' branch,
> then maint is already there, so there is no point in giving
> '--current' to begin with.
> 
> Because
> 
>     $ git show-branch --reflog
> 
> defaults to showing the reflog entries from current branch,
> 
>     $ git show-branch --reflog --current 
> 
> hoping that it would show the reflog entries of the current branch
> is indeed a plausible interpretation, but even in this case, it is
> not necessary to give "--current".
> 
> So, unless there is a reason why it makes sense to enumerate recent
> reflog entries from a branch *and* the tip of the current branch at
> the same time, I am very much inclined to make it clear that
> "--reflog" and "--current" are mutually incompatible by making it an
> error to give both.
> 
> In addition, we _could_ allow a command line with "--reflog --current"
> and nothing else on it, and ignore "--current" only in that case, to
> "support" the "plausible interpretation" above, but I do not think
> it is worth it.
> 
>> It seems that it has been introduced in: Commit 1aa68d6735
>> (show-branch: --current includes the current branch., 2006-01-11)
> 
> Yes, the commit should have noticed the invalid combination of
> options were given and errored out.  Since omission of such a check
> lead to a segfaulting bug without producing any useful output, it
> is safe to make it an error to give these options at the same time.
> 
> Thanks.

-- 
Gregory David
Security Engineer
https://www.p1sec.com

[gitgitgadget]

User Gregory David <gregory.david@p1sec.com> has been added to the cc: list.