# [PATCH] show-branch: fix SEGFAULT when `--current` and `--reflog` together 3 messages from 2022-04-19 to 2022-04-20. Participants: Gregory David, Ævar Arnfjörð Bjarmason, Phillip Wood. Thread: https://gitlist.dev/t/57763 ## Gregory David, 2022-04-19 16:32 Subject: [PATCH] show-branch: fix SEGFAULT when `--current` and `--reflog` together Message-ID: URL: https://gitlist.dev/e/a36fd2b0-0573-b93e-a765-ce57a651934e%40p1sec.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 Thanks-to: Ævar Arnfjörð Bjarmason --- 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, + ×tamp, &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 ``` ## Ævar Arnfjörð Bjarmason, 2022-04-19 20:49 Subject: Re: [PATCH] show-branch: fix SEGFAULT when `--current` and `--reflog` together Message-ID: <220419.86ee1s94mp.gmgdl@evledraar.gmail.com> URL: https://gitlist.dev/e/220419.86ee1s94mp.gmgdl%40evledraar.gmail.com In-Reply-To: ``` 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 > Thanks-to: Ævar Arnfjörð Bjarmason > --- > 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, > + ×tamp, &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); > } ``` ## Phillip Wood, 2022-04-20 10:04 Subject: Re: [PATCH] show-branch: fix SEGFAULT when `--current` and `--reflog` together Message-ID: <0825f153-1d03-feb4-1e34-4aaed766deb0@gmail.com> URL: https://gitlist.dev/e/0825f153-1d03-feb4-1e34-4aaed766deb0%40gmail.com In-Reply-To: ``` 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 > Thanks-to: Ævar Arnfjörð Bjarmason > --- > 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, > + ×tamp, &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); > } ```