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

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

[Gregory David]

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 | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/builtin/show-branch.c b/builtin/show-branch.c
index e12c5e80e3..892241ce0d 100644
--- a/builtin/show-branch.c
+++ b/builtin/show-branch.c
@@ -812,6 +812,26 @@ 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;
+			char *end_log_msg;
+			timestamp_t timestamp;
+			int tz;
+
+			if (!dwim_ref(*av, strlen(*av), &oid, &ref, 0))
+				die(_("no such ref %s"), *av);
+			if(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';
+			}
+			if(log_msg == 0 || *log_msg == '\0')
+				log_msg = xstrfmt("(none)");
+			reflog_msg[ref_name_cnt] = xstrfmt("(%s) (current) %s",
show_date(timestamp, tz, DATE_MODE(RELATIVE)), log_msg);
 			skip_prefix(name, "refs/heads/", &name);
 			append_one_rev(name);
 		}
-- 
2.35.1

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

[Ævar Arnfjörð Bjarmason]

On Tue, Apr 19 2022, Gregory David wrote:

> 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 | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
>
> diff --git a/builtin/show-branch.c b/builtin/show-branch.c
> index e12c5e80e3..892241ce0d 100644
> --- a/builtin/show-branch.c
> +++ b/builtin/show-branch.c
> @@ -812,6 +812,26 @@ int cmd_show_branch(int ac, const char **av, const
> char *prefix)
>  		}

Hi. Thanks for sticking with this.

First, your patch is corrupt, I think because your mailer is wrapping it
for you. The Documentation/SubmittingPatches has some advice on how to
use "git format-patch/send-email" directly.

It would be great to have a test case for the existing segfault, is it
sometihng you think you could add to t/t3202-show-branch.sh?

>  		if (!has_head) {
>  			const char *name = head;
> +			struct object_id oid;
> +			char *ref;
> +			unsigned int flags = 0;
> +			char *log_msg;
> +			char *end_log_msg;
> +			timestamp_t timestamp;
> +			int tz;
> +
> +			if (!dwim_ref(*av, strlen(*av), &oid, &ref, 0))
> +				die(_("no such ref %s"), *av);
> +			if(read_ref_at(get_main_ref_store(the_repository),

Please use the usual coding style & one in that file, e.g. "if (" not
"if(". 

> +					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';
> +			}

Most of this code is copied from a few lines above where we do much the
same for another case, I wonder if it's worth it to combine the two into
a helper.

> +			if(log_msg == 0 || *log_msg == '\0')

if (!log_msg || !*log_msg) instead, but I see some of this is using a
pattern copied from above...

> +				log_msg = xstrfmt("(none)");
> +			reflog_msg[ref_name_cnt] = xstrfmt("(%s) (current) %s",
> show_date(timestamp, tz, DATE_MODE(RELATIVE)), log_msg);


This code already leaks memory, but we can avoid this case easily by
making that "(none)" part of the second xstrfmt()'s argument list
(i.e. as a ternary).

>  			skip_prefix(name, "refs/heads/", &name);
>  			append_one_rev(name);
>  		}

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

[Phillip Wood]

Hi Gregory

Thanks for working on this

On 19/04/2022 17:32, Gregory David wrote:
> 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 | 20 ++++++++++++++++++++
>   1 file changed, 20 insertions(+)
> 
> diff --git a/builtin/show-branch.c b/builtin/show-branch.c
> index e12c5e80e3..892241ce0d 100644
> --- a/builtin/show-branch.c
> +++ b/builtin/show-branch.c
> @@ -812,6 +812,26 @@ 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;
> +			char *end_log_msg;
> +			timestamp_t timestamp;
> +			int tz;
> +
> +			if (!dwim_ref(*av, strlen(*av), &oid, &ref, 0))
> +				die(_("no such ref %s"), *av);
> +			if(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';
> +			}
> +			if(log_msg == 0 || *log_msg == '\0')
> +				log_msg = xstrfmt("(none)");
> +			reflog_msg[ref_name_cnt] = xstrfmt("(%s) (current) %s",

Do we need a bounds check here? I think we're only guaranteed that 
ref_name_cnt - 1 < MAX_REVS though I found the existing code a bit hard 
to follow

Best Wishes

Phillip

> show_date(timestamp, tz, DATE_MODE(RELATIVE)), log_msg);
>   			skip_prefix(name, "refs/heads/", &name);
>   			append_one_rev(name);
>   		}

[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 --]