[PATCH] show-branch: fix SEGFAULT when `--current` and `--reflog` together

[PATCH] show-branch: fix SEGFAULT when `--current` and `--reflog` together

[Gregory David]

[-- Attachment #1.1.1: Type: text/plain, Size: 3552 bytes --]

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)

Signed-off-by: Gregory DAVID <gregory.david@p1sec.com>
Thanks-to: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/show-branch.c  | 22 +++++++++++++++++++--
 t/t3202-show-branch.sh | 43 ++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 63 insertions(+), 2 deletions(-)

diff --git a/builtin/show-branch.c b/builtin/show-branch.c
index e12c5e80e3..c8d830b7c6 100644
--- a/builtin/show-branch.c
+++ b/builtin/show-branch.c
@@ -812,8 +812,26 @@ int cmd_show_branch(int ac, const char **av, const
char *prefix)
 		}
 		if (!has_head) {
 			const char *name = head;
-			skip_prefix(name, "refs/heads/", &name);
-			append_one_rev(name);
+			struct object_id oid;
+			char *ref;
+			unsigned int flags = 0;
+			char *log_msg = 0;
+			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);
 		}
 	}
 diff --git a/t/t3202-show-branch.sh b/t/t3202-show-branch.sh
index 7a1be73ce8..7f6ffcf8a5 100755
--- a/t/t3202-show-branch.sh
+++ b/t/t3202-show-branch.sh
@@ -161,4 +161,47 @@ test_expect_success 'show branch --reflog=2' '
 	test_cmp actual expect
 '
 +test_expect_success 'show branch --reflog=2 --current' '
+	sed "s/^>	//" >expect <<-\EOF &&
+	>	! [refs/heads/branch10@{0}] (4 years, 5 months ago) commit: branch10
+	>	 ! [refs/heads/branch10@{1}] (4 years, 5 months ago) commit: branch10
+	>	  * [branch10] (4 years, 5 months ago) (current) branch: Created
from initial
+	>	---
+	>	+ * [refs/heads/branch10@{0}] branch10
+	>	++* [refs/heads/branch10@{1}] initial
+	EOF
+	git show-branch --reflog=2 --current >actual &&
+	test_cmp actual expect
+'
+
+test_expect_success 'show branch --current' '
+	sed "s/^>	//" >expect <<-\EOF &&
+	>	! [branch1] branch1
+	>	 ! [branch2] branch2
+	>	  ! [branch3] branch3
+	>	   ! [branch4] branch4
+	>	    ! [branch5] branch5
+	>	     ! [branch6] branch6
+	>	      ! [branch7] branch7
+	>	       ! [branch8] branch8
+	>	        ! [branch9] branch9
+	>	         * [branch10] branch10
+	>	          ! [master] initial
+	>	-----------
+	>	         *  [branch10] branch10
+	>	        +   [branch9] branch9
+	>	       +    [branch8] branch8
+	>	      +     [branch7] branch7
+	>	     +      [branch6] branch6
+	>	    +       [branch5] branch5
+	>	   +        [branch4] branch4
+	>	  +         [branch3] branch3
+	>	 +          [branch2] branch2
+	>	+           [branch1] branch1
+	>	+++++++++*+ [master] initial
+	EOF
+	git show-branch --current >actual &&
+	test_cmp actual expect
+'
+
 test_done
-- 
2.35.1

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 2501 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 665 bytes --]

[PATCH v3 0/2] show-brach: segfault fix from Gregory David

[Ævar Arnfjörð Bjarmason]

