threads / patch / 51660

patchref-filter: initialize empty name or email fields

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

## tl;dr

9 messages between Aug 17, 2019 and Aug 22, 2019. Diffs are folded; open one to read it.

replies: 8people: 2as markdown or json

Mischa POSLAWSKY· Aug 17, 2019, 21:51 UTC · lore

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(-)
Show changes to ref-filter.c +3 −3
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
Junio C Hamano· Aug 19, 2019, 17:55 UTC · re: Mischa POSLAWSKY · lore

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

Mischa POSLAWSKY <git@shiar.nl> writes:
Show 9 quoted lines
> 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?

Show 28 quoted lines
>  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, 16:37 UTC · re: Junio C Hamano · lore

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

Junio C Hamano <gitster@pobox.com> writes:
Show 18 quoted lines
> 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.
Show 28 quoted lines
>>  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);
>>  }
Mischa POSLAWSKY· Aug 22, 2019, 13:23 UTC · re: Junio C Hamano · lore

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

Junio wrote:
Show 25 quoted lines
> 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!
Show 28 quoted lines
> >>  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, 21:57 UTC · re: Junio C Hamano · lore

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

Junio C Hamano <gitster@pobox.com> writes:
Show 6 quoted lines
> 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(+)
Show changes to t/t6300-for-each-ref.sh +12 −0
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, 13:55 UTC · re: Junio C Hamano · lore

[PATCH 2/1] t6300: format missing tagger

Junio wrote:
Show 52 quoted lines
> 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(+)
Show changes to t/t6300-for-each-ref.sh +19 −0
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
Junio C Hamano· Aug 22, 2019, 16:15 UTC · re: Mischa POSLAWSKY · lore

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

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

Show 5 quoted lines
> -- >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).

Show 36 quoted lines
>
> 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 &&
Mischa POSLAWSKY· Aug 22, 2019, 16:27 UTC · re: Junio C Hamano · lore

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

Junio wrote:
Show 16 quoted lines
> 
> 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 :)
Junio C Hamano· Aug 22, 2019, 18:05 UTC · re: Mischa POSLAWSKY · lore

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

Mischa POSLAWSKY <git@shiar.nl> writes:
Show 19 quoted lines
> 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.

← back to recent threads