ref-filter: initialize empty name or email fields
diff mbox series

Message ID 20190817215107.13733-1-git@shiar.nl
State New
Headers show
Series
  • ref-filter: initialize empty name or email fields
Related show

Commit Message

Mischa POSLAWSKY Aug. 17, 2019, 9:51 p.m. UTC
Formatting $(taggername) on headerless tags such as v0.99 in Git
causes a SIGABRT with error "munmap_chunk(): invalid pointer",
because of an oversight in commit f0062d3b74 (ref-filter: free
item->value and item->value->s, 2018-10-19).

Signed-off-by: Mischa POSLAWSKY <git@shiar.nl>
---
If I understand correctly, such tags cannot be produced normally anymore.
Therefore I'm unsure how to make tests, and if that is even warranted.

 ref-filter.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

Comments

Junio C Hamano Aug. 19, 2019, 5:55 p.m. UTC | #1
Mischa POSLAWSKY <git@shiar.nl> writes:

> Formatting $(taggername) on headerless tags such as v0.99 in Git
> causes a SIGABRT with error "munmap_chunk(): invalid pointer",
> because of an oversight in commit f0062d3b74 (ref-filter: free
> item->value and item->value->s, 2018-10-19).
>
> Signed-off-by: Mischa POSLAWSKY <git@shiar.nl>
> ---
> If I understand correctly, such tags cannot be produced normally anymore.
> Therefore I'm unsure how to make tests, and if that is even warranted.

Thanks for spotting.

I am not sure if the approach taken by this patch is the right one,
though.  I didn't follow the call/dataflow thoroughly, but if we
replace unfree-able "" with NULL in these places, wouldn't
fill_missing_values() take care of them?

>  ref-filter.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/ref-filter.c b/ref-filter.c
> index f27cfc8c3e..7338cfc671 100644
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -1028,7 +1028,7 @@ static const char *copy_name(const char *buf)
>  		if (!strncmp(cp, " <", 2))
>  			return xmemdupz(buf, cp - buf);
>  	}
> -	return "";
> +	return xstrdup("");
>  }
>  
>  static const char *copy_email(const char *buf)
> @@ -1036,10 +1036,10 @@ static const char *copy_email(const char *buf)
>  	const char *email = strchr(buf, '<');
>  	const char *eoemail;
>  	if (!email)
> -		return "";
> +		return xstrdup("");
>  	eoemail = strchr(email, '>');
>  	if (!eoemail)
> -		return "";
> +		return xstrdup("");
>  	return xmemdupz(email, eoemail + 1 - email);
>  }
Junio C Hamano Aug. 20, 2019, 4:37 p.m. UTC | #2
Junio C Hamano <gitster@pobox.com> writes:

> Mischa POSLAWSKY <git@shiar.nl> writes:
>
>> Formatting $(taggername) on headerless tags such as v0.99 in Git
>> causes a SIGABRT with error "munmap_chunk(): invalid pointer",
>> because of an oversight in commit f0062d3b74 (ref-filter: free
>> item->value and item->value->s, 2018-10-19).
>>
>> Signed-off-by: Mischa POSLAWSKY <git@shiar.nl>
>> ---
>> If I understand correctly, such tags cannot be produced normally anymore.
>> Therefore I'm unsure how to make tests, and if that is even warranted.
>
> Thanks for spotting.
>
> I am not sure if the approach taken by this patch is the right one,
> though.  I didn't follow the call/dataflow thoroughly, but if we
> replace unfree-able "" with NULL in these places, wouldn't
> fill_missing_values() take care of them?

I think replacing these "" with NULL would be safe, but there are
many places that return xstrdup("") from inside the callees of
populate_value(), so the patch presented here would be more
consistent with the current practice, I think.

So let's take the patch as is, at least for now.  Thanks.

>>  ref-filter.c | 6 +++---
>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/ref-filter.c b/ref-filter.c
>> index f27cfc8c3e..7338cfc671 100644
>> --- a/ref-filter.c
>> +++ b/ref-filter.c
>> @@ -1028,7 +1028,7 @@ static const char *copy_name(const char *buf)
>>  		if (!strncmp(cp, " <", 2))
>>  			return xmemdupz(buf, cp - buf);
>>  	}
>> -	return "";
>> +	return xstrdup("");
>>  }
>>  
>>  static const char *copy_email(const char *buf)
>> @@ -1036,10 +1036,10 @@ static const char *copy_email(const char *buf)
>>  	const char *email = strchr(buf, '<');
>>  	const char *eoemail;
>>  	if (!email)
>> -		return "";
>> +		return xstrdup("");
>>  	eoemail = strchr(email, '>');
>>  	if (!eoemail)
>> -		return "";
>> +		return xstrdup("");
>>  	return xmemdupz(email, eoemail + 1 - email);
>>  }
Junio C Hamano Aug. 21, 2019, 9:57 p.m. UTC | #3
Junio C Hamano <gitster@pobox.com> writes:

