ref-filter: fix NULL check for parse object failure

Commit Message

Jeff King April 1, 2021, 8:32 a.m. UTC

On Thu, Apr 01, 2021 at 03:54:56AM -0400, Jeff King wrote:

> On Wed, Mar 31, 2021 at 10:46:22PM +0200, Ævar Arnfjörð Bjarmason wrote:
> 
> > > Neither of those types is the correct one. And the segfault is just a
> > > bonus! :)
> > >
> > > I'd expect similar cases with parsing commit parents and tree pointers.
> > > And probably tree entries whose modes are wrong.
> > 
> > So the segfault happens without my patches,
> 
> Yeah, sorry if that was unclear. It is definitely a pre-existing bug.

Here's a patch to fix it. This is mostly orthogonal to your patch
series. It happens to use a similar recipe to reproduce, but that is not
the only way to do it, and the fix and the test shouldn't conflict
textually or semantically.

-- >8 --
Subject: [PATCH] ref-filter: fix NULL check for parse object failure

After we run parse_object_buffer() to get an object's contents, we try
to check that the return value wasn't NULL. However, since our "struct
object" is a pointer-to-pointer, and we assign like:

  *obj = parse_object_buffer(...);

it's not correct to check:

  if (!obj)

That will always be true, since our double pointer will continue to
point to the single pointer (which is itself NULL). This is a regression
that was introduced by aa46a0da30 (ref-filter: use oid_object_info() to
get object, 2018-07-17); since that commit we'll segfault on a parse
failure, as we try to look at the NULL object pointer.

There are many ways a parse could fail, but most of them are hard to set
up in the tests (it's easy to make a bogus object, but update-ref will
refuse to point to it). The test here uses a tag which points to a wrong
object type. A parse of just the broken tag object will succeed, but
seeing both tag objects in the same process will lead to a parse error
(since we'll see the pointed-to object as both types).

Signed-off-by: Jeff King <[e-mail]>
---
 ref-filter.c            |  2 +-
 t/t6300-for-each-ref.sh | 10 ++++++++++
 2 files changed, 11 insertions(+), 1 deletion(-)

Comments

Junio C Hamano April 1, 2021, 7:52 p.m. UTC | #1

Jeff King <[e-mail]> writes:

> Here's a patch to fix it. This is mostly orthogonal to your patch
> series. It happens to use a similar recipe to reproduce, but that is not
> the only way to do it, and the fix and the test shouldn't conflict
> textually or semantically.
>
> -- >8 --
> Subject: [PATCH] ref-filter: fix NULL check for parse object failure
>
> After we run parse_object_buffer() to get an object's contents, we try
> to check that the return value wasn't NULL. However, since our "struct
> object" is a pointer-to-pointer, and we assign like:
>
>   *obj = parse_object_buffer(...);
>
> it's not correct to check:
>
>   if (!obj)
>
> That will always be true, since our double pointer will continue to
> point to the single pointer (which is itself NULL). This is a regression
> that was introduced by aa46a0da30 (ref-filter: use oid_object_info() to
> get object, 2018-07-17); since that commit we'll segfault on a parse
> failure, as we try to look at the NULL object pointer.
>
> There are many ways a parse could fail, but most of them are hard to set
> up in the tests (it's easy to make a bogus object, but update-ref will
> refuse to point to it). The test here uses a tag which points to a wrong
> object type. A parse of just the broken tag object will succeed, but
> seeing both tag objects in the same process will lead to a parse error
> (since we'll see the pointed-to object as both types).
>
> Signed-off-by: Jeff King <[e-mail]>
> ---
>  ref-filter.c            |  2 +-
>  t/t6300-for-each-ref.sh | 10 ++++++++++
>  2 files changed, 11 insertions(+), 1 deletion(-)

Makes sense.  Will queue.

> diff --git a/ref-filter.c b/ref-filter.c
> index f0bd32f714..a0adb4551d 100644
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -1608,7 +1608,7 @@ static int get_object(struct ref_array_item *ref, int deref, struct object **obj
>  
>  	if (oi->info.contentp) {
>  		*obj = parse_object_buffer(the_repository, &oi->oid, oi->type, oi->size, oi->content, &eaten);
> -		if (!obj) {
> +		if (!*obj) {
>  			if (!eaten)
>  				free(oi->content);
>  			return strbuf_addf_ret(err, -1, _("parse_object_buffer failed on %s for %s"),
> diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
> index cac7f443d0..2e7c32d50c 100755
> --- a/t/t6300-for-each-ref.sh
> +++ b/t/t6300-for-each-ref.sh
> @@ -1134,4 +1134,14 @@ test_expect_success 'for-each-ref --ignore-case works on multiple sort keys' '
>  	test_cmp expect actual
>  '
>  
> +test_expect_success 'for-each-ref reports broken tags' '
> +	git tag -m "good tag" broken-tag-good HEAD &&
> +	git cat-file tag broken-tag-good >good &&
> +	sed s/commit/blob/ <good >bad &&
> +	bad=$(git hash-object -w -t tag bad) &&
> +	git update-ref refs/tags/broken-tag-bad $bad &&
> +	test_must_fail git for-each-ref --format="%(*objectname)" \
> +		refs/tags/broken-tag-*
> +'
> +
>  test_done

Patch

diff --git a/ref-filter.c b/ref-filter.c
index f0bd32f714..a0adb4551d 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1608,7 +1608,7 @@  static int get_object(struct ref_array_item *ref, int deref, struct object **obj
 
 	if (oi->info.contentp) {
 		*obj = parse_object_buffer(the_repository, &oi->oid, oi->type, oi->size, oi->content, &eaten);
-		if (!obj) {
+		if (!*obj) {
 			if (!eaten)
 				free(oi->content);
 			return strbuf_addf_ret(err, -1, _("parse_object_buffer failed on %s for %s"),
diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
index cac7f443d0..2e7c32d50c 100755
--- a/t/t6300-for-each-ref.sh
+++ b/t/t6300-for-each-ref.sh
@@ -1134,4 +1134,14 @@  test_expect_success 'for-each-ref --ignore-case works on multiple sort keys' '
 	test_cmp expect actual
 '
 
+test_expect_success 'for-each-ref reports broken tags' '
+	git tag -m "good tag" broken-tag-good HEAD &&
+	git cat-file tag broken-tag-good >good &&
+	sed s/commit/blob/ <good >bad &&
+	bad=$(git hash-object -w -t tag bad) &&
+	git update-ref refs/tags/broken-tag-bad $bad &&
+	test_must_fail git for-each-ref --format="%(*objectname)" \
+		refs/tags/broken-tag-*
+'
+
 test_done