| Message ID | 20190817215107.13733-1-git@shiar.nl (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ref-filter: initialize empty name or email fields | expand |
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 <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 <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
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); > >> }
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); }
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(-)