> Mischa POSLAWSKY <git@shiar.nl> writes:
>
>> If I understand correctly, such tags cannot be produced normally anymore.
>> Therefore I'm unsure how to make tests, and if that is even warranted.
>
> Thanks for spotting.

A quick trial to recreate a tag object seems to succeed:

    $ git cat-file tag v0.99 |
    > sed -e '/-----BEGIN/,$d' |
    > git hash-object --stdin -w -t tag
    667d141b478eee5e53d2ee05acd61bb1f640249a
    $ git cat-file tag 667d141b47
    object a3eb250f996bf5e12376ec88622c4ccaabf20ea8
    type commit
    tag v0.99

    Test-release for wider distribution.

    I'll make the first public RPM's etc, thus the tag.

So we should be able to do something along the above line.  Here is
my quick-n-dirty one.

 t/t6300-for-each-ref.sh | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
index ab69aa176d..b3a6b336fa 100755
--- a/t/t6300-for-each-ref.sh
+++ b/t/t6300-for-each-ref.sh
@@ -869,4 +869,16 @@ test_expect_success 'for-each-ref --ignore-case ignores case' '
 	test_cmp expect actual
 '
 
+test_expect_success 'show a taggerless tag' '
+	test_commit tagged &&
+	git tag -a -m "a normal tag" to-be-shown-0 HEAD &&
+	another=$(git cat-file tag to-be-shown-0 |
+		sed -e "/^tagger /d" \
+		    -e "/^tag to-be-shown/s/0/1/" \
+		    -e "s/a normal tag/a broken tag/" |
+		git hash-object --stdin -w -t tag) &&
+	git tag to-be-shown-1 $another &&
+	git for-each-ref --format="%(refname:short) %(taggername)" refs/tags/to-be-shown\*
+'
+
 test_done
Mischa POSLAWSKY Aug. 22, 2019, 1:23 p.m. UTC | #4
Junio wrote:
> Junio C Hamano <gitster@pobox.com> writes:
> 
> > Mischa POSLAWSKY <git@shiar.nl> writes:
> >
> >> Formatting $(taggername) on headerless tags such as v0.99 in Git
> >> causes a SIGABRT with error "munmap_chunk(): invalid pointer",
> >> because of an oversight in commit f0062d3b74 (ref-filter: free
> >> item->value and item->value->s, 2018-10-19).
> >>
> >> Signed-off-by: Mischa POSLAWSKY <git@shiar.nl>
> >> ---
> >> If I understand correctly, such tags cannot be produced normally anymore.
> >> Therefore I'm unsure how to make tests, and if that is even warranted.
> >
> > Thanks for spotting.
> >
> > I am not sure if the approach taken by this patch is the right one,
> > though.  I didn't follow the call/dataflow thoroughly, but if we
> > replace unfree-able "" with NULL in these places, wouldn't
> > fill_missing_values() take care of them?
> 
> I think replacing these "" with NULL would be safe, but there are
> many places that return xstrdup("") from inside the callees of
> populate_value(), so the patch presented here would be more
> consistent with the current practice, I think.

Indeed, I just copied the existing style.  Returning NULL seems to work,
but not something I'm confident to clean up here.

> So let's take the patch as is, at least for now.  Thanks.

Thank you!

> >>  ref-filter.c | 6 +++---
> >>  1 file changed, 3 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/ref-filter.c b/ref-filter.c
> >> index f27cfc8c3e..7338cfc671 100644
> >> --- a/ref-filter.c
> >> +++ b/ref-filter.c
> >> @@ -1028,7 +1028,7 @@ static const char *copy_name(const char *buf)
> >>  		if (!strncmp(cp, " <", 2))
> >>  			return xmemdupz(buf, cp - buf);
> >>  	}
> >> -	return "";
> >> +	return xstrdup("");
> >>  }
> >>  
> >>  static const char *copy_email(const char *buf)
> >> @@ -1036,10 +1036,10 @@ static const char *copy_email(const char *buf)
> >>  	const char *email = strchr(buf, '<');
> >>  	const char *eoemail;
> >>  	if (!email)
> >> -		return "";
> >> +		return xstrdup("");
> >>  	eoemail = strchr(email, '>');
> >>  	if (!eoemail)
> >> -		return "";
> >> +		return xstrdup("");
> >>  	return xmemdupz(email, eoemail + 1 - email);
> >>  }

Patch
diff mbox series

diff --git a/ref-filter.c b/ref-filter.c
index f27cfc8c3e..7338cfc671 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1028,7 +1028,7 @@  static const char *copy_name(const char *buf)
 		if (!strncmp(cp, " <", 2))
 			return xmemdupz(buf, cp - buf);
 	}
-	return "";
+	return xstrdup("");
 }
 
 static const char *copy_email(const char *buf)
@@ -1036,10 +1036,10 @@  static const char *copy_email(const char *buf)
 	const char *email = strchr(buf, '<');
 	const char *eoemail;
 	if (!email)
-		return "";
+		return xstrdup("");
 	eoemail = strchr(email, '>');
 	if (!eoemail)
-		return "";
+		return xstrdup("");
 	return xmemdupz(email, eoemail + 1 - email);
 }