[PATCH] ref-filter: initialize empty name or email fields

[PATCH] ref-filter: initialize empty name or email fields

[Mischa POSLAWSKY]

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(-)

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);
 }
 
-- 
2.23.0

Re: [PATCH] ref-filter: initialize empty name or email fields

[Junio C Hamano]

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);
>  }

Re: [PATCH] ref-filter: initialize empty name or email fields

[Junio C Hamano]

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);
>>  }

Re: [PATCH] ref-filter: initialize empty name or email fields

[Junio C Hamano]

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

Re: [PATCH] ref-filter: initialize empty name or email fields

[Mischa POSLAWSKY]

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 2/1] t6300: format missing tagger

[Mischa POSLAWSKY]

Junio wrote:
> 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
> 

Alright, thanks for the pointer.
Here's a batch of tests on all pertaining atoms.

-- >8 --

Strip an annotated tag of its tagger header and verify it's ignored
correctly in all cases, as fixed in commit e2a81276e8 (ref-filter:
initialize empty name or email fields, 2019-08-19).

Signed-off-by: Mischa POSLAWSKY <git@shiar.nl>
---
 t/t6300-for-each-ref.sh | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
index ab69aa176d..9c910ce746 100755
--- a/t/t6300-for-each-ref.sh
+++ b/t/t6300-for-each-ref.sh
@@ -526,6 +526,25 @@ test_expect_success 'Check ambiguous head and tag refs II (loose)' '
 	test_cmp expected actual
 '
 
+test_expect_success 'create tag without tagger' '
+	git tag -a -m "Broken tag" taggerless &&
+	git tag -f taggerless $(git cat-file tag taggerless |
+		sed -e "/^tagger /d" |
+		git hash-object --stdin -w -t tag)
+'
+
+test_atom refs/tags/taggerless type 'commit'
+test_atom refs/tags/taggerless tag 'taggerless'
+test_atom refs/tags/taggerless tagger ''
+test_atom refs/tags/taggerless taggername ''
+test_atom refs/tags/taggerless taggeremail ''
+test_atom refs/tags/taggerless taggerdate ''
+test_atom refs/tags/taggerless committer ''
+test_atom refs/tags/taggerless committername ''
+test_atom refs/tags/taggerless committeremail ''
+test_atom refs/tags/taggerless committerdate ''
+test_atom refs/tags/taggerless subject 'Broken tag'
+
 test_expect_success 'an unusual tag with an incomplete line' '
 
 	git tag -m "bogo" bogo &&
-- 
2.23.0

Re: [PATCH 2/1] t6300: format missing tagger

[Junio C Hamano]

Mischa POSLAWSKY <git@shiar.nl> writes:

> Alright, thanks for the pointer.
> Here's a batch of tests on all pertaining atoms.

Good to see that you made it much more thorough than my q-n-d
illustration patch ;-)

> -- >8 --
>
> Strip an annotated tag of its tagger header and verify it's ignored
> correctly in all cases, as fixed in commit e2a81276e8 (ref-filter:
> initialize empty name or email fields, 2019-08-19).

I am inclined to squash this test part of the update into the said
commit; you'd lose one commit count, but hopefully you do not mind?

My motivation for doing so is that it would allow us to lose the "as
fixed in commit X" comment in a log message, which in turn would
mean that the code-fix patch can later be rebased safely without
having to remember that this one needs to be adjusted ("git rebase"
does not do such a rewrite for us, and I personally do not think
"git rebase" should do such a rewrite silently, as I cannot quantify
the risk of false positives).

>
> Signed-off-by: Mischa POSLAWSKY <git@shiar.nl>
> ---
>  t/t6300-for-each-ref.sh | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
>
> diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
> index ab69aa176d..9c910ce746 100755
> --- a/t/t6300-for-each-ref.sh
> +++ b/t/t6300-for-each-ref.sh
> @@ -526,6 +526,25 @@ test_expect_success 'Check ambiguous head and tag refs II (loose)' '
>  	test_cmp expected actual
>  '
>  
> +test_expect_success 'create tag without tagger' '
> +	git tag -a -m "Broken tag" taggerless &&
> +	git tag -f taggerless $(git cat-file tag taggerless |
> +		sed -e "/^tagger /d" |
> +		git hash-object --stdin -w -t tag)
> +'
> +
> +test_atom refs/tags/taggerless type 'commit'
> +test_atom refs/tags/taggerless tag 'taggerless'
> +test_atom refs/tags/taggerless tagger ''
> +test_atom refs/tags/taggerless taggername ''
> +test_atom refs/tags/taggerless taggeremail ''
> +test_atom refs/tags/taggerless taggerdate ''
> +test_atom refs/tags/taggerless committer ''
> +test_atom refs/tags/taggerless committername ''
> +test_atom refs/tags/taggerless committeremail ''
> +test_atom refs/tags/taggerless committerdate ''
> +test_atom refs/tags/taggerless subject 'Broken tag'
> +
>  test_expect_success 'an unusual tag with an incomplete line' '
>  
>  	git tag -m "bogo" bogo &&

Re: [PATCH 2/1] t6300: format missing tagger

[Mischa POSLAWSKY]

Junio wrote:
> 
> Mischa POSLAWSKY <git@shiar.nl> writes:
> > Strip an annotated tag of its tagger header and verify it's ignored
> > correctly in all cases, as fixed in commit e2a81276e8 (ref-filter:
> > initialize empty name or email fields, 2019-08-19).
> 
> I am inclined to squash this test part of the update into the said
> commit; you'd lose one commit count, but hopefully you do not mind?
> 
> My motivation for doing so is that it would allow us to lose the "as
> fixed in commit X" comment in a log message, which in turn would
> mean that the code-fix patch can later be rebased safely without
> having to remember that this one needs to be adjusted ("git rebase"
> does not do such a rewrite for us, and I personally do not think
> "git rebase" should do such a rewrite silently, as I cannot quantify
> the risk of false positives).

Of course.  Might get one commit back if you pick it into maint :)

Re: [PATCH 2/1] t6300: format missing tagger

[Junio C Hamano]

Mischa POSLAWSKY <git@shiar.nl> writes:

> Junio wrote:
>> 
>> Mischa POSLAWSKY <git@shiar.nl> writes:
>> > Strip an annotated tag of its tagger header and verify it's ignored
>> > correctly in all cases, as fixed in commit e2a81276e8 (ref-filter:
>> > initialize empty name or email fields, 2019-08-19).
>> 
>> I am inclined to squash this test part of the update into the said
>> commit; you'd lose one commit count, but hopefully you do not mind?
>> 
>> My motivation for doing so is that it would allow us to lose the "as
>> fixed in commit X" comment in a log message, which in turn would
>> mean that the code-fix patch can later be rebased safely without
>> having to remember that this one needs to be adjusted ("git rebase"
>> does not do such a rewrite for us, and I personally do not think
>> "git rebase" should do such a rewrite silently, as I cannot quantify
>> the risk of false positives).
>
> Of course.  Might get one commit back if you pick it into maint :)

Actually you won't; I generally do not cherry-pick, even though I
merge down relevant fixes to older maintenance tracks.