{"thread":{"id":"62089","subject":"[PATCH 0/9] ref-filter %(trailer) fixes","startedAt":"2024-09-09T23:08:00Z","lastAt":"2024-09-12T20:24:16Z","messageCount":27,"participants":["Jeff King","Patrick Steinhardt","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":9},"messages":[{"id":"502480","messageId":"20240909230758.GA921697@coredump.intra.peff.net","threadId":"62089","inReplyTo":null,"subject":"[PATCH 0/9] ref-filter %(trailer) fixes","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-09-09T23:07:58Z","receivedAt":"2024-09-09T23:08:00Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This series fixes two bugs noticed by Brooke:\n\n  - parsing trailers from signed tags doesn't work, because the\n    signature confuses the trailers code\n\n  - multiple %(trailers) placeholders share some storage, so their\n    options may conflict\n\nThe fixes for those are in patches 3 and 5, respectively. The other\npatches up to there are related cleanups and preparation.\n\nWhen fixing the second one, I noticed an obvious memory leak, fixed in\npatch 6. And then that made me wonder if that made t6300 leak-free. It\ndidn't, but patches 7-9 get it there.\n\nSo 6-9 could be taken as a separate series, but they do textually depend\non what came before.\n\n  [1/9]: t6300: drop newline from wrapped test title\n  [2/9]: ref-filter: avoid extra copies of payload/signature\n  [3/9]: ref-filter: strip signature when parsing tag trailers\n  [4/9]: ref-filter: drop useless cast in trailers_atom_parser()\n  [5/9]: ref-filter: store ref_trailer_buf data per-atom\n  [6/9]: ref-filter: fix leak of %(trailers) \"argbuf\"\n  [7/9]: ref-filter: fix leak with %(describe) arguments\n  [8/9]: ref-filter: fix leak when formatting %(push:remoteref)\n  [9/9]: ref-filter: add ref_format_clear() function\n\n builtin/branch.c        |  1 +\n builtin/for-each-ref.c  |  1 +\n builtin/tag.c           |  1 +\n builtin/verify-tag.c    |  1 +\n ref-filter.c            | 90 ++++++++++++++++++++++++++++++-----------\n ref-filter.h            |  3 ++\n remote.c                |  8 ++--\n remote.h                |  2 +-\n t/t6300-for-each-ref.sh | 41 ++++++++++++++++++-\n 9 files changed, 118 insertions(+), 30 deletions(-)\n\n-Peff\n"},{"id":"502481","messageId":"20240909230841.GA921834@coredump.intra.peff.net","threadId":"62089","inReplyTo":"20240909230758.GA921697@coredump.intra.peff.net","subject":"[PATCH 1/9] t6300: drop newline from wrapped test title","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-09-09T23:08:41Z","receivedAt":"2024-09-09T23:08:43Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We don't usually include newlines in test titles, because you get funny\nTAP output like:\n\n  ok 417 - show good signature with custom format\n  ok 418 - show good signature with custom format\n  \t\t\t    with ssh\n  ok 419 - signature atom with grade option and bad signature\n\nwhere a TAP parser would ignore the extra line anyway, giving the wrong\ntitle. This comes from 26c9c03f0a (ref-filter: add new \"signature\" atom,\n2023-06-04), and I think it was probably just editor line wrapping.\n\nI checked for other cases with:\n\n  git grep \"test_expect_success [A-Z_,]* '[^']*$\"\n  git grep 'test_expect_success [A-Z_,]* \"[^\"]*$'\n\nbut this was the only hit.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nJust an annoyance I noticed while running t6300 over and over.\n\n t/t6300-for-each-ref.sh | 3 +--\n 1 file changed, 1 insertion(+), 2 deletions(-)\n\ndiff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh\nindex 8d15713cc6..7c208e20a6 100755\n--- a/t/t6300-for-each-ref.sh\n+++ b/t/t6300-for-each-ref.sh\n@@ -2003,8 +2003,7 @@ test_expect_success GPG 'show good signature with custom format' '\n \t\t--format=\"$GRADE_FORMAT\" >actual &&\n \ttest_cmp expect actual\n '\n-test_expect_success GPGSSH 'show good signature with custom format\n-\t\t\t    with ssh' '\n+test_expect_success GPGSSH 'show good signature with custom format with ssh' '\n \ttest_config gpg.ssh.allowedSignersFile \"${GPGSSH_ALLOWED_SIGNERS}\" &&\n \tFINGERPRINT=$(ssh-keygen -lf \"${GPGSSH_KEY_PRIMARY}\" | awk \"{print \\$2;}\") &&\n \tcat >expect.tmpl <<-\\EOF &&\n-- \n2.46.0.867.gf99b2b8e0f\n\n"},{"id":"502482","messageId":"20240909231228.GB921834@coredump.intra.peff.net","threadId":"62089","inReplyTo":"20240909230758.GA921697@coredump.intra.peff.net","subject":"[PATCH 2/9] ref-filter: avoid extra copies of payload/signature","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-09-09T23:12:28Z","receivedAt":"2024-09-09T23:12:29Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When we know we're going to show the subject or body of a tag or commit,\nwe call find_subpos(), which returns pointers and lengths for the three\nparts: subject, body, signature.\n\nOddly, the function finds the signature twice: once by calling\nparse_signature() at the start, which copies the signature into a\nseparate strbuf, and then again by calling parse_signed_buffer() after\nwe've parsed past the subject.\n\nThis is due to 482c119186 (gpg-interface: improve interface for parsing\ntags, 2021-02-11) and 88bce0e24c (ref-filter: hoist signature parsing,\n2021-02-11). The idea is that in a multi-hash world, tag signatures may\nappear in the header, rather than at the end of the body, in which case\nwe need to extract them into a separate buffer.\n\nBut parse_signature() would never find such a buffer! It only looks for\nsignature lines (like \"-----BEGIN PGP\") at the start of each line,\nwithout any header keyword. So this code will never find anything except\nthe usual in-body signature.\n\nAnd the extra code has two downsides:\n\n  1. We spend time copying the payload and signature into strbufs. That\n     might even be useful if we ended up with a NUL-terminated copy of\n     the payload data, but we throw it away immediately. And the\n     signature, since it comes at the end of the message, is already its\n     own NUL-terminated buffer.\n\n     The overhead isn't huge, but I measured a pretty consistent 1-2%\n     speedup running \"git for-each-ref --format='%(subject)'\" with this\n     patch on a clone of linux.git.\n\n  2. The output of find_subpos() is a set of three ptr/len combinations,\n     but only two of them point into the original buffer. This makes the\n     interface confusing: you can't do pointer comparisons between them,\n     and you have to remember to free the signature buffer. Since\n     there's only one caller, it's not too bad in practice, but it did\n     bite me while working on the next patch (and simplifying it will\n     pave the way for that).\n\nIn the long run we might have to go back to something like this\napproach, if we do have multi-hash header signatures. But I would argue\nthat the extra buffer should kick in only for a header signature, and be\npassed out of find_subpos() separately.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nThis should produce no behavior change.\n\nIt does seem funny to me that we do this signature parsing for commits\nas well as tags, even through the former would have an in-header\nsignature. So arguably we are wrongly cutting in-body signatures from\ncommits, and not showing their signatures with %(contents:signature).\n(But note that %(signature) is its own thing and does handle commits\ncorrectly).\n\nSo I don't know if we'd want to fix that or not, but I left it as-is in\nthis series. And as I said above, if we did want to go that way, I think\nwe'd still want to build it on top of this, so that find_subpos()\nreturns the in-header and in-body signatures separately.\n\n ref-filter.c | 11 +++--------\n 1 file changed, 3 insertions(+), 8 deletions(-)\n\ndiff --git a/ref-filter.c b/ref-filter.c\nindex b6c6c10127..0f5513ba7e 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -1833,16 +1833,10 @@ static void find_subpos(const char *buf,\n \t\t\tsize_t *nonsiglen,\n \t\t\tconst char **sig, size_t *siglen)\n {\n-\tstruct strbuf payload = STRBUF_INIT;\n-\tstruct strbuf signature = STRBUF_INIT;\n \tconst char *eol;\n \tconst char *end = buf + strlen(buf);\n \tconst char *sigstart;\n \n-\t/* parse signature first; we might not even have a subject line */\n-\tparse_signature(buf, end - buf, &payload, &signature);\n-\tstrbuf_release(&payload);\n-\n \t/* skip past header until we hit empty line */\n \twhile (*buf && *buf != '\\n') {\n \t\teol = strchrnul(buf, '\\n');\n@@ -1853,8 +1847,10 @@ static void find_subpos(const char *buf,\n \t/* skip any empty lines */\n \twhile (*buf == '\\n')\n \t\tbuf++;\n-\t*sig = strbuf_detach(&signature, siglen);\n+\t/* parse signature first; we might not even have a subject line */\n \tsigstart = buf + parse_signed_buffer(buf, strlen(buf));\n+\t*sig = sigstart;\n+\t*siglen = end - *sig;\n \n \t/* subject is first non-empty line */\n \t*sub = buf;\n@@ -2021,7 +2017,6 @@ static void grab_sub_body_contents(struct atom_value *val, int deref, struct exp\n \t\t\tv->s = xstrdup(subpos);\n \n \t}\n-\tfree((void *)sigpos);\n }\n \n /*\n-- \n2.46.0.867.gf99b2b8e0f\n\n"},{"id":"502483","messageId":"20240909231445.GC921834@coredump.intra.peff.net","threadId":"62089","inReplyTo":"20240909230758.GA921697@coredump.intra.peff.net","subject":"[PATCH 3/9] ref-filter: strip signature when parsing tag trailers","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-09-09T23:14:45Z","receivedAt":"2024-09-09T23:14:47Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"To expand the \"%(trailers)\" placeholder, we have to feed the commit or\ntag body to the trailer API. But that API doesn't know anything about\nsignatures, and will be confused by a signed tag like this:\n\n  the subject\n\n  the body\n\n  Some-trailer: foo\n  -----BEGIN PGP SIGNATURE-----\n  ...etc...\n\nbecause it will start looking for trailers after the signature, and get\nstopped walking backwards by the very non-trailer signature lines. So it\nthinks there are no trailers.\n\nThis problem has existed since %(trailers) was added to the ref-filter\ncode, but back then trailers on tags weren't something we really\nconsidered (commits don't have the same problem because their signatures\nare embedded in the header). But since 066cef7707 (builtin/tag: add\n--trailer option, 2024-05-05), we'd generate an object like the above\nfor \"git tag -s --trailer 'Some-trailer: foo' my-tag\".\n\nThe implementation here is pretty simple: we just make a NUL-terminated\ncopy of the non-signature part of the tag (which we've already parsed)\nand pass it to the trailer API. There are some alternatives I rejected,\nat least for now:\n\n  - the trailer code already understands skipping past some cruft at the\n    end of a commit, such as patch dividers. see find_end_of_log_message().\n    We could teach it to do the same for signatures. But since this is\n    the only context where we'd want that feature, and since we've already\n    parsed the object into subject/body/signature here, it seemed easier\n    to just pass in the truncated message.\n\n  - it would be nice if we could just pass in a pointer/len pair to the\n    trailer API (rather than a NUL-terminated string) to avoid the extra\n    copy. I think this is possible, since as noted above, the trailer\n    code already has to deal with ignoring some cruft at the end of the\n    input. But after an initial attempt at this, it got pretty messy, as\n    we have to touch a lot of intermediate functions that are also\n    called in other contexts.\n\n    So I went for the simple and stupid thing, at least for now. I don't\n    think the extra copy overhead will be all that bad. The previous\n    patch noted that an extra copy seemed to cause about 1-2% slowdown\n    for something simple like \"%(subject)\". But here we are only\n    triggering it for \"%(trailers)\" (and only when there is a\n    signature), and the trailer code is a bit allocation-heavy already.\n    I couldn't measure any difference formatting \"%(trailers)\" on\n    linux.git before and after (even though there are not even any\n    trailers to find).\n\nReported-by: Brooke Kuhlmann <brooke@alchemists.io>\nSigned-off-by: Jeff King <peff@peff.net>\n---\n ref-filter.c            | 10 +++++++++-\n t/t6300-for-each-ref.sh | 18 ++++++++++++++++++\n 2 files changed, 27 insertions(+), 1 deletion(-)\n\ndiff --git a/ref-filter.c b/ref-filter.c\nindex 0f5513ba7e..e39f505a81 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -2008,9 +2008,17 @@ static void grab_sub_body_contents(struct atom_value *val, int deref, struct exp\n \t\t\tv->s = strbuf_detach(&s, NULL);\n \t\t} else if (atom->u.contents.option == C_TRAILERS) {\n \t\t\tstruct strbuf s = STRBUF_INIT;\n+\t\t\tconst char *msg;\n+\t\t\tchar *to_free = NULL;\n+\n+\t\t\tif (siglen)\n+\t\t\t\tmsg = to_free = xmemdupz(subpos, sigpos - subpos);\n+\t\t\telse\n+\t\t\t\tmsg = subpos;\n \n \t\t\t/* Format the trailer info according to the trailer_opts given */\n-\t\t\tformat_trailers_from_commit(&atom->u.contents.trailer_opts, subpos, &s);\n+\t\t\tformat_trailers_from_commit(&atom->u.contents.trailer_opts, msg, &s);\n+\t\t\tfree(to_free);\n \n \t\t\tv->s = strbuf_detach(&s, NULL);\n \t\t} else if (atom->u.contents.option == C_BARE)\ndiff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh\nindex 7c208e20a6..b830b542c4 100755\n--- a/t/t6300-for-each-ref.sh\n+++ b/t/t6300-for-each-ref.sh\n@@ -1835,6 +1835,24 @@ sig_crlf=\"$(printf \"%s\" \"$sig\" | append_cr; echo dummy)\"\n sig_crlf=${sig_crlf%dummy}\n test_atom refs/tags/fake-sig-crlf contents:signature \"$sig_crlf\"\n \n+test_expect_success 'set up tag with signature and trailers' '\n+\tgit tag -F - fake-sig-trailer <<-\\EOF\n+\tthis is the subject\n+\n+\tthis is the body\n+\n+\tMy-Trailer: foo\n+\t-----BEGIN PGP SIGNATURE-----\n+\n+\tnot a real signature, but we just care about the\n+\tsubject/body/trailer parsing.\n+\t-----END PGP SIGNATURE-----\n+\tEOF\n+'\n+\n+# use \"separator=\" here to suppress the terminating newline\n+test_atom refs/tags/fake-sig-trailer trailers:separator= 'My-Trailer: foo'\n+\n test_expect_success 'git for-each-ref --stdin: empty' '\n \t>in &&\n \tgit for-each-ref --format=\"%(refname)\" --stdin <in >actual &&\n-- \n2.46.0.867.gf99b2b8e0f\n\n"},{"id":"502484","messageId":"20240909231459.GD921834@coredump.intra.peff.net","threadId":"62089","inReplyTo":"20240909230758.GA921697@coredump.intra.peff.net","subject":"[PATCH 4/9] ref-filter: drop useless cast in trailers_atom_parser()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-09-09T23:14:59Z","receivedAt":"2024-09-09T23:15:01Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"There's no need to cast invalid_arg before freeing it. It is already a\nnon-const pointer.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n ref-filter.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/ref-filter.c b/ref-filter.c\nindex e39f505a81..4d0df546da 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -578,7 +578,7 @@ static int trailers_atom_parser(struct ref_format *format UNUSED,\n \t\t\t\tstrbuf_addf(err, _(\"expected %%(trailers:key=<value>)\"));\n \t\t\telse\n \t\t\t\tstrbuf_addf(err, _(\"unknown %%(trailers) argument: %s\"), invalid_arg);\n-\t\t\tfree((char *)invalid_arg);\n+\t\t\tfree(invalid_arg);\n \t\t\treturn -1;\n \t\t}\n \t}\n-- \n2.46.0.867.gf99b2b8e0f\n\n"},{"id":"502485","messageId":"20240909231653.GE921834@coredump.intra.peff.net","threadId":"62089","inReplyTo":"20240909230758.GA921697@coredump.intra.peff.net","subject":"[PATCH 5/9] ref-filter: store ref_trailer_buf data per-atom","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-09-09T23:16:53Z","receivedAt":"2024-09-09T23:16:55Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The trailer API takes options via a trailer_opts struct. Some of those\noptions point to data structures which require extra storage. Those\nstructures aren't actually embedded in the options struct, but rather we\npass pointers, and the caller is responsible for managing them. This is\na little convoluted, but makes sense since some of them are not even\nconcrete (e.g., you can pass a filter function and a void data pointer,\nbut the trailer code doesn't even know what's in the pointer).\n\nWhen for-each-ref, etc, parse the %(trailers) placeholder, they stuff\nthe extra data into a ref_trailer_buf struct. But we only hold a single\nstatic global instance of this struct. So if a format string has\nmultiple %(trailer) placeholders, they'll stomp on each other: the \"key\"\nlist will end up with entries for all of them, and the separator buffers\nwill use the values from whichever was parsed last.\n\nInstead, we should have a ref_trailer_buf for each instance of the\nplaceholder, and store it alongside the trailer_opts in the used_atom\nstructure.\n\nAnd that's what this patch does. Note that we also have to add code to\nclean them up in ref_array_clear(). The original code did not bother\ncleaning them up, but it wasn't technically a \"leak\" since they were\nstill reachable from the static global instance.\n\nReported-by: Brooke Kuhlmann <brooke@alchemists.io>\nSigned-off-by: Jeff King <peff@peff.net>\n---\n ref-filter.c            | 36 ++++++++++++++++++++++++++++++------\n t/t6300-for-each-ref.sh | 19 +++++++++++++++++++\n 2 files changed, 49 insertions(+), 6 deletions(-)\n\ndiff --git a/ref-filter.c b/ref-filter.c\nindex 4d0df546da..ebddc041c7 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -75,11 +75,11 @@ struct refname_atom {\n \tint lstrip, rstrip;\n };\n \n-static struct ref_trailer_buf {\n+struct ref_trailer_buf {\n \tstruct string_list filter_list;\n \tstruct strbuf sepbuf;\n \tstruct strbuf kvsepbuf;\n-} ref_trailer_buf = {STRING_LIST_INIT_NODUP, STRBUF_INIT, STRBUF_INIT};\n+};\n \n static struct expand_data {\n \tstruct object_id oid;\n@@ -201,6 +201,7 @@ static struct used_atom {\n \t\t\tenum { C_BARE, C_BODY, C_BODY_DEP, C_LENGTH, C_LINES,\n \t\t\t       C_SIG, C_SUB, C_SUB_SANITIZE, C_TRAILERS } option;\n \t\t\tstruct process_trailer_options trailer_opts;\n+\t\t\tstruct ref_trailer_buf *trailer_buf;\n \t\t\tunsigned int nlines;\n \t\t} contents;\n \t\tstruct {\n@@ -568,12 +569,24 @@ static int trailers_atom_parser(struct ref_format *format UNUSED,\n \tif (arg) {\n \t\tconst char *argbuf = xstrfmt(\"%s)\", arg);\n \t\tchar *invalid_arg = NULL;\n+\t\tstruct ref_trailer_buf *tb;\n+\n+\t\t/*\n+\t\t * Do not inline these directly into the used_atom struct!\n+\t\t * When we parse them in format_set_trailers_options(),\n+\t\t * we will make pointer references directly to them,\n+\t\t * which will not survive a realloc() of the used_atom list.\n+\t\t * They must be allocated in a separate, stable struct.\n+\t\t */\n+\t\tatom->u.contents.trailer_buf = tb = xmalloc(sizeof(*tb));\n+\t\tstring_list_init_nodup(&tb->filter_list);\n+\t\tstrbuf_init(&tb->sepbuf, 0);\n+\t\tstrbuf_init(&tb->kvsepbuf, 0);\n \n \t\tif (format_set_trailers_options(&atom->u.contents.trailer_opts,\n-\t\t    &ref_trailer_buf.filter_list,\n-\t\t    &ref_trailer_buf.sepbuf,\n-\t\t    &ref_trailer_buf.kvsepbuf,\n-\t\t    &argbuf, &invalid_arg)) {\n+\t\t\t\t\t\t&tb->filter_list,\n+\t\t\t\t\t\t&tb->sepbuf, &tb->kvsepbuf,\n+\t\t\t\t\t\t&argbuf, &invalid_arg)) {\n \t\t\tif (!invalid_arg)\n \t\t\t\tstrbuf_addf(err, _(\"expected %%(trailers:key=<value>)\"));\n \t\t\telse\n@@ -2988,6 +3001,17 @@ void ref_array_clear(struct ref_array *array)\n \t\tstruct used_atom *atom = &used_atom[i];\n \t\tif (atom->atom_type == ATOM_HEAD)\n \t\t\tfree(atom->u.head);\n+\t\telse if (atom->atom_type == ATOM_TRAILERS ||\n+\t\t\t (atom->atom_type == ATOM_CONTENTS &&\n+\t\t\t  atom->u.contents.option == C_TRAILERS)) {\n+\t\t\tstruct ref_trailer_buf *tb = atom->u.contents.trailer_buf;\n+\t\t\tif (tb) {\n+\t\t\t\tstring_list_clear(&tb->filter_list, 0);\n+\t\t\t\tstrbuf_release(&tb->sepbuf);\n+\t\t\t\tstrbuf_release(&tb->kvsepbuf);\n+\t\t\t\tfree(tb);\n+\t\t\t}\n+\t\t}\n \t\tfree((char *)atom->name);\n \t}\n \tFREE_AND_NULL(used_atom);\ndiff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh\nindex b830b542c4..e8db612f95 100755\n--- a/t/t6300-for-each-ref.sh\n+++ b/t/t6300-for-each-ref.sh\n@@ -1560,6 +1560,25 @@ test_trailer_option '%(trailers:separator,key_value_separator) changes both sepa\n \tReviewed-by,A U Thor <author@example.com>,Signed-off-by,A U Thor <author@example.com>\n \tEOF\n \n+test_expect_success 'multiple %(trailers) use their own options' '\n+\tgit tag -F - tag-with-trailers <<-\\EOF &&\n+\tbody\n+\n+\tone: foo\n+\tone: bar\n+\ttwo: baz\n+\ttwo: qux\n+\tEOF\n+\tt1=\"%(trailers:key=one,key_value_separator=W,separator=X)\" &&\n+\tt2=\"%(trailers:key=two,key_value_separator=Y,separator=Z)\" &&\n+\tgit for-each-ref --format=\"$t1%0a$t2\" refs/tags/tag-with-trailers >actual &&\n+\tcat >expect <<-\\EOF &&\n+\toneWfooXoneWbar\n+\ttwoYbazZtwoYqux\n+\tEOF\n+\ttest_cmp expect actual\n+'\n+\n test_failing_trailer_option () {\n \ttitle=$1 option=$2\n \tcat >expect\n-- \n2.46.0.867.gf99b2b8e0f\n\n"},{"id":"502486","messageId":"20240909231828.GF921834@coredump.intra.peff.net","threadId":"62089","inReplyTo":"20240909230758.GA921697@coredump.intra.peff.net","subject":"[PATCH 6/9] ref-filter: fix leak of %(trailers) \"argbuf\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-09-09T23:18:28Z","receivedAt":"2024-09-09T23:18:29Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When we parse a placeholder like \"%(trailers:key=foo)\", our atom parsing\nfunction is passed just the argument string \"key=foo\". We duplicate this\ninto its own string, but never free it, causing a leak.\n\nWe do the duplication for two reasons:\n\n  1. There's a mismatch with the pretty.c trailer-formatting code that\n     we rely on. It expects to see a closing paren, like \"key=foo)\". So\n     we duplicate the argument string with that extra character to pass\n     along.\n\n     This is probably something we could fix in the long run, but it's\n     somewhat non-trivial if we want to avoid regressing error cases for\n     things like \"git log --format='%(trailer:oops'\". So let's accept\n     it as a necessity for now.\n\n  2. The argument parser expects to store the list of \"key\" entries\n     (\"foo\" in this case) in a string-list. It also stores the length of\n     the string in the string-list \"util\" field. The original caller in\n     pretty.c uses this with a \"nodup\" string list to avoid making extra\n     copies, which creates a subtle dependency on the lifetime of the\n     original format string.\n\n     We do the same here, which creates that same dependency. So we\n     can't simply free it as soon as the parsing is done.\n\nThere are two possible solutions here. The first is to hold on to the\nduplicated \"argbuf\" string in the used_atom struct, so that it lives as\nlong as the string_list which references it.\n\nBut I think a less-subtle solution, and what this patch does, is to\nswitch to a duplicating string_list. That makes it self-contained, and\nlets us free argbuf immediately. It may involve a few extra allocations,\nbut this parsing is something that happens once per program, not once\nper output ref.\n\nThis clears up one case that LSan finds in t6300, but there are more.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n ref-filter.c | 9 ++++++---\n 1 file changed, 6 insertions(+), 3 deletions(-)\n\ndiff --git a/ref-filter.c b/ref-filter.c\nindex ebddc041c7..54c5079dde 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -567,7 +567,8 @@ static int trailers_atom_parser(struct ref_format *format UNUSED,\n \tatom->u.contents.trailer_opts.no_divider = 1;\n \n \tif (arg) {\n-\t\tconst char *argbuf = xstrfmt(\"%s)\", arg);\n+\t\tchar *argbuf = xstrfmt(\"%s)\", arg);\n+\t\tconst char *arg = argbuf;\n \t\tchar *invalid_arg = NULL;\n \t\tstruct ref_trailer_buf *tb;\n \n@@ -579,21 +580,23 @@ static int trailers_atom_parser(struct ref_format *format UNUSED,\n \t\t * They must be allocated in a separate, stable struct.\n \t\t */\n \t\tatom->u.contents.trailer_buf = tb = xmalloc(sizeof(*tb));\n-\t\tstring_list_init_nodup(&tb->filter_list);\n+\t\tstring_list_init_dup(&tb->filter_list);\n \t\tstrbuf_init(&tb->sepbuf, 0);\n \t\tstrbuf_init(&tb->kvsepbuf, 0);\n \n \t\tif (format_set_trailers_options(&atom->u.contents.trailer_opts,\n \t\t\t\t\t\t&tb->filter_list,\n \t\t\t\t\t\t&tb->sepbuf, &tb->kvsepbuf,\n-\t\t\t\t\t\t&argbuf, &invalid_arg)) {\n+\t\t\t\t\t\t&arg, &invalid_arg)) {\n \t\t\tif (!invalid_arg)\n \t\t\t\tstrbuf_addf(err, _(\"expected %%(trailers:key=<value>)\"));\n \t\t\telse\n \t\t\t\tstrbuf_addf(err, _(\"unknown %%(trailers) argument: %s\"), invalid_arg);\n \t\t\tfree(invalid_arg);\n+\t\t\tfree(argbuf);\n \t\t\treturn -1;\n \t\t}\n+\t\tfree(argbuf);\n \t}\n \tatom->u.contents.option = C_TRAILERS;\n \treturn 0;\n-- \n2.46.0.867.gf99b2b8e0f\n\n"},{"id":"502487","messageId":"20240909231902.GG921834@coredump.intra.peff.net","threadId":"62089","inReplyTo":"20240909230758.GA921697@coredump.intra.peff.net","subject":"[PATCH 7/9] ref-filter: fix leak with %(describe) arguments","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-09-09T23:19:02Z","receivedAt":"2024-09-09T23:19:03Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When we parse a %(describe) placeholder, we stuff its arguments into a\nstrvec, which is then detached into the used_atom struct. But later,\nwhen ref_array_clear() frees the atom, we never free the memory.\n\nTo solve this, we just need to add the appropriate free() calls. But\nit's a little awkward, since we have to free each element of the array,\nin addition to the array itself. Instead, let's store the actual strvec,\nwhich lets us do a simple strvec_clear().\n\nThis clears up one case that LSan finds in t6300, but there are more.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n ref-filter.c | 11 ++++++-----\n 1 file changed, 6 insertions(+), 5 deletions(-)\n\ndiff --git a/ref-filter.c b/ref-filter.c\nindex 54c5079dde..370cc5b44a 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -233,7 +233,7 @@ static struct used_atom {\n \t\t\tenum { S_BARE, S_GRADE, S_SIGNER, S_KEY,\n \t\t\t       S_FINGERPRINT, S_PRI_KEY_FP, S_TRUST_LEVEL } option;\n \t\t} signature;\n-\t\tconst char **describe_args;\n+\t\tstruct strvec describe_args;\n \t\tstruct refname_atom refname;\n \t\tchar *head;\n \t} u;\n@@ -693,7 +693,7 @@ static int describe_atom_parser(struct ref_format *format UNUSED,\n \t\t\t\tstruct used_atom *atom,\n \t\t\t\tconst char *arg, struct strbuf *err)\n {\n-\tstruct strvec args = STRVEC_INIT;\n+\tstrvec_init(&atom->u.describe_args);\n \n \tfor (;;) {\n \t\tint found = 0;\n@@ -702,13 +702,12 @@ static int describe_atom_parser(struct ref_format *format UNUSED,\n \t\tif (!arg || !*arg)\n \t\t\tbreak;\n \n-\t\tfound = describe_atom_option_parser(&args, &arg, err);\n+\t\tfound = describe_atom_option_parser(&atom->u.describe_args, &arg, err);\n \t\tif (found < 0)\n \t\t\treturn found;\n \t\tif (!found)\n \t\t\treturn err_bad_arg(err, \"describe\", bad_arg);\n \t}\n-\tatom->u.describe_args = strvec_detach(&args);\n \treturn 0;\n }\n \n@@ -1941,7 +1940,7 @@ static void grab_describe_values(struct atom_value *val, int deref,\n \n \t\tcmd.git_cmd = 1;\n \t\tstrvec_push(&cmd.args, \"describe\");\n-\t\tstrvec_pushv(&cmd.args, atom->u.describe_args);\n+\t\tstrvec_pushv(&cmd.args, atom->u.describe_args.v);\n \t\tstrvec_push(&cmd.args, oid_to_hex(&commit->object.oid));\n \t\tif (pipe_command(&cmd, NULL, 0, &out, 0, &err, 0) < 0) {\n \t\t\terror(_(\"failed to run 'describe'\"));\n@@ -3004,6 +3003,8 @@ void ref_array_clear(struct ref_array *array)\n \t\tstruct used_atom *atom = &used_atom[i];\n \t\tif (atom->atom_type == ATOM_HEAD)\n \t\t\tfree(atom->u.head);\n+\t\telse if (atom->atom_type == ATOM_DESCRIBE)\n+\t\t\tstrvec_clear(&atom->u.describe_args);\n \t\telse if (atom->atom_type == ATOM_TRAILERS ||\n \t\t\t (atom->atom_type == ATOM_CONTENTS &&\n \t\t\t  atom->u.contents.option == C_TRAILERS)) {\n-- \n2.46.0.867.gf99b2b8e0f\n\n"},{"id":"502488","messageId":"20240909231951.GH921834@coredump.intra.peff.net","threadId":"62089","inReplyTo":"20240909230758.GA921697@coredump.intra.peff.net","subject":"[PATCH 8/9] ref-filter: fix leak when formatting %(push:remoteref)","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-09-09T23:19:51Z","receivedAt":"2024-09-09T23:19:53Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When we expand the %(upstream) or %(push) placeholders, we rely on\nremote.c's remote_ref_for_branch() to fill in the \":refname\" argument.\nBut that function has confusing memory ownership semantics: it may or\nmay not return an allocated string, depending on whether we are in\n\"upstream\" mode or \"push\" mode. The caller in ref-filter.c always\nduplicates the result, meaning that we leak the original in the case of\n%(push:refname).\n\nTo solve this, let's make the return value from remote_ref_for_branch()\nconsistent, by always returning an allocated pointer. Note that the\nswitch to returning a non-const pointer has a ripple effect inside the\nfunction, too. We were storing the \"dst\" result as a const pointer, too,\neven though it is always allocated! It is the return value from\napply_refspecs(), which is always a non-const allocated string.\n\nAnd then on the caller side in ref-filter.c (and this is the only caller\nat all), we just need to avoid the extra duplication when the return\nvalue is non-NULL.\n\nThis clears up one case that LSan finds in t6300, but there are more.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n ref-filter.c | 2 +-\n remote.c     | 8 ++++----\n remote.h     | 2 +-\n 3 files changed, 6 insertions(+), 6 deletions(-)\n\ndiff --git a/ref-filter.c b/ref-filter.c\nindex 370cc5b44a..0f51095bbd 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -2237,7 +2237,7 @@ static void fill_remote_ref_details(struct used_atom *atom, const char *refname,\n \t\tconst char *merge;\n \n \t\tmerge = remote_ref_for_branch(branch, atom->u.remote_ref.push);\n-\t\t*s = xstrdup(merge ? merge : \"\");\n+\t\t*s = merge ? merge : xstrdup(\"\");\n \t} else\n \t\tBUG(\"unhandled RR_* enum\");\n }\ndiff --git a/remote.c b/remote.c\nindex 8f3dee1318..539e5ceae3 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -632,19 +632,19 @@ const char *pushremote_for_branch(struct branch *branch, int *explicit)\n static struct remote *remotes_remote_get(struct remote_state *remote_state,\n \t\t\t\t\t const char *name);\n \n-const char *remote_ref_for_branch(struct branch *branch, int for_push)\n+char *remote_ref_for_branch(struct branch *branch, int for_push)\n {\n \tread_config(the_repository, 0);\n \tdie_on_missing_branch(the_repository, branch);\n \n \tif (branch) {\n \t\tif (!for_push) {\n \t\t\tif (branch->merge_nr) {\n-\t\t\t\treturn branch->merge_name[0];\n+\t\t\t\treturn xstrdup(branch->merge_name[0]);\n \t\t\t}\n \t\t} else {\n-\t\t\tconst char *dst,\n-\t\t\t\t*remote_name = remotes_pushremote_for_branch(\n+\t\t\tchar *dst;\n+\t\t\tconst char *remote_name = remotes_pushremote_for_branch(\n \t\t\t\t\tthe_repository->remote_state, branch,\n \t\t\t\t\tNULL);\n \t\t\tstruct remote *remote = remotes_remote_get(\ndiff --git a/remote.h b/remote.h\nindex b901b56746..a58713f20a 100644\n--- a/remote.h\n+++ b/remote.h\n@@ -329,7 +329,7 @@ struct branch {\n struct branch *branch_get(const char *name);\n const char *remote_for_branch(struct branch *branch, int *explicit);\n const char *pushremote_for_branch(struct branch *branch, int *explicit);\n-const char *remote_ref_for_branch(struct branch *branch, int for_push);\n+char *remote_ref_for_branch(struct branch *branch, int for_push);\n \n /* returns true if the given branch has merge configuration given. */\n int branch_has_merge_config(struct branch *branch);\n-- \n2.46.0.867.gf99b2b8e0f\n\n"},{"id":"502489","messageId":"20240909232118.GI921834@coredump.intra.peff.net","threadId":"62089","inReplyTo":"20240909230758.GA921697@coredump.intra.peff.net","subject":"[PATCH 9/9] ref-filter: add ref_format_clear() function","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-09-09T23:21:18Z","receivedAt":"2024-09-09T23:21:19Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"After using the ref-filter API, callers should use ref_filter_clear() to\nfree any used memory. However, there's not a matching function to clear\nthe ref_format struct.\n\nTraditionally this did not need to be cleaned up, as it was just a way\nfor the caller to store and pass format options as a single unit. Even\nthough the parsing step of some placeholders may allocate data, that's\nusually inside their \"used_atom\" structs, which are part of the\nref_filter itself.\n\nBut a few placeholders keep data outside of there. The %(ahead-behind)\nand %(is-base) parsers both keep a master list of bases, because they\nperform a single filtering pass outside of the use of any particular\natom. And since the format parser does not have access to the ref_filter\nstruct, they store their cross-atom data in the ref_format struct\nitself.\n\nAnd thus when they are finished, the ref_format also needs to be cleaned\nup. So let's add a function to do so, and call it from all of the users\nof the ref-filter API.\n\nThe %(is-base) case is found by running LSan on t6300. After this patch,\nthe script can now be marked leak-free.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/branch.c        |  1 +\n builtin/for-each-ref.c  |  1 +\n builtin/tag.c           |  1 +\n builtin/verify-tag.c    |  1 +\n ref-filter.c            | 13 +++++++++++++\n ref-filter.h            |  3 +++\n t/t6300-for-each-ref.sh |  1 +\n 7 files changed, 21 insertions(+)\n\ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex 3f870741bf..c98601c6fe 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -878,6 +878,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n \t\tstring_list_clear(&output, 0);\n \t\tref_sorting_release(sorting);\n \t\tref_filter_clear(&filter);\n+\t\tref_format_clear(&format);\n \t\treturn 0;\n \t} else if (edit_description) {\n \t\tconst char *branch_name;\ndiff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c\nindex 5517a4a1c0..c72fa05bcb 100644\n--- a/builtin/for-each-ref.c\n+++ b/builtin/for-each-ref.c\n@@ -104,6 +104,7 @@ int cmd_for_each_ref(int argc, const char **argv, const char *prefix)\n \tfilter_and_format_refs(&filter, flags, sorting, &format);\n \n \tref_filter_clear(&filter);\n+\tref_format_clear(&format);\n \tref_sorting_release(sorting);\n \tstrvec_clear(&vec);\n \treturn 0;\ndiff --git a/builtin/tag.c b/builtin/tag.c\nindex a1fb218512..607e48e311 100644\n--- a/builtin/tag.c\n+++ b/builtin/tag.c\n@@ -702,6 +702,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n cleanup:\n \tref_sorting_release(sorting);\n \tref_filter_clear(&filter);\n+\tref_format_clear(&format);\n \tstrbuf_release(&buf);\n \tstrbuf_release(&ref);\n \tstrbuf_release(&reflog_msg);\ndiff --git a/builtin/verify-tag.c b/builtin/verify-tag.c\nindex c731e2f87b..77becf7e75 100644\n--- a/builtin/verify-tag.c\n+++ b/builtin/verify-tag.c\n@@ -65,5 +65,6 @@ int cmd_verify_tag(int argc, const char **argv, const char *prefix)\n \t\tif (format.format)\n \t\t\tpretty_print_ref(name, &oid, &format);\n \t}\n+\tref_format_clear(&format);\n \treturn had_error;\n }\ndiff --git a/ref-filter.c b/ref-filter.c\nindex 0f51095bbd..ce1bcfad85 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -3621,3 +3621,16 @@ void ref_filter_clear(struct ref_filter *filter)\n \tfree_commit_list(filter->unreachable_from);\n \tref_filter_init(filter);\n }\n+\n+void ref_format_init(struct ref_format *format)\n+{\n+\tstruct ref_format blank = REF_FORMAT_INIT;\n+\tmemcpy(format, &blank, sizeof(blank));\n+}\n+\n+void ref_format_clear(struct ref_format *format)\n+{\n+\tstring_list_clear(&format->bases, 0);\n+\tstring_list_clear(&format->is_base_tips, 0);\n+\tref_format_init(format);\n+}\ndiff --git a/ref-filter.h b/ref-filter.h\nindex e794b8a676..754038ab07 100644\n--- a/ref-filter.h\n+++ b/ref-filter.h\n@@ -221,4 +221,7 @@ void filter_is_base(struct repository *r,\n void ref_filter_init(struct ref_filter *filter);\n void ref_filter_clear(struct ref_filter *filter);\n \n+void ref_format_init(struct ref_format *format);\n+void ref_format_clear(struct ref_format *format);\n+\n #endif /*  REF_FILTER_H  */\ndiff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh\nindex e8db612f95..b3163629c5 100755\n--- a/t/t6300-for-each-ref.sh\n+++ b/t/t6300-for-each-ref.sh\n@@ -5,6 +5,7 @@\n \n test_description='for-each-ref test'\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n GNUPGHOME_NOT_USED=$GNUPGHOME\n . \"$TEST_DIRECTORY\"/lib-gpg.sh\n-- \n2.46.0.867.gf99b2b8e0f\n"},{"id":"502530","messageId":"Zt_id3Fm6IXMhODF@pks.im","threadId":"62089","inReplyTo":"20240909231445.GC921834@coredump.intra.peff.net","subject":"Re: [PATCH 3/9] ref-filter: strip signature when parsing tag trailers","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-10T06:08:55Z","receivedAt":"2024-09-10T06:09:01Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Mon, Sep 09, 2024 at 07:14:45PM -0400, Jeff King wrote:\n> The implementation here is pretty simple: we just make a NUL-terminated\n> copy of the non-signature part of the tag (which we've already parsed)\n> and pass it to the trailer API. There are some alternatives I rejected,\n> at least for now:\n> \n>   - the trailer code already understands skipping past some cruft at the\n>     end of a commit, such as patch dividers. see find_end_of_log_message().\n\ns/./,\n\n>     We could teach it to do the same for signatures. But since this is\n>     the only context where we'd want that feature, and since we've already\n>     parsed the object into subject/body/signature here, it seemed easier\n>     to just pass in the truncated message.\n> \n>   - it would be nice if we could just pass in a pointer/len pair to the\n\ns/it/It\n\n> diff --git a/ref-filter.c b/ref-filter.c\n> index 0f5513ba7e..e39f505a81 100644\n> --- a/ref-filter.c\n> +++ b/ref-filter.c\n> @@ -2008,9 +2008,17 @@ static void grab_sub_body_contents(struct atom_value *val, int deref, struct exp\n>  \t\t\tv->s = strbuf_detach(&s, NULL);\n>  \t\t} else if (atom->u.contents.option == C_TRAILERS) {\n>  \t\t\tstruct strbuf s = STRBUF_INIT;\n> +\t\t\tconst char *msg;\n> +\t\t\tchar *to_free = NULL;\n> +\n> +\t\t\tif (siglen)\n> +\t\t\t\tmsg = to_free = xmemdupz(subpos, sigpos - subpos);\n> +\t\t\telse\n> +\t\t\t\tmsg = subpos;\n>  \n>  \t\t\t/* Format the trailer info according to the trailer_opts given */\n> -\t\t\tformat_trailers_from_commit(&atom->u.contents.trailer_opts, subpos, &s);\n> +\t\t\tformat_trailers_from_commit(&atom->u.contents.trailer_opts, msg, &s);\n> +\t\t\tfree(to_free);\n>  \n>  \t\t\tv->s = strbuf_detach(&s, NULL);\n>  \t\t} else if (atom->u.contents.option == C_BARE)\n\nI've been surprised that we use `subpos` as the starting point here,\nwhich includes the whole message including its subject. I would have\nthought that it was sufficient to only pass the message body as input,\nwhich saves allocating some bytes. At least `trailer_info_get()` does\nnot seem to care about the subject at all.\n\nIn any case this would be a micro-optimization anyway, it just left me\nscratching my head for a second or two.\n\nPatrick\n"},{"id":"502531","messageId":"Zt_ie4o3x8l6P64u@pks.im","threadId":"62089","inReplyTo":"20240909231653.GE921834@coredump.intra.peff.net","subject":"Re: [PATCH 5/9] ref-filter: store ref_trailer_buf data per-atom","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-10T06:08:59Z","receivedAt":"2024-09-10T06:09:03Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Mon, Sep 09, 2024 at 07:16:53PM -0400, Jeff King wrote:\n> @@ -2988,6 +3001,17 @@ void ref_array_clear(struct ref_array *array)\n>  \t\tstruct used_atom *atom = &used_atom[i];\n>  \t\tif (atom->atom_type == ATOM_HEAD)\n>  \t\t\tfree(atom->u.head);\n\nNit: this branch should grow some braces now, too.\n\nPatrick\n"},{"id":"502532","messageId":"Zt_ifxQq-6wNaQ3J@pks.im","threadId":"62089","inReplyTo":"20240909231828.GF921834@coredump.intra.peff.net","subject":"Re: [PATCH 6/9] ref-filter: fix leak of %(trailers) \"argbuf\"","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-10T06:09:03Z","receivedAt":"2024-09-10T06:09:06Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Mon, Sep 09, 2024 at 07:18:28PM -0400, Jeff King wrote:\n> When we parse a placeholder like \"%(trailers:key=foo)\", our atom parsing\n> function is passed just the argument string \"key=foo\". We duplicate this\n> into its own string, but never free it, causing a leak.\n> \n> We do the duplication for two reasons:\n> \n>   1. There's a mismatch with the pretty.c trailer-formatting code that\n>      we rely on. It expects to see a closing paren, like \"key=foo)\". So\n>      we duplicate the argument string with that extra character to pass\n>      along.\n> \n>      This is probably something we could fix in the long run, but it's\n>      somewhat non-trivial if we want to avoid regressing error cases for\n>      things like \"git log --format='%(trailer:oops'\". So let's accept\n>      it as a necessity for now.\n> \n>   2. The argument parser expects to store the list of \"key\" entries\n>      (\"foo\" in this case) in a string-list. It also stores the length of\n>      the string in the string-list \"util\" field. The original caller in\n>      pretty.c uses this with a \"nodup\" string list to avoid making extra\n>      copies, which creates a subtle dependency on the lifetime of the\n>      original format string.\n> \n>      We do the same here, which creates that same dependency. So we\n>      can't simply free it as soon as the parsing is done.\n> \n> There are two possible solutions here. The first is to hold on to the\n> duplicated \"argbuf\" string in the used_atom struct, so that it lives as\n> long as the string_list which references it.\n> \n> But I think a less-subtle solution, and what this patch does, is to\n> switch to a duplicating string_list. That makes it self-contained, and\n> lets us free argbuf immediately. It may involve a few extra allocations,\n> but this parsing is something that happens once per program, not once\n> per output ref.\n\nSensible. I found that in many cases, the `nodup` variants of string\nlists bring more pain than real benefit.\n\n> This clears up one case that LSan finds in t6300, but there are more.\n\nYeah, there are a bunch of memory leaks around atom parsing in general\nexposed by t6300. Thanks for plugging some of them!\n\nPatrick\n"},{"id":"502533","messageId":"Zt_ihDoECjCKczfN@pks.im","threadId":"62089","inReplyTo":"20240909231951.GH921834@coredump.intra.peff.net","subject":"Re: [PATCH 8/9] ref-filter: fix leak when formatting %(push:remoteref)","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-10T06:09:08Z","receivedAt":"2024-09-10T06:09:15Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Mon, Sep 09, 2024 at 07:19:51PM -0400, Jeff King wrote:\n> When we expand the %(upstream) or %(push) placeholders, we rely on\n> remote.c's remote_ref_for_branch() to fill in the \":refname\" argument.\n> But that function has confusing memory ownership semantics: it may or\n> may not return an allocated string, depending on whether we are in\n> \"upstream\" mode or \"push\" mode. The caller in ref-filter.c always\n> duplicates the result, meaning that we leak the original in the case of\n> %(push:refname).\n\nAh, I remember this issue, I think I also have it pending somewhere.\nAnyway, I'm happy if I can drop one more patch.\n\nThe change looks sensible to me.\n\nPatrick\n"},{"id":"502534","messageId":"Zt_ijEqdsylYYkNn@pks.im","threadId":"62089","inReplyTo":"20240909232118.GI921834@coredump.intra.peff.net","subject":"Re: [PATCH 9/9] ref-filter: add ref_format_clear() function","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-10T06:09:16Z","receivedAt":"2024-09-10T06:09:20Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Mon, Sep 09, 2024 at 07:21:18PM -0400, Jeff King wrote:\n> diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh\n> index e8db612f95..b3163629c5 100755\n> --- a/t/t6300-for-each-ref.sh\n> +++ b/t/t6300-for-each-ref.sh\n> @@ -5,6 +5,7 @@\n>  \n>  test_description='for-each-ref test'\n>  \n> +TEST_PASSES_SANITIZE_LEAK=true\n>  . ./test-lib.sh\n>  GNUPGHOME_NOT_USED=$GNUPGHOME\n>  . \"$TEST_DIRECTORY\"/lib-gpg.sh\n\nNice! There's also t6302, which has been failing due to all the memory\nleaks in our atom handling, as well. After your series there's a single\nmemory leak left to make it pass. So we may want to add below patch on\ntop as a low-hanging fruit.\n\nPatrick\n\n-- >8 --\n\ndiff --git a/ref-filter.c b/ref-filter.c\nindex ce1bcfad857..b06e18a569a 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -1001,6 +1001,7 @@ struct ref_formatting_stack {\n \tstruct ref_formatting_stack *prev;\n \tstruct strbuf output;\n \tvoid (*at_end)(struct ref_formatting_stack **stack);\n+\tvoid (*at_end_data_free)(void *data);\n \tvoid *at_end_data;\n };\n \n@@ -1169,6 +1170,8 @@ static void pop_stack_element(struct ref_formatting_stack **stack)\n \tif (prev)\n \t\tstrbuf_addbuf(&prev->output, &current->output);\n \tstrbuf_release(&current->output);\n+\tif (current->at_end_data_free)\n+\t\tcurrent->at_end_data_free(current->at_end_data);\n \tfree(current);\n \t*stack = prev;\n }\n@@ -1228,15 +1231,13 @@ static void if_then_else_handler(struct ref_formatting_stack **stack)\n \t}\n \n \t*stack = cur;\n-\tfree(if_then_else);\n }\n \n static int if_atom_handler(struct atom_value *atomv, struct ref_formatting_state *state,\n \t\t\t   struct strbuf *err UNUSED)\n {\n \tstruct ref_formatting_stack *new_stack;\n-\tstruct if_then_else *if_then_else = xcalloc(1,\n-\t\t\t\t\t\t    sizeof(struct if_then_else));\n+\tstruct if_then_else *if_then_else = xcalloc(1, sizeof(*if_then_else));\n \n \tif_then_else->str = atomv->atom->u.if_then_else.str;\n \tif_then_else->cmp_status = atomv->atom->u.if_then_else.cmp_status;\n@@ -1245,6 +1246,7 @@ static int if_atom_handler(struct atom_value *atomv, struct ref_formatting_state\n \tnew_stack = state->stack;\n \tnew_stack->at_end = if_then_else_handler;\n \tnew_stack->at_end_data = if_then_else;\n+\tnew_stack->at_end_data_free = free;\n \treturn 0;\n }\n \ndiff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh\nindex 163c378cfd1..7f44d3c3f22 100755\n--- a/t/t6302-for-each-ref-filter.sh\n+++ b/t/t6302-for-each-ref-filter.sh\n@@ -2,6 +2,7 @@\n \n test_description='test for-each-refs usage of ref-filter APIs'\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n . \"$TEST_DIRECTORY\"/lib-gpg.sh\n \n"},{"id":"502535","messageId":"Zt_ikkhuMrqVcFOx@pks.im","threadId":"62089","inReplyTo":"20240909231228.GB921834@coredump.intra.peff.net","subject":"Re: [PATCH 2/9] ref-filter: avoid extra copies of payload/signature","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-10T06:09:22Z","receivedAt":"2024-09-10T06:09:25Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Mon, Sep 09, 2024 at 07:12:28PM -0400, Jeff King wrote:\n> When we know we're going to show the subject or body of a tag or commit,\n> we call find_subpos(), which returns pointers and lengths for the three\n> parts: subject, body, signature.\n> \n> Oddly, the function finds the signature twice: once by calling\n> parse_signature() at the start, which copies the signature into a\n> separate strbuf, and then again by calling parse_signed_buffer() after\n> we've parsed past the subject.\n> \n> This is due to 482c119186 (gpg-interface: improve interface for parsing\n> tags, 2021-02-11) and 88bce0e24c (ref-filter: hoist signature parsing,\n> 2021-02-11). The idea is that in a multi-hash world, tag signatures may\n> appear in the header, rather than at the end of the body, in which case\n> we need to extract them into a separate buffer.\n> \n> But parse_signature() would never find such a buffer! It only looks for\n> signature lines (like \"-----BEGIN PGP\") at the start of each line,\n> without any header keyword. So this code will never find anything except\n> the usual in-body signature.\n\nOkay. So in other words the intent was to parse in-header signatures,\nbut the code failed to do so correctly and thus this never worked in the\nfirst place?\n\nIn any case, `parse_signature()` is only a glorified wrapper around\n`parse_signed_buffer()` in the first place, so in the end they would\nboth parse the buffer in the same way.\n\nNice cleanup, even though it leaves one wondering why the in-header\nsignatures have only been wired up partially.\n\nPatrick\n"},{"id":"502538","messageId":"20240910062642.GA1459778@coredump.intra.peff.net","threadId":"62089","inReplyTo":"Zt_ikkhuMrqVcFOx@pks.im","subject":"Re: [PATCH 2/9] ref-filter: avoid extra copies of payload/signature","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-09-10T06:26:42Z","receivedAt":"2024-09-10T06:26:53Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Sep 10, 2024 at 08:09:22AM +0200, Patrick Steinhardt wrote:\n\n> > But parse_signature() would never find such a buffer! It only looks for\n> > signature lines (like \"-----BEGIN PGP\") at the start of each line,\n> > without any header keyword. So this code will never find anything except\n> > the usual in-body signature.\n> \n> Okay. So in other words the intent was to parse in-header signatures,\n> but the code failed to do so correctly and thus this never worked in the\n> first place?\n> \n> In any case, `parse_signature()` is only a glorified wrapper around\n> `parse_signed_buffer()` in the first place, so in the end they would\n> both parse the buffer in the same way.\n> \n> Nice cleanup, even though it leaves one wondering why the in-header\n> signatures have only been wired up partially.\n\nI, too, was confused. See this exchange with brian:\n\n  https://lore.kernel.org/git/20240908233636.GA4026999@coredump.intra.peff.net/\n\n-Peff\n"},{"id":"502539","messageId":"20240910062848.GB1459778@coredump.intra.peff.net","threadId":"62089","inReplyTo":"Zt_id3Fm6IXMhODF@pks.im","subject":"Re: [PATCH 3/9] ref-filter: strip signature when parsing tag trailers","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-09-10T06:28:48Z","receivedAt":"2024-09-10T06:28:50Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Sep 10, 2024 at 08:08:55AM +0200, Patrick Steinhardt wrote:\n\n> > diff --git a/ref-filter.c b/ref-filter.c\n> > index 0f5513ba7e..e39f505a81 100644\n> > --- a/ref-filter.c\n> > +++ b/ref-filter.c\n> > @@ -2008,9 +2008,17 @@ static void grab_sub_body_contents(struct atom_value *val, int deref, struct exp\n> >  \t\t\tv->s = strbuf_detach(&s, NULL);\n> >  \t\t} else if (atom->u.contents.option == C_TRAILERS) {\n> >  \t\t\tstruct strbuf s = STRBUF_INIT;\n> > +\t\t\tconst char *msg;\n> > +\t\t\tchar *to_free = NULL;\n> > +\n> > +\t\t\tif (siglen)\n> > +\t\t\t\tmsg = to_free = xmemdupz(subpos, sigpos - subpos);\n> > +\t\t\telse\n> > +\t\t\t\tmsg = subpos;\n> >  \n> >  \t\t\t/* Format the trailer info according to the trailer_opts given */\n> > -\t\t\tformat_trailers_from_commit(&atom->u.contents.trailer_opts, subpos, &s);\n> > +\t\t\tformat_trailers_from_commit(&atom->u.contents.trailer_opts, msg, &s);\n> > +\t\t\tfree(to_free);\n> >  \n> >  \t\t\tv->s = strbuf_detach(&s, NULL);\n> >  \t\t} else if (atom->u.contents.option == C_BARE)\n> \n> I've been surprised that we use `subpos` as the starting point here,\n> which includes the whole message including its subject. I would have\n> thought that it was sufficient to only pass the message body as input,\n> which saves allocating some bytes. At least `trailer_info_get()` does\n> not seem to care about the subject at all.\n> \n> In any case this would be a micro-optimization anyway, it just left me\n> scratching my head for a second or two.\n\nYeah, I suspect it would be fine to start at \"bodypos\" instead (and then\nyou could just use \"nonsiglen\" directly as the length). But there may be\ncorner cases for instances where there's no subject/body at all (though\nhaving _just_ trailers feels weird).\n\nAt any rate, I was just following the existing code, which passed in the\nwhole contents starting from subpos, to avoid any unexpected changes.\n\n-Peff\n"},{"id":"502540","messageId":"20240910063332.GC1459778@coredump.intra.peff.net","threadId":"62089","inReplyTo":"Zt_ifxQq-6wNaQ3J@pks.im","subject":"Re: [PATCH 6/9] ref-filter: fix leak of %(trailers) \"argbuf\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-09-10T06:33:32Z","receivedAt":"2024-09-10T06:33:33Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Sep 10, 2024 at 08:09:03AM +0200, Patrick Steinhardt wrote:\n\n> > But I think a less-subtle solution, and what this patch does, is to\n> > switch to a duplicating string_list. That makes it self-contained, and\n> > lets us free argbuf immediately. It may involve a few extra allocations,\n> > but this parsing is something that happens once per program, not once\n> > per output ref.\n> \n> Sensible. I found that in many cases, the `nodup` variants of string\n> lists bring more pain than real benefit.\n\nI have found that, too. :) I've argued in the past for getting rid of it\n(and especially not propagating its world-view to other data structures\nlike strmap, and so on). But Elijah presented some pretty compelling\nnumbers that it does help for some of the large merge-ort strmaps.\n\nSo I've softened my argument to asking people to use it judiciously. ;)\n\n> > This clears up one case that LSan finds in t6300, but there are more.\n> \n> Yeah, there are a bunch of memory leaks around atom parsing in general\n> exposed by t6300. Thanks for plugging some of them!\n\nWell, you're really going to like the rest of the series, then!\n\n-Peff\n"},{"id":"502541","messageId":"20240910063715.GD1459778@coredump.intra.peff.net","threadId":"62089","inReplyTo":"Zt_ijEqdsylYYkNn@pks.im","subject":"Re: [PATCH 9/9] ref-filter: add ref_format_clear() function","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-09-10T06:37:15Z","receivedAt":"2024-09-10T06:37:17Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Sep 10, 2024 at 08:09:16AM +0200, Patrick Steinhardt wrote:\n\n> On Mon, Sep 09, 2024 at 07:21:18PM -0400, Jeff King wrote:\n> > diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh\n> > index e8db612f95..b3163629c5 100755\n> > --- a/t/t6300-for-each-ref.sh\n> > +++ b/t/t6300-for-each-ref.sh\n> > @@ -5,6 +5,7 @@\n> >  \n> >  test_description='for-each-ref test'\n> >  \n> > +TEST_PASSES_SANITIZE_LEAK=true\n> >  . ./test-lib.sh\n> >  GNUPGHOME_NOT_USED=$GNUPGHOME\n> >  . \"$TEST_DIRECTORY\"/lib-gpg.sh\n> \n> Nice! There's also t6302, which has been failing due to all the memory\n> leaks in our atom handling, as well. After your series there's a single\n> memory leak left to make it pass. So we may want to add below patch on\n> top as a low-hanging fruit.\n\nI was afraid to go looking for other almost-there scripts, knowing what\na rabbit hole it can turn into (which I know you are also familiar\nwith).\n\n> -- >8 --\n> diff --git a/ref-filter.c b/ref-filter.c\n> index ce1bcfad857..b06e18a569a 100644\n> --- a/ref-filter.c\n> +++ b/ref-filter.c\n\nThis looks plausibly correct to me, but I'm not at all familiar with the\nconditional placeholders. I think it would make more sense for you to\nwrap it up with a commit message.\n\n-Peff\n"},{"id":"502546","messageId":"4faf815b780218769520561ecf3abca384a2ee6c.1725951400.git.ps@pks.im","threadId":"62089","inReplyTo":"20240909230758.GA921697@coredump.intra.peff.net","subject":"[PATCH 10/9] ref-filter: fix leak with unterminated %(if) atoms","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-10T06:57:15Z","receivedAt":"2024-09-10T06:57:20Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"When parsing `%(if)` atoms we expect a few other atoms to exist to\ncomplete it, like `%(then)` and `%(end)`. Whether or not we have seen\nthese other atoms is tracked in an allocated `if_then_else` structure,\nwhich gets free'd by the `if_then_else_handler()` once we have parsed\nthe complete conditional expression.\n\nThis results in a memory leak when the `%(if)` atom is not terminated\ncorrectly and thus incomplete. We never end up executing its handler and\nthus don't end up freeing the structure.\n\nPlug this memory leak by introducing a new `at_end_data_free` callback\nfunction. If set, we'll execute it in `pop_stack_element()` and pass it\nthe `at_end_data` variable with the intent to free its state. Wire it up\nfor the `%(if)` atom accordingly.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n ref-filter.c                   | 8 +++++---\n t/t6302-for-each-ref-filter.sh | 1 +\n 2 files changed, 6 insertions(+), 3 deletions(-)\n\ndiff --git a/ref-filter.c b/ref-filter.c\nindex ce1bcfad857..b06e18a569a 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -1001,6 +1001,7 @@ struct ref_formatting_stack {\n \tstruct ref_formatting_stack *prev;\n \tstruct strbuf output;\n \tvoid (*at_end)(struct ref_formatting_stack **stack);\n+\tvoid (*at_end_data_free)(void *data);\n \tvoid *at_end_data;\n };\n \n@@ -1169,6 +1170,8 @@ static void pop_stack_element(struct ref_formatting_stack **stack)\n \tif (prev)\n \t\tstrbuf_addbuf(&prev->output, &current->output);\n \tstrbuf_release(&current->output);\n+\tif (current->at_end_data_free)\n+\t\tcurrent->at_end_data_free(current->at_end_data);\n \tfree(current);\n \t*stack = prev;\n }\n@@ -1228,15 +1231,13 @@ static void if_then_else_handler(struct ref_formatting_stack **stack)\n \t}\n \n \t*stack = cur;\n-\tfree(if_then_else);\n }\n \n static int if_atom_handler(struct atom_value *atomv, struct ref_formatting_state *state,\n \t\t\t   struct strbuf *err UNUSED)\n {\n \tstruct ref_formatting_stack *new_stack;\n-\tstruct if_then_else *if_then_else = xcalloc(1,\n-\t\t\t\t\t\t    sizeof(struct if_then_else));\n+\tstruct if_then_else *if_then_else = xcalloc(1, sizeof(*if_then_else));\n \n \tif_then_else->str = atomv->atom->u.if_then_else.str;\n \tif_then_else->cmp_status = atomv->atom->u.if_then_else.cmp_status;\n@@ -1245,6 +1246,7 @@ static int if_atom_handler(struct atom_value *atomv, struct ref_formatting_state\n \tnew_stack = state->stack;\n \tnew_stack->at_end = if_then_else_handler;\n \tnew_stack->at_end_data = if_then_else;\n+\tnew_stack->at_end_data_free = free;\n \treturn 0;\n }\n \ndiff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh\nindex 163c378cfd1..7f44d3c3f22 100755\n--- a/t/t6302-for-each-ref-filter.sh\n+++ b/t/t6302-for-each-ref-filter.sh\n@@ -2,6 +2,7 @@\n \n test_description='test for-each-refs usage of ref-filter APIs'\n \n+TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n . \"$TEST_DIRECTORY\"/lib-gpg.sh\n \n-- \n2.46.0.519.g2e7b89e038.dirty\n\n"},{"id":"502547","messageId":"20240910071222.GA1466029@coredump.intra.peff.net","threadId":"62089","inReplyTo":"4faf815b780218769520561ecf3abca384a2ee6c.1725951400.git.ps@pks.im","subject":"Re: [PATCH 10/9] ref-filter: fix leak with unterminated %(if) atoms","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-09-10T07:12:22Z","receivedAt":"2024-09-10T07:12:23Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Sep 10, 2024 at 08:57:15AM +0200, Patrick Steinhardt wrote:\n\n> When parsing `%(if)` atoms we expect a few other atoms to exist to\n> complete it, like `%(then)` and `%(end)`. Whether or not we have seen\n> these other atoms is tracked in an allocated `if_then_else` structure,\n> which gets free'd by the `if_then_else_handler()` once we have parsed\n> the complete conditional expression.\n> \n> This results in a memory leak when the `%(if)` atom is not terminated\n> correctly and thus incomplete. We never end up executing its handler and\n> thus don't end up freeing the structure.\n> \n> Plug this memory leak by introducing a new `at_end_data_free` callback\n> function. If set, we'll execute it in `pop_stack_element()` and pass it\n> the `at_end_data` variable with the intent to free its state. Wire it up\n> for the `%(if)` atom accordingly.\n\nAh, thanks for explaining. The patch makes much more sense now. :)\n\nIn particular, this:\n\n> @@ -1169,6 +1170,8 @@ static void pop_stack_element(struct ref_formatting_stack **stack)\n>  \tif (prev)\n>  \t\tstrbuf_addbuf(&prev->output, &current->output);\n>  \tstrbuf_release(&current->output);\n> +\tif (current->at_end_data_free)\n> +\t\tcurrent->at_end_data_free(current->at_end_data);\n>  \tfree(current);\n>  \t*stack = prev;\n>  }\n\nwhich frees on pop, replaces the manual:\n\n> @@ -1228,15 +1231,13 @@ static void if_then_else_handler(struct ref_formatting_stack **stack)\n>  \t}\n>  \n>  \t*stack = cur;\n> -\tfree(if_then_else);\n>  }\n\nfree that was happening in the success case.\n\nI think putting this on top of my series makes sense.\n\n-Peff\n"},{"id":"502576","messageId":"xmqqseu7jvz3.fsf@gitster.g","threadId":"62089","inReplyTo":"4faf815b780218769520561ecf3abca384a2ee6c.1725951400.git.ps@pks.im","subject":"Re: [PATCH 10/9] ref-filter: fix leak with unterminated %(if) atoms","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-09-10T16:48:32Z","receivedAt":"2024-09-10T16:48:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt <ps@pks.im> writes:\n\n> When parsing `%(if)` atoms we expect a few other atoms to exist to\n> complete it, like `%(then)` and `%(end)`. Whether or not we have seen\n> these other atoms is tracked in an allocated `if_then_else` structure,\n> which gets free'd by the `if_then_else_handler()` once we have parsed\n> the complete conditional expression.\n>\n> This results in a memory leak when the `%(if)` atom is not terminated\n> correctly and thus incomplete. We never end up executing its handler and\n> thus don't end up freeing the structure.\n>\n> Plug this memory leak by introducing a new `at_end_data_free` callback\n> function. If set, we'll execute it in `pop_stack_element()` and pass it\n> the `at_end_data` variable with the intent to free its state. Wire it up\n> for the `%(if)` atom accordingly.\n\nSounds good.  We diagnose unclosed \"%(if)\", report mismatch, and\ndie() soon, so plugging this may more about \"let's silence leak\nchecker so that it can be more effective to help us find real leaks\nthat matter\", not \"this is leaking proportionally to the size of the\nuser data, and must be plugged\".\n\nI see this code snippet (not touched by your patch):\n\n\tif (state.stack->prev) {\n\t\tpop_stack_element(&state.stack);\n\t\treturn strbuf_addf_ret(error_buf, -1, _(\"format: %%(end) atom missing\"));\n\t}\n\nand wonder how this handles the case where state.stack->prev->prev\nis also not NULL.  Shouldn't it be looping while .prev is not NULL?\n\ne.g.\n\ndiff --git c/ref-filter.c w/ref-filter.c\nindex b06e18a569..d2040f5047 100644\n--- c/ref-filter.c\n+++ w/ref-filter.c\n@@ -3471,7 +3471,8 @@ int format_ref_array_item(struct ref_array_item *info,\n \t\t}\n \t}\n \tif (state.stack->prev) {\n-\t\tpop_stack_element(&state.stack);\n+\t\twhile (state.stack->prev)\n+\t\t\tpop_stack_element(&state.stack);\n \t\treturn strbuf_addf_ret(error_buf, -1, _(\"format: %%(end) atom missing\"));\n \t}\n \tstrbuf_addbuf(final_buf, &state.stack->output);\n\n"},{"id":"502667","messageId":"ZuLA0SBqhBbBdcd1@pks.im","threadId":"62089","inReplyTo":"xmqqseu7jvz3.fsf@gitster.g","subject":"Re: [PATCH 10/9] ref-filter: fix leak with unterminated %(if) atoms","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-12T10:22:16Z","receivedAt":"2024-09-12T10:22:20Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, Sep 10, 2024 at 09:48:32AM -0700, Junio C Hamano wrote:\n> Patrick Steinhardt <ps@pks.im> writes:\n> \n> > When parsing `%(if)` atoms we expect a few other atoms to exist to\n> > complete it, like `%(then)` and `%(end)`. Whether or not we have seen\n> > these other atoms is tracked in an allocated `if_then_else` structure,\n> > which gets free'd by the `if_then_else_handler()` once we have parsed\n> > the complete conditional expression.\n> >\n> > This results in a memory leak when the `%(if)` atom is not terminated\n> > correctly and thus incomplete. We never end up executing its handler and\n> > thus don't end up freeing the structure.\n> >\n> > Plug this memory leak by introducing a new `at_end_data_free` callback\n> > function. If set, we'll execute it in `pop_stack_element()` and pass it\n> > the `at_end_data` variable with the intent to free its state. Wire it up\n> > for the `%(if)` atom accordingly.\n> \n> Sounds good.  We diagnose unclosed \"%(if)\", report mismatch, and\n> die() soon, so plugging this may more about \"let's silence leak\n> checker so that it can be more effective to help us find real leaks\n> that matter\", not \"this is leaking proportionally to the size of the\n> user data, and must be plugged\".\n> \n> I see this code snippet (not touched by your patch):\n> \n> \tif (state.stack->prev) {\n> \t\tpop_stack_element(&state.stack);\n> \t\treturn strbuf_addf_ret(error_buf, -1, _(\"format: %%(end) atom missing\"));\n> \t}\n> \n> and wonder how this handles the case where state.stack->prev->prev\n> is also not NULL.  Shouldn't it be looping while .prev is not NULL?\n> \n> e.g.\n> \n> diff --git c/ref-filter.c w/ref-filter.c\n> index b06e18a569..d2040f5047 100644\n> --- c/ref-filter.c\n> +++ w/ref-filter.c\n> @@ -3471,7 +3471,8 @@ int format_ref_array_item(struct ref_array_item *info,\n>  \t\t}\n>  \t}\n>  \tif (state.stack->prev) {\n> -\t\tpop_stack_element(&state.stack);\n> +\t\twhile (state.stack->prev)\n> +\t\t\tpop_stack_element(&state.stack);\n>  \t\treturn strbuf_addf_ret(error_buf, -1, _(\"format: %%(end) atom missing\"));\n>  \t}\n>  \tstrbuf_addbuf(final_buf, &state.stack->output);\n\nHm. It certainly feels like we should do that. I couldn't construct a\ntest case that fails with the leak sanitizer though. If it's a leak I'm\nsure I'll eventually hit it when I continue down the road headed towards\nleak-free-ness.\n\nPatrick\n"},{"id":"502682","messageId":"20240912111858.GA617985@coredump.intra.peff.net","threadId":"62089","inReplyTo":"ZuLA0SBqhBbBdcd1@pks.im","subject":"Re: [PATCH 10/9] ref-filter: fix leak with unterminated %(if) atoms","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-09-12T11:18:58Z","receivedAt":"2024-09-12T11:18:59Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Sep 12, 2024 at 12:22:16PM +0200, Patrick Steinhardt wrote:\n\n> > diff --git c/ref-filter.c w/ref-filter.c\n> > index b06e18a569..d2040f5047 100644\n> > --- c/ref-filter.c\n> > +++ w/ref-filter.c\n> > @@ -3471,7 +3471,8 @@ int format_ref_array_item(struct ref_array_item *info,\n> >  \t\t}\n> >  \t}\n> >  \tif (state.stack->prev) {\n> > -\t\tpop_stack_element(&state.stack);\n> > +\t\twhile (state.stack->prev)\n> > +\t\t\tpop_stack_element(&state.stack);\n> >  \t\treturn strbuf_addf_ret(error_buf, -1, _(\"format: %%(end) atom missing\"));\n> >  \t}\n> >  \tstrbuf_addbuf(final_buf, &state.stack->output);\n> \n> Hm. It certainly feels like we should do that. I couldn't construct a\n> test case that fails with the leak sanitizer though. If it's a leak I'm\n> sure I'll eventually hit it when I continue down the road headed towards\n> leak-free-ness.\n\nHmm. I think just:\n\n  ./git for-each-ref --format='%(if)%(then)%(if)%(then)%(if)%(then)'\n\nshould trigger it, and running it in the debugger I can see that we exit\nthe function with multiple entries.\n\nValgrind claims the memory is still reachable, but I don't see how. The\n\"state\" variable is accessible only inside that function. The only thing\nwe do after returning is die(). I wonder if it is a false negative\nbecause the stack is left undisturbed (especially because the compiler\nknows that die() does not return).\n\nAt any rate, I think the same would apply to the earlier error returns:\n\ndiff --git a/ref-filter.c b/ref-filter.c\nindex b06e18a569..a339f0ab0f 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -3454,7 +3454,8 @@ int format_ref_array_item(struct ref_array_item *info,\n \t\tpos = parse_ref_filter_atom(format, sp + 2, ep, error_buf);\n \t\tif (pos < 0 || get_ref_atom_value(info, pos, &atomv, error_buf) ||\n \t\t    atomv->handler(atomv, &state, error_buf)) {\n-\t\t\tpop_stack_element(&state.stack);\n+\t\t\twhile (state.stack->prev)\n+\t\t\t\tpop_stack_element(&state.stack);\n \t\t\treturn -1;\n \t\t}\n \t}\n@@ -3466,7 +3467,8 @@ int format_ref_array_item(struct ref_array_item *info,\n \t\tstruct atom_value resetv = ATOM_VALUE_INIT;\n \t\tresetv.s = GIT_COLOR_RESET;\n \t\tif (append_atom(&resetv, &state, error_buf)) {\n-\t\t\tpop_stack_element(&state.stack);\n+\t\t\twhile (state.stack->prev)\n+\t\t\t\tpop_stack_element(&state.stack);\n \t\t\treturn -1;\n \t\t}\n \t}\n\nI wasn't sure why the non-error code path wouldn't need the same, but it\nlooks like there's some popping that happens in the various callbacks?\nI'm not very familiar with this code, and it's hard to follow the flow\nthrough the function pointers.\n\nAll that said, I am content to leave it for now. Even if it's a real\nleak, it's one that happens once per program right before exiting with\nan error. Most of the value in cleaning up trivial leaks like that are\nto reduce the noise from analyzers so that we can find the much more\nimportant leaks that scale with the input. If the analyzers aren't\ncomplaining and we think it's trivial, it may not be worth spending a\nlot of time on.\n\n-Peff\n"},{"id":"502706","messageId":"ZuLRRGELzNl5VH_R@pks.im","threadId":"62089","inReplyTo":"20240912111858.GA617985@coredump.intra.peff.net","subject":"Re: [PATCH 10/9] ref-filter: fix leak with unterminated %(if) atoms","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-09-12T11:32:25Z","receivedAt":"2024-09-12T11:32:28Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Thu, Sep 12, 2024 at 07:18:58AM -0400, Jeff King wrote:\n> All that said, I am content to leave it for now. Even if it's a real\n> leak, it's one that happens once per program right before exiting with\n> an error. Most of the value in cleaning up trivial leaks like that are\n> to reduce the noise from analyzers so that we can find the much more\n> important leaks that scale with the input. If the analyzers aren't\n> complaining and we think it's trivial, it may not be worth spending a\n> lot of time on.\n\nAgreed. Thanks for digging!\n\nPatrick\n"},{"id":"502713","messageId":"xmqqa5gc4o42.fsf@gitster.g","threadId":"62089","inReplyTo":"20240912111858.GA617985@coredump.intra.peff.net","subject":"Re: [PATCH 10/9] ref-filter: fix leak with unterminated %(if) atoms","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-09-12T20:24:13Z","receivedAt":"2024-09-12T20:24:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Thu, Sep 12, 2024 at 12:22:16PM +0200, Patrick Steinhardt wrote:\n>\n>> > diff --git c/ref-filter.c w/ref-filter.c\n>> > index b06e18a569..d2040f5047 100644\n>> > --- c/ref-filter.c\n>> > +++ w/ref-filter.c\n>> > @@ -3471,7 +3471,8 @@ int format_ref_array_item(struct ref_array_item *info,\n>> >  \t\t}\n>> >  \t}\n>> >  \tif (state.stack->prev) {\n>> > -\t\tpop_stack_element(&state.stack);\n>> > +\t\twhile (state.stack->prev)\n>> > +\t\t\tpop_stack_element(&state.stack);\n>> >  \t\treturn strbuf_addf_ret(error_buf, -1, _(\"format: %%(end) atom missing\"));\n>> >  \t}\n>> >  \tstrbuf_addbuf(final_buf, &state.stack->output);\n>> \n>> Hm. It certainly feels like we should do that. I couldn't construct a\n>> test case that fails with the leak sanitizer though. If it's a leak I'm\n>> sure I'll eventually hit it when I continue down the road headed towards\n>> leak-free-ness.\n>\n> Hmm. I think just:\n>\n>   ./git for-each-ref --format='%(if)%(then)%(if)%(then)%(if)%(then)'\n>\n> should trigger it, and running it in the debugger I can see that we exit\n> the function with multiple entries.\n>\n> Valgrind claims the memory is still reachable, but I don't see how. The\n> \"state\" variable is accessible only inside that function. The only thing\n> we do after returning is die(). I wonder if it is a false negative\n> because the stack is left undisturbed (especially because the compiler\n> knows that die() does not return).\n\nYup, the reason why I didn't add any test was because the leak\nchecker failed to notice the apparent leak.\n\n> At any rate, I think the same would apply to the earlier error returns:\n> ...\n> All that said, I am content to leave it for now. Even if it's a real\n> leak, it's one that happens once per program right before exiting with\n> an error. Most of the value in cleaning up trivial leaks like that are\n> to reduce the noise from analyzers so that we can find the much more\n> important leaks that scale with the input. If the analyzers aren't\n> complaining and we think it's trivial, it may not be worth spending a\n> lot of time on.\n\nThat is good to me, too.\n"}]}