On Tue, Nov 17, 2015 at 6:22 PM, Jeff King <peff@xxxxxxxx> wrote:
> On Tue, Nov 17, 2015 at 06:01:25PM -0500, Eric Sunshine wrote:
>> On Tue, Nov 17, 2015 at 5:48 PM, Jeff King <peff@xxxxxxxx> wrote:
>> > Hmm. Out of curiosity I tried:
>> >
>> > git blame v2.4.0 -- t/t6031-merge-recursive.sh
>> >
>> > and it segfaults. This bisects to Max's recent 1b0d400 (blame: extract
>> > find_single_final, 2015-10-30), but I do not see anything obviously
>> > wrong with it from a quick glance.
>>
>> In the original code, sb->final received was assigned value of obj,
>> which may have gone through deref_tag(), however, after 1b0d400,
>> sb->final is unconditionally assigned the original value of obj, not
>> the (potentially) deref'd value.
>
> Good eye. I fed it a tag, find_single_final knows that points to a
> commit, then prepare_final casts the tag object to a commit. Whoops.
>
> The patch below fixes it for me. It probably needs a test, but I have to
> run for the moment.
Sorry for the late response. This patch mirrors my thoughts on fixing
the bug, and appears correct. For what it's worth:
Reviewed-by: Eric Sunshine <sunshine@xxxxxxxxxxxxxx>
> -- >8 --
> Subject: [PATCH] blame: fix object casting regression
>
> Commit 1b0d400 refactored the prepare_final() function so
> that it could be reused in multiple places. Originally, the
> loop had two outputs: a commit to stuff into sb->final, and
> the name of the commit from the rev->pending array.
>
> After the refactor, that loop is put in its own function
> with a single return value: the object_array_entry from the
> rev->pending array. This contains both the name and the object,
> but with one important difference: the object is the
> _original_ object found by the revision parser, not the
> dereferenced commit. If one feeds a tag to "git blame", we
> end up casting the tag object to a "struct commit", which
> causes a segfault.
>
> Instead, let's return the commit (properly casted) directly
> from the function, and take the "name" as an optional
> out-parameter. This does the right thing, and actually
> simplifies the callers, who no longer need to cast or
> dereference the object_array_entry themselves.
>
> Signed-off-by: Jeff King <peff@xxxxxxxx>
> ---
> builtin/blame.c | 30 ++++++++++++++----------------
> 1 file changed, 14 insertions(+), 16 deletions(-)
>
> diff --git a/builtin/blame.c b/builtin/blame.c
> index ac36738..2184e39 100644
> --- a/builtin/blame.c
> +++ b/builtin/blame.c
> @@ -2403,10 +2403,12 @@ static struct commit *fake_working_tree_commit(struct diff_options *opt,
> return commit;
> }
>
> -static struct object_array_entry *find_single_final(struct rev_info *revs)
> +static struct commit *find_single_final(struct rev_info *revs,
> + const char **name_p)
> {
> int i;
> - struct object_array_entry *found = NULL;
> + struct commit *found = NULL;
> + const char *name = NULL;
>
> for (i = 0; i < revs->pending.nr; i++) {
> struct object *obj = revs->pending.objects[i].item;
> @@ -2418,22 +2420,20 @@ static struct object_array_entry *find_single_final(struct rev_info *revs)
> die("Non commit %s?", revs->pending.objects[i].name);
> if (found)
> die("More than one commit to dig from %s and %s?",
> - revs->pending.objects[i].name,
> - found->name);
> - found = &(revs->pending.objects[i]);
> + revs->pending.objects[i].name, name);
> + found = (struct commit *)obj;
> + name = revs->pending.objects[i].name;
> }
> + if (name_p)
> + *name_p = name;
> return found;
> }
>
> static char *prepare_final(struct scoreboard *sb)
> {
> - struct object_array_entry *found = find_single_final(sb->revs);
> - if (found) {
> - sb->final = (struct commit *) found->item;
> - return xstrdup(found->name);
> - } else {
> - return NULL;
> - }
> + const char *name;
> + sb->final = find_single_final(sb->revs, &name);
> + return xstrdup_or_null(name);
> }
>
> static char *prepare_initial(struct scoreboard *sb)
> @@ -2721,11 +2721,9 @@ parse_done:
> die("Cannot use --contents with final commit object name");
>
> if (reverse && revs.first_parent_only) {
> - struct object_array_entry *entry = find_single_final(sb.revs);
> - if (!entry)
> + final_commit = find_single_final(sb.revs, NULL);
> + if (!final_commit)
> die("--reverse and --first-parent together require specified latest commit");
> - else
> - final_commit = (struct commit*) entry->item;
> }
>
> /*
> --
> 2.6.3.636.g1460207
>
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html