threads / patch / 57773

patchshow-branch: fix SEGFAULT when `--current` and `--reflog` together

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

## tl;dr

7 messages between Apr 21, 2022 and Apr 21, 2022. Diffs are folded; open one to read it.

replies: 6people: 3as markdown or json

Gregory David· Apr 21, 2022, 13:34 UTC · lore

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(-)
Show changes to 2 files +62 −2

builtin/show-branch.c, t/t3202-show-branch.sh

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
Ævar Arnfjörð Bjarmason· Apr 21, 2022, 15:33 UTC · re: Gregory David · lore

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

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
Ævar Arnfjörð Bjarmason· Apr 21, 2022, 15:33 UTC · re: Ævar Arnfjörð Bjarmason · lore

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

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(-)
Show changes to builtin/show-branch.c +21 −13
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
Junio C Hamano· Apr 21, 2022, 18:13 UTC · re: Ævar Arnfjörð Bjarmason · lore

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

Æ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.
Show 78 quoted lines
> 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);
Ævar Arnfjörð Bjarmason· Apr 21, 2022, 15:33 UTC · re: Ævar Arnfjörð Bjarmason · lore

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

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(+)
Show changes to 2 files +56 −0

builtin/show-branch.c, t/t3202-show-branch.sh

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
Junio C Hamano· Apr 21, 2022, 18:36 UTC · re: Ævar Arnfjörð Bjarmason · lore

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

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
Show 39 quoted lines
> 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.

Junio C Hamano· Apr 21, 2022, 21:25 UTC · re: Junio C Hamano · lore

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

Junio C Hamano <gitster@pobox.com> writes:
Show 11 quoted lines
> 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(+)
Show changes to diff +18 −0
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

← back to recent threads