Re: [PATCH] fix segv with corrupt tag object

From: René Scharfe <l.s.r@web.de>
Date: Aug 25, 2019, 07:52 UTC
Message-ID: <bcc29199-a4ac-6bdc-6715-9807737253d8@web.de>
In-Reply-To: <20190824230944.GA14132@jessup.stsp.name>

Am 25.08.19 um 01:09 schrieb Stefan Sperling:

Show 8 quoted lines

> A tag object which lacks newlines won't be parsed correctly.
> Git fails to detect this error and crashes due to a NULL deref:
>
> $ git archive 1.0.0
> Segmentation fault (core dumped)
> $ git checkout 1.0.0
> Segmentation fault (core dumped)
> $

Good find.

Show 11 quoted lines

>
> See the attached tarball for a reproduction repository.
> Also mirrored at https://stsp.name/git-checkout-tag-segv-repo.tgz
>
> With the patch below:
>
> $ git checkout 1.0.0
> fatal: reference is not a tree: 1.0.0
> $ git archive 1.0.0
> fatal: not a tree object: a99665eea5ee50171b5b7249880aa2ae35e35823
> $

Sign-off?

Show 16 quoted lines

>
> diff --git a/tree.c b/tree.c
> index 4720945e6a..92d8bd57a3 100644
> --- a/tree.c
> +++ b/tree.c
> @@ -252,9 +252,11 @@ struct tree *parse_tree_indirect(const struct object_id *oid)
>  			return (struct tree *) obj;
>  		else if (obj->type == OBJ_COMMIT)
>  			obj = &(get_commit_tree(((struct commit *)obj))->object);
> -		else if (obj->type == OBJ_TAG)
> +		else if (obj->type == OBJ_TAG) {
>  			obj = ((struct tag *) obj)->tagged;
> -		else
> +			if (!obj)
> +				return NULL;
> +		} else

OK.

There seem to be some more placed the use ->tagged without checking (found with "git grep -wW tagged"):

  builtin/describe.c::describe_commit()
  builtin/fast-export.c::handle_tag()
  builtin/log.c::cmd_show()
  builtin/replace.c::check_one_mergetag()
  fsck.c::fsck_walk_tag() -- I'm not sure about that one
  log-tree.c::show_one_mergetag()
  packfile.c::add_promisor_object()
  ref-filter.c::populate_value()
  ref-filter.c::match_points_at()
  walker.c::process_tag()

Ugh!  Do you perhaps want to have a go at them as well?

>  			return NULL;
>  		if (!obj->parsed)
>  			parse_object(the_repository, &obj->oid);
>

Hmm, I find it a bit sad that this function is almost a duplicate of sha1-name.c::repo_peel_to_type(), which already checks for ->tagged being NULL.

René