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@xxxxxxxxx>
> Thanks-to: Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx>
> ---
> 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);
> }