This is a version of
https://lore.kernel.org/git/225b410d-2d98-8c0b-c289-22f753c175d4@p1sec.com/
that applies with "git am", the patch got munged in transit (from the
looks of it from Thunderbird's wrapping).

I added a 1/2 to factor out some shared code used by the segfault fix,
and made the now-smaller 2/2 use that function.

I also renamed some varibles to reduce wrapping, and made the
append_one_rev() come after the ref formatting. Other similar code
above does that, it looks to me like they're equivalent
(append_one_rev increments ref_name_cnt).

Gregory David (1):
  show-branch: fix SEGFAULT when `--current` and `--reflog` together

Ævar Arnfjörð Bjarmason (1):
  show-branch: refactor in preparation for next commit

 builtin/show-branch.c  | 47 ++++++++++++++++++++++++++++++------------
 t/t3202-show-branch.sh | 43 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 77 insertions(+), 13 deletions(-)

-- 
2.36.0.876.g7efc8a7728c

[PATCH v3 1/2] show-branch: refactor in preparation for next commit

[Ævar Arnfjörð Bjarmason]

Move the code in cmd_show_branch() that formats a reflog message for
us into a function, and change the "flags" variable that we never
change into a "const", in addition to moving it up a scope in
preparation for the subsequent commit.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/show-branch.c | 34 +++++++++++++++++++++-------------
 1 file changed, 21 insertions(+), 13 deletions(-)

diff --git a/builtin/show-branch.c b/builtin/show-branch.c
index 330b0553b9d..499ef76a508 100644
--- a/builtin/show-branch.c
+++ b/builtin/show-branch.c
@@ -618,6 +618,24 @@ static int parse_reflog_param(const struct option *opt, const char *arg,
 	return 0;
 }
 
+static char *fmt_reflog(char *const logmsg, const timestamp_t ts, const int tz,
+			const char *fmt)
+{
+	char *const end = strchr(logmsg, '\n');
+	const char *msg;
+	char *ret;
+
+	if (end)
+		*end = '\0';
+
+	msg = *logmsg ? logmsg : "(none)";
+
+	ret = xstrfmt(fmt, show_date(ts, tz, DATE_MODE(RELATIVE)), msg);
+	free(logmsg);
+
+	return ret;
+}
+
 int cmd_show_branch(int ac, const char **av, const char *prefix)
 {
 	struct commit *rev[MAX_REVS], *commit;
@@ -640,6 +658,7 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)
 	int topics = 0;
 	int dense = 1;
 	const char *reflog_base = NULL;
+	const unsigned int flags = 0;
 	struct option builtin_show_branch_options[] = {
 		OPT_BOOL('a', "all", &all_heads,
 			 N_("show remote-tracking and local branches")),
@@ -720,7 +739,6 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)
 		struct object_id oid;
 		char *ref;
 		int base = 0;
-		unsigned int flags = 0;
 
 		if (ac == 0) {
 			static const char *fake_av[2];
@@ -761,8 +779,6 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)
 		for (i = 0; i < reflog; i++) {
 			char *logmsg;
 			char *nth_desc;
-			const char *msg;
-			char *end;
 			timestamp_t timestamp;
 			int tz;
 
@@ -773,16 +789,8 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)
 				break;
 			}
 
-			end = strchr(logmsg, '\n');
-			if (end)
-				*end = '\0';
-
-			msg = (*logmsg == '\0') ? "(none)" : logmsg;
-			reflog_msg[i] = xstrfmt("(%s) %s",
-						show_date(timestamp, tz,
-							  DATE_MODE(RELATIVE)),
-						msg);
-			free(logmsg);
+			reflog_msg[i] = fmt_reflog(logmsg, timestamp, tz,
+						   "(%s) %s");
 
 			nth_desc = xstrfmt("%s@{%d}", *av, base+i);
 			append_ref(nth_desc, &oid, 1);
-- 
2.36.0.876.g7efc8a7728c

[PATCH v3 2/2] show-branch: fix SEGFAULT when `--current` and `--reflog` together

[Ævar Arnfjörð Bjarmason]

From: Gregory David <gregory.david@p1sec.com>

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)

Signed-off-by: Gregory DAVID <gregory.david@p1sec.com>
Thanks-to: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/show-branch.c  | 13 +++++++++++++
 t/t3202-show-branch.sh | 43 ++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 56 insertions(+)

