show-branch: fix SEGFAULT on both --current & --reflog

From: Gregory DAVID <[email protected]>

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 1aa68d6735 (show-branch: --current includes the current branch., 2006-01-11)

Thanks-to: Ævar Arnfjörð Bjarmason <[email protected]>
---
show-branch: fix SEGFAULT on --current + --reflog

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 1aa68d6735 (show-branch:
--current includes the current branch., 2006-01-11)

Arnfjörð Bjarmason [email protected]

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1222%2Fp1-gdd%2Ffix%2Fshow-branch-segfault-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1222/p1-gdd/fix/show-branch-segfault-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1222

 builtin/show-branch.c  | 16 ++++++++++++++++
 t/t3202-show-branch.sh | 43 ++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 59 insertions(+)

index e12c5e80e3e..723637079d8 100644
@@ -812,8 +812,24 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)
 		}
 		if (!has_head) {
 			const char *name = head;
+			struct object_id oid;
+			char *ref;
+			unsigned int flags = 0;
+			char *log_msg = NULL;
+			char *end_log_msg;
+			timestamp_t timestamp;
+			int tz;
+
+			if (!dwim_ref(*av, strlen(*av), &oid, &ref, 0))
+				die(_("no such ref %s"), *av);
+			read_ref_at(get_main_ref_store(the_repository), ref, flags, 0, i, &oid, &log_msg, &timestamp, &tz, NULL);
+			end_log_msg = strchr(log_msg, '\n');
+			if (end_log_msg)
+				*end_log_msg = '\0';
 			skip_prefix(name, "refs/heads/", &name);
 			append_one_rev(name);
+			reflog_msg[ref_name_cnt - 1] = xstrfmt("(%s) (current) %s", show_date(timestamp, tz, DATE_MODE(RELATIVE)), (!log_msg || !*log_msg) ? "(none)" : log_msg);
+			free(log_msg);
 		}
 	}
 

base-commit: d516b2db0af2221bd6b13e7347abdcb5830b2829

gitgitgadget

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.

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.

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.

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.

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