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

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 21, 2022, 18:36 UTC
Message-ID: <xmqqczhai8qv.fsf@gitster.g>
In-Reply-To: <patch-v3-2.2-396c3338533-20220421T152704Z-avarab@gmail.com>

Æ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.