diff --git a/builtin/show-branch.c b/builtin/show-branch.c
index 499ef76a508..50c17fb5c31 100644
--- a/builtin/show-branch.c
+++ b/builtin/show-branch.c
@@ -821,6 +821,19 @@ 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;
+			char *msg;
+			timestamp_t ts;
+			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, &msg, &ts, &tz, NULL);
+
+			reflog_msg[ref_name_cnt] = fmt_reflog(msg, ts, tz,
+							      "(%s) (current) %s");
 			skip_prefix(name, "refs/heads/", &name);
 			append_one_rev(name);
 		}
diff --git a/t/t3202-show-branch.sh b/t/t3202-show-branch.sh
index 7a1be73ce87..7f6ffcf8a57 100755
--- a/t/t3202-show-branch.sh
+++ b/t/t3202-show-branch.sh
@@ -161,4 +161,47 @@ test_expect_success 'show branch --reflog=2' '
 	test_cmp actual expect
 '
 
+test_expect_success 'show branch --reflog=2 --current' '
+	sed "s/^>	//" >expect <<-\EOF &&
+	>	! [refs/heads/branch10@{0}] (4 years, 5 months ago) commit: branch10
+	>	 ! [refs/heads/branch10@{1}] (4 years, 5 months ago) commit: branch10
+	>	  * [branch10] (4 years, 5 months ago) (current) branch: Created from initial
+	>	---
+	>	+ * [refs/heads/branch10@{0}] branch10
+	>	++* [refs/heads/branch10@{1}] initial
+	EOF
+	git show-branch --reflog=2 --current >actual &&
+	test_cmp actual expect
+'
+
+test_expect_success 'show branch --current' '
+	sed "s/^>	//" >expect <<-\EOF &&
+	>	! [branch1] branch1
+	>	 ! [branch2] branch2
+	>	  ! [branch3] branch3
+	>	   ! [branch4] branch4
+	>	    ! [branch5] branch5
+	>	     ! [branch6] branch6
+	>	      ! [branch7] branch7
+	>	       ! [branch8] branch8
+	>	        ! [branch9] branch9
+	>	         * [branch10] branch10
+	>	          ! [master] initial
+	>	-----------
+	>	         *  [branch10] branch10
+	>	        +   [branch9] branch9
+	>	       +    [branch8] branch8
+	>	      +     [branch7] branch7
+	>	     +      [branch6] branch6
+	>	    +       [branch5] branch5
+	>	   +        [branch4] branch4
+	>	  +         [branch3] branch3
+	>	 +          [branch2] branch2
+	>	+           [branch1] branch1
+	>	+++++++++*+ [master] initial
+	EOF
+	git show-branch --current >actual &&
+	test_cmp actual expect
+'
+
 test_done
-- 
2.36.0.876.g7efc8a7728c

Re: [PATCH v3 1/2] show-branch: refactor in preparation for next commit

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> Move the code in cmd_show_branch() that formats a reflog message for
> us into a function,

Makes quite a lot of sense.

"factoring out the formatting of reflog message, given a single
reflog entry", is a very good unit of work that is independently
good and can be evaluated as such.

> and change the "flags" variable that we never
> change into a "const", in addition to moving it up a scope in
> preparation for the subsequent commit.

Perhaps doing that as part of the next step is better (or not doing
it if it can be helped)?  Mixing it in contaminates a good isolation
done with an unrelated change.  We somehow ended up seeing too many
"while at it" in the recent patches, and I think that needs to stop.
We should go back to better patch hygiene.

Other than that, looks sensible.  Let's see what 2/2 does.

Thanks.


> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>  builtin/show-branch.c | 34 +++++++++++++++++++++-------------
>  1 file changed, 21 insertions(+), 13 deletions(-)
>
> diff --git a/builtin/show-branch.c b/builtin/show-branch.c
> index 330b0553b9d..499ef76a508 100644
> --- a/builtin/show-branch.c
> +++ b/builtin/show-branch.c
> @@ -618,6 +618,24 @@ static int parse_reflog_param(const struct option *opt, const char *arg,
>  	return 0;
>  }
>  
> +static char *fmt_reflog(char *const logmsg, const timestamp_t ts, const int tz,
> +			const char *fmt)
> +{
> +	char *const end = strchr(logmsg, '\n');
> +	const char *msg;
> +	char *ret;
> +
> +	if (end)
> +		*end = '\0';
> +
> +	msg = *logmsg ? logmsg : "(none)";
> +
> +	ret = xstrfmt(fmt, show_date(ts, tz, DATE_MODE(RELATIVE)), msg);
> +	free(logmsg);
> +
> +	return ret;
> +}
> +
>  int cmd_show_branch(int ac, const char **av, const char *prefix)
>  {
>  	struct commit *rev[MAX_REVS], *commit;
> @@ -640,6 +658,7 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)
>  	int topics = 0;
>  	int dense = 1;
>  	const char *reflog_base = NULL;
> +	const unsigned int flags = 0;
>  	struct option builtin_show_branch_options[] = {
>  		OPT_BOOL('a', "all", &all_heads,
>  			 N_("show remote-tracking and local branches")),
> @@ -720,7 +739,6 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)
>  		struct object_id oid;
>  		char *ref;
>  		int base = 0;
> -		unsigned int flags = 0;
>  
>  		if (ac == 0) {
>  			static const char *fake_av[2];
> @@ -761,8 +779,6 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)
>  		for (i = 0; i < reflog; i++) {
>  			char *logmsg;
>  			char *nth_desc;
> -			const char *msg;
> -			char *end;
>  			timestamp_t timestamp;
>  			int tz;
>  
> @@ -773,16 +789,8 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)
>  				break;
>  			}
>  
> -			end = strchr(logmsg, '\n');
> -			if (end)
> -				*end = '\0';
> -
> -			msg = (*logmsg == '\0') ? "(none)" : logmsg;
> -			reflog_msg[i] = xstrfmt("(%s) %s",
> -						show_date(timestamp, tz,
> -							  DATE_MODE(RELATIVE)),
> -						msg);
> -			free(logmsg);
> +			reflog_msg[i] = fmt_reflog(logmsg, timestamp, tz,
> +						   "(%s) %s");
>  
>  			nth_desc = xstrfmt("%s@{%d}", *av, base+i);
>  			append_ref(nth_desc, &oid, 1);

Re: [PATCH v3 2/2] show-branch: fix SEGFAULT when `--current` and `--reflog` together

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> From: Gregory David <gregory.david@p1sec.com>
>
> 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)
>
> Signed-off-by: Gregory DAVID <gregory.david@p1sec.com>
> Thanks-to: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>  builtin/show-branch.c  | 13 +++++++++++++
>  t/t3202-show-branch.sh | 43 ++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 56 insertions(+)
>
> diff --git a/builtin/show-branch.c b/builtin/show-branch.c
> index 499ef76a508..50c17fb5c31 100644
> --- a/builtin/show-branch.c
> +++ b/builtin/show-branch.c
> @@ -821,6 +821,19 @@ 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;
> +			char *msg;
> +			timestamp_t ts;
> +			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, &msg, &ts, &tz, NULL);

Do we really mean to pass 'i', which is the number of other things we have added,
to read_ref_at()?  Does that mean

    git show-branch -g4 --current

shows the 4th entry from the current branch while

    git show-branch -g2 --current

shows the second?

> +			reflog_msg[ref_name_cnt] = fmt_reflog(msg, ts, tz,
> +							      "(%s) (current) %s");

This unconditionally reads reflog and adds to reflog_msg[], without
even seeing if we are actually showing reflog entries.  Is that
sensible?

I am wondering if we should have factored out a bit more in the
previous step.  Instead of "here is what we read from read_ref_at(),
format it", I wonder if the interface should be "here is a ref and
the offset N; format the Nth entry".  And then this part can become
something like (I do not know about 'i', but that is meant to be the
reflog offset, i.e. "HEAD@{i}").

	if (!has_head) {
		if (reflog) {
			dwim_ref to learn ref;
			reflog_msg[ref_name_cnt] = new_helper(ref, i);
		} else {
			skip_prefix(name, "refs/heads/", head);
			append_one_rev(name);
		}
	}

In any case, I am not sure if it even makes sense to allow the
reflog listing mode with "current" in the first place, and a simpler
option may be to just forbid the combination.  After all, when I
adeed "--current" to "git show-branch" in 1aa68d67 (show-branch:
--current includes the current branch., 2006-01-11), it was clearly
meant to be used with "other branches"---"I would list branches I
care about and I use as anchoring points on the command line, and I
may or may not be on one of these main branches. Please make sure I
can view the current one with respect to these other branches" is
what "--current" is about, and mixing it with "how do recent reflog
entries relate to each other" does not make much sense.

[PATCH] show-branch: -g and --current are incompatible

[Junio C Hamano]

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

> In any case, I am not sure if it even makes sense to allow the
> reflog listing mode with "current" in the first place, and a simpler
> option may be to just forbid the combination.  After all, when I
> adeed "--current" to "git show-branch" in 1aa68d67 (show-branch:
> --current includes the current branch., 2006-01-11), it was clearly
> meant to be used with "other branches"---"I would list branches I
> care about and I use as anchoring points on the command line, and I
> may or may not be on one of these main branches. Please make sure I
> can view the current one with respect to these other branches" is
> what "--current" is about, and mixing it with "how do recent reflog
> entries relate to each other" does not make much sense.

So, here is an alternative "fix" along that line.

----- >8 --------- >8 --------- >8 --------- >8 --------- >8 -----
Subject: [PATCH] show-branch: -g and --current are incompatible

When "--current" is given to "git show-branch" running in the
"--reflog" mode, the code tries to reference a "reflog" message
that does not even exist.  This is because the --current is not
prepared to work in that mode.

The reason "--current" exists is to support this request:

    I list branches on the command line.  These are the branchesI
    care about and I use as anchoring points. I may or may not be on
    one of these main branches.  Please make sure I can view the
    commits on the current branch with respect to what is in these
    other branches.

And to serve that request, the code checks if the current branch is
among the ones listed on the command line, and adds it only if it is
not to the end of one array, which essentially lists the objects.
The reflog mode additionally uses another array to list reflog
messages, which the "--current" code does not add to.  This leaves
one uninitialized slot at the end of the array of reflog messages,
and causes the program to show garbage or segfault.

Catch the unsupported (and meaningless) combination and exit with a
usage error.

There are other combinations of options that are incompatible but
have not been tested.  Add test to cover them while adding coverage
for this new combination.

Reported-by: Gregory David <gregory.david@p1sec.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/show-branch.c  |  4 ++++
 t/t3202-show-branch.sh | 14 ++++++++++++++
 2 files changed, 18 insertions(+)

diff --git c/builtin/show-branch.c w/builtin/show-branch.c
index e12c5e80e3..73e8edbf02 100644
--- c/builtin/show-branch.c
+++ w/builtin/show-branch.c
@@ -711,6 +711,10 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)
 				"--all/--remotes/--independent/--merge-base");
 	}
 
+	if (with_current_branch && reflog)
+		die(_("options '%s' and '%s' cannot be used together"),
+		    "--reflog", "--current");
+
 	/* If nothing is specified, show all branches by default */
 	if (ac <= topics && all_heads + all_remotes == 0)
 		all_heads = 1;
diff --git c/t/t3202-show-branch.sh w/t/t3202-show-branch.sh
index 7a1be73ce8..f2b9199007 100755
--- c/t/t3202-show-branch.sh
+++ w/t/t3202-show-branch.sh
@@ -161,4 +161,18 @@ test_expect_success 'show branch --reflog=2' '
 	test_cmp actual expect
 '
 
+# incompatible options
+while read combo
+do
+	test_expect_success "show-branch $combo (should fail)" '
+		test_must_fail git show-branch $combo 2>error &&
+		grep -e "cannot be used together" -e "usage:" error
+	'
+done <<\EOF
+--all --reflog
+--merge-base --reflog
+--list --merge-base
+--reflog --current
+EOF
+
 test_done