{"thread":{"id":"47400","subject":"[PATCH] fmt-merge-msg: avoid leaking strbuf in shortlog()","startedAt":"2017-12-07T20:23:09Z","lastAt":"2017-12-20T13:05:12Z","messageCount":16,"participants":["René Scharfe","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"334361","messageId":"b2238da3-9eba-1521-f4ca-3b805f103555@web.de","threadId":"47400","inReplyTo":null,"subject":"[PATCH] fmt-merge-msg: avoid leaking strbuf in shortlog()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2017-12-07T20:22:49Z","receivedAt":"2017-12-07T20:23:09Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Use string_list_append_nodup() instead of string_list_append() to hand\nover ownership of a detached strbuf and thus avoid leaking its memory.\n\nSigned-off-by: Rene Scharfe \n---\n builtin/fmt-merge-msg.c | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/fmt-merge-msg.c b/builtin/fmt-merge-msg.c\nindex 22034f87e7..8e8a15ea4a 100644\n--- a/builtin/fmt-merge-msg.c\n+++ b/builtin/fmt-merge-msg.c\n@@ -377,7 +377,8 @@ static void shortlog(const char *name,\n \t\t\tstring_list_append(&subjects,\n \t\t\t\t\t oid_to_hex(&commit->object.oid));\n \t\telse\n-\t\t\tstring_list_append(&subjects, strbuf_detach(&sb, NULL));\n+\t\t\tstring_list_append_nodup(&subjects,\n+\t\t\t\t\t\t strbuf_detach(&sb, NULL));\n \t}\n \n \tif (opts->credit_people)\n-- \n2.15.1\n"},{"id":"334376","messageId":"20171207212735.GD12850@sigill.intra.peff.net","threadId":"47400","inReplyTo":"b2238da3-9eba-1521-f4ca-3b805f103555@web.de","subject":"Re: [PATCH] fmt-merge-msg: avoid leaking strbuf in shortlog()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-12-07T21:27:36Z","receivedAt":"2017-12-07T21:27:51Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Dec 07, 2017 at 09:22:49PM +0100, René Scharfe wrote:\n\n> Use string_list_append_nodup() instead of string_list_append() to hand\n> over ownership of a detached strbuf and thus avoid leaking its memory.\n\nLooks obviously correct (though one thing missing from the diff context\nis whether \"subjects\" is set to DUP -- it is, which is good).\n\nGrepping for \"list_append.*detach\" shows a few other possible cases in\ntransport-helper.c, which I think are leaks.\n\nI wondered if it would be possible to write a coccinelle rule for this,\nbut I think it's not possible. Whether this is right depends on the\nstrdup_strings flag, which could change at runtime (though in practice\nit doesn't).\n\n-Peff\n"},{"id":"334380","messageId":"xmqq4lp2cisd.fsf@gitster.mtv.corp.google.com","threadId":"47400","inReplyTo":"b2238da3-9eba-1521-f4ca-3b805f103555@web.de","subject":"Re: [PATCH] fmt-merge-msg: avoid leaking strbuf in shortlog()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-12-07T21:47:14Z","receivedAt":"2017-12-07T21:47:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe writes:\n\n> Use string_list_append_nodup() instead of string_list_append() to hand\n> over ownership of a detached strbuf and thus avoid leaking its memory.\n>\n> Signed-off-by: Rene Scharfe \n> ---\n> builtin/fmt-merge-msg.c | 3 ++-\n> 1 file changed, 2 insertions(+), 1 deletion(-)\n>\n> diff --git a/builtin/fmt-merge-msg.c b/builtin/fmt-merge-msg.c\n> index 22034f87e7..8e8a15ea4a 100644\n> --- a/builtin/fmt-merge-msg.c\n> +++ b/builtin/fmt-merge-msg.c\n> @@ -377,7 +377,8 @@ static void shortlog(const char *name,\n> \t\t\tstring_list_append(&subjects,\n> \t\t\t\t\t oid_to_hex(&commit->object.oid));\n> \t\telse\n> -\t\t\tstring_list_append(&subjects, strbuf_detach(&sb, NULL));\n> +\t\t\tstring_list_append_nodup(&subjects,\n> +\t\t\t\t\t\t strbuf_detach(&sb, NULL));\n> \t}\n> \n> \tif (opts->credit_people)\n\nWhat is leaked comes from strbuf, so the title is not a lie, but I\ntend to think that this leak is caused by a somewhat strange\nstring_list API. The subjects string-list is initialized as a \"dup\"\nkind, but a caller that wants to avoid leaking can (and should) use\n_nodup() call to add a string without duping. It all feels a bit\ntoo convoluted.\n\nThe patch looks good.\n"},{"id":"334407","messageId":"20171208101455.GC1899@sigill.intra.peff.net","threadId":"47400","inReplyTo":"xmqq4lp2cisd.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] fmt-merge-msg: avoid leaking strbuf in shortlog()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-12-08T10:14:56Z","receivedAt":"2017-12-08T10:15:10Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Dec 07, 2017 at 01:47:14PM -0800, Junio C Hamano wrote:\n\n> > diff --git a/builtin/fmt-merge-msg.c b/builtin/fmt-merge-msg.c\n> > index 22034f87e7..8e8a15ea4a 100644\n> > --- a/builtin/fmt-merge-msg.c\n> > +++ b/builtin/fmt-merge-msg.c\n> > @@ -377,7 +377,8 @@ static void shortlog(const char *name,\n> > \t\t\tstring_list_append(&subjects,\n> > \t\t\t\t\t oid_to_hex(&commit->object.oid));\n> > \t\telse\n> > -\t\t\tstring_list_append(&subjects, strbuf_detach(&sb, NULL));\n> > +\t\t\tstring_list_append_nodup(&subjects,\n> > +\t\t\t\t\t\t strbuf_detach(&sb, NULL));\n> > \t}\n> > \n> > \tif (opts->credit_people)\n> \n> What is leaked comes from strbuf, so the title is not a lie, but I\n> tend to think that this leak is caused by a somewhat strange\n> string_list API. The subjects string-list is initialized as a \"dup\"\n> kind, but a caller that wants to avoid leaking can (and should) use\n> _nodup() call to add a string without duping. It all feels a bit\n> too convoluted.\n\nI'm not sure it's string-list's fault. Many callers (including this one)\nhave _some_ entries whose strings must be duplicated and others which do\nnot.\n\nSo either:\n\n 1. The list gets marked as \"nodup\", and we add an extra xstrdup() to the\n oid_to_hex call above. And also need to remember to free() the\n strings later, since the list does not own them.\n\nor\n\n 2. We mark it as \"dup\" and incur an extra allocation and copy, like:\n\n string_list_append(&subjects, sb.buf);\n strbuf_release(&buf);\n\nSo I'd really blame the caller, which doesn't want to do (2) out of a\nsense of optimization. It could also perhaps write it as:\n\n while (commit = get_revision(rev)) {\n\tstrbuf_reset(&sb);\n\t... maybe put some stuff in sb ...\n\tif (!sb.len)\n\t\tstring_list_append(&subjects, oid_to_hex(obj));\n\telse\n\t\tstring_list_append(&subjects, sb.buf);\n }\n strbuf_release(&sb);\n\nwhich at least avoids the extra allocations.\n\nBy the way, I think there's another quite subtle leak in this function.\nWe do this:\n\n format_commit_message(commit, \"%s\", &sb, &ctx);\n strbuf_ltrim(&sb);\n\nand then only use \"sb\" if sb.len is non-zero. But we may have actually\nallocated to create our zero-length string (e.g., if we had a strbuf\nfull of spaces and trimmed them all off). Since we reuse \"sb\" over and\nover as we loop, this will actually only leak once for the whole loop,\nnot once per iteration. So it's probably not a big deal, but writing it\nwith the explicit reset/release pattern fixes that (and is more\nidiomatic for our code base, I think).\n\n-Peff\n"},{"id":"334461","messageId":"e1daa2b6-004b-ad66-b84d-84fa70a44baf@web.de","threadId":"47400","inReplyTo":"20171207212735.GD12850@sigill.intra.peff.net","subject":"Re: [PATCH] fmt-merge-msg: avoid leaking strbuf in shortlog()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2017-12-08T17:29:31Z","receivedAt":"2017-12-08T17:29:45Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 07.12.2017 um 22:27 schrieb Jeff King:\n> Grepping for \"list_append.*detach\" shows a few other possible cases in\n> transport-helper.c, which I think are leaks.\n\n-- >8 --\nSubject: [PATCH] transport-helper: plug strbuf and string_list leaks\n\nTransfer ownership of detached strbufs to string_lists of the\nduplicating variety by calling string_list_append_nodup() instead of\nstring_list_append() to avoid duplicating and then leaking the buffer.\n\nWhile at it make sure to release the string_list when done;\npush_refs_with_export() already does that.\n\nReported-by: Jeff King \nSigned-off-by: Rene Scharfe \n---\n transport-helper.c | 7 +++++--\n 1 file changed, 5 insertions(+), 2 deletions(-)\n\ndiff --git a/transport-helper.c b/transport-helper.c\nindex bf05a2dcf1..f682e7c534 100644\n--- a/transport-helper.c\n+++ b/transport-helper.c\n@@ -882,7 +882,8 @@ static int push_refs_with_push(struct transport *transport,\n \t\t\tstruct strbuf cas = STRBUF_INIT;\n \t\t\tstrbuf_addf(&cas, \"%s:%s\",\n \t\t\t\t ref->name, oid_to_hex(&ref->old_oid_expect));\n-\t\t\tstring_list_append(&cas_options, strbuf_detach(&cas, NULL));\n+\t\t\tstring_list_append_nodup(&cas_options,\n+\t\t\t\t\t\t strbuf_detach(&cas, NULL));\n \t\t}\n \t}\n \tif (buf.len == 0) {\n@@ -897,6 +898,7 @@ static int push_refs_with_push(struct transport *transport,\n \tstrbuf_addch(&buf, '\\n');\n \tsendline(data, &buf);\n \tstrbuf_release(&buf);\n+\tstring_list_release(&cas_options, 0);\n \n \treturn push_update_refs_status(data, remote_refs, flags);\n }\n@@ -930,7 +932,8 @@ static int push_refs_with_export(struct transport *transport,\n \t\tprivate = apply_refspecs(data->refspecs, data->refspec_nr, ref->name);\n \t\tif (private && !get_oid(private, &oid)) {\n \t\t\tstrbuf_addf(&buf, \"^%s\", private);\n-\t\t\tstring_list_append(&revlist_args, strbuf_detach(&buf, NULL));\n+\t\t\tstring_list_append_nodup(&revlist_args,\n+\t\t\t\t\t\t strbuf_detach(&buf, NULL));\n \t\t\toidcpy(&ref->old_oid, &oid);\n \t\t}\n \t\tfree(private);\n-- \n2.15.1\n"},{"id":"334462","messageId":"1654a696-73d5-c9ef-0fc2-bd82aaf2cabb@web.de","threadId":"47400","inReplyTo":"20171208101455.GC1899@sigill.intra.peff.net","subject":"Re: [PATCH] fmt-merge-msg: avoid leaking strbuf in shortlog()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2017-12-08T17:29:34Z","receivedAt":"2017-12-08T17:29:48Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 08.12.2017 um 11:14 schrieb Jeff King:\n> On Thu, Dec 07, 2017 at 01:47:14PM -0800, Junio C Hamano wrote:\n> \n>>> diff --git a/builtin/fmt-merge-msg.c b/builtin/fmt-merge-msg.c\n>>> index 22034f87e7..8e8a15ea4a 100644\n>>> --- a/builtin/fmt-merge-msg.c\n>>> +++ b/builtin/fmt-merge-msg.c\n>>> @@ -377,7 +377,8 @@ static void shortlog(const char *name,\n>>> \t\t\tstring_list_append(&subjects,\n>>> \t\t\t\t\t oid_to_hex(&commit->object.oid));\n>>> \t\telse\n>>> -\t\t\tstring_list_append(&subjects, strbuf_detach(&sb, NULL));\n>>> +\t\t\tstring_list_append_nodup(&subjects,\n>>> +\t\t\t\t\t\t strbuf_detach(&sb, NULL));\n>>> \t}\n>>> \n>>> \tif (opts->credit_people)\n>>\n>> What is leaked comes from strbuf, so the title is not a lie, but I\n>> tend to think that this leak is caused by a somewhat strange\n>> string_list API. The subjects string-list is initialized as a \"dup\"\n>> kind, but a caller that wants to avoid leaking can (and should) use\n>> _nodup() call to add a string without duping. It all feels a bit\n>> too convoluted.\n> \n> I'm not sure it's string-list's fault. Many callers (including this one)\n> have _some_ entries whose strings must be duplicated and others which do\n> not.\n> \n> So either:\n> \n> 1. The list gets marked as \"nodup\", and we add an extra xstrdup() to the\n> oid_to_hex call above. And also need to remember to free() the\n> strings later, since the list does not own them.\n> \n> or\n> \n> 2. We mark it as \"dup\" and incur an extra allocation and copy, like:\n> \n> string_list_append(&subjects, sb.buf);\n> strbuf_release(&buf);\n\nThe two modes (dup/nodup) make string_list code tricky. Not sure\nhow far we'd get with something simpler (e.g. an array of char pointers),\nbut having the caller do all string allocations would make the code\neasier to analyze.\n\n> So I'd really blame the caller, which doesn't want to do (2) out of a\n> sense of optimization. It could also perhaps write it as:\n> \n> while (commit = get_revision(rev)) {\n> \tstrbuf_reset(&sb);\n> \t... maybe put some stuff in sb ...\n> \tif (!sb.len)\n> \t\tstring_list_append(&subjects, oid_to_hex(obj));\n> \telse\n> \t\tstring_list_append(&subjects, sb.buf);\n> }\n> strbuf_release(&sb);\n> \n> which at least avoids the extra allocations.\n\nRight, we'd just have extra string copies in that case.\n\n> By the way, I think there's another quite subtle leak in this function.\n> We do this:\n> \n> format_commit_message(commit, \"%s\", &sb, &ctx);\n> strbuf_ltrim(&sb);\n> \n> and then only use \"sb\" if sb.len is non-zero. But we may have actually\n> allocated to create our zero-length string (e.g., if we had a strbuf\n> full of spaces and trimmed them all off). Since we reuse \"sb\" over and\n> over as we loop, this will actually only leak once for the whole loop,\n> not once per iteration. So it's probably not a big deal, but writing it\n> with the explicit reset/release pattern fixes that (and is more\n> idiomatic for our code base, I think).\n\nIt's subtle, but I think it's not leaking, at least not in your example\ncase (and I can't think of another way). IIUC format_subject(), which\nhandles the \"%s\" part, doesn't touch sb if the subject is made up only\nof whitespace.\n\nRené\n"},{"id":"334478","messageId":"xmqqd13p83sb.fsf@gitster.mtv.corp.google.com","threadId":"47400","inReplyTo":"1654a696-73d5-c9ef-0fc2-bd82aaf2cabb@web.de","subject":"Re: [PATCH] fmt-merge-msg: avoid leaking strbuf in shortlog()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-12-08T18:37:08Z","receivedAt":"2017-12-08T18:37:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe writes:\n\n>> I'm not sure it's string-list's fault. Many callers (including this one)\n>> ...\n> The two modes (dup/nodup) make string_list code tricky. Not sure\n> how far we'd get with something simpler (e.g. an array of char pointers),\n> but having the caller do all string allocations would make the code\n> easier to analyze.\n\nYes.\n\nIt probably would have been more sensible if the API did not have\ntwo modes (instead, have the caller pass whatever string to be\nstored, *and* make the caller responsible for freeing them *if* it\npassed an allocated string).\n\nFor the push_refs_with_push() patch you sent, another possible fix\nwould be to make cas_options a nodup kind so that the result of\nstrbuf_detach() does not get an extra strdup to be lost when placed\nin cas_options. With the current string-list API that would not\nquite work, because freeing done in _release() is tied to the\n\"dup/nodup\" ness of the string list. I think there even is a\ncodepath that initializes a string_list as nodup kind, stuffs string\nin it giving the ownership, and then flips it into dup kind just\nbefore calling _release() only to have it free the strings, or\nsomething silly/ugly like that.\n\nIn any case, the patch looks sensible. Thanks for plugging the\nleaks.\n"},{"id":"334479","messageId":"xmqq609h83gg.fsf@gitster.mtv.corp.google.com","threadId":"47400","inReplyTo":"e1daa2b6-004b-ad66-b84d-84fa70a44baf@web.de","subject":"Re: [PATCH] fmt-merge-msg: avoid leaking strbuf in shortlog()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-12-08T18:44:15Z","receivedAt":"2017-12-08T18:44:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe writes:\n\n> Am 07.12.2017 um 22:27 schrieb Jeff King:\n>> Grepping for \"list_append.*detach\" shows a few other possible cases in\n>> transport-helper.c, which I think are leaks.\n>\n> -- >8 --\n> Subject: [PATCH] transport-helper: plug strbuf and string_list leaks\n>\n> Transfer ownership of detached strbufs to string_lists of the\n> duplicating variety by calling string_list_append_nodup() instead of\n> string_list_append() to avoid duplicating and then leaking the buffer.\n>\n> While at it make sure to release the string_list when done;\n> push_refs_with_export() already does that.\n>\n> Reported-by: Jeff King \n> Signed-off-by: Rene Scharfe \n> ---\n> transport-helper.c | 7 +++++--\n> 1 file changed, 5 insertions(+), 2 deletions(-)\n>\n> diff --git a/transport-helper.c b/transport-helper.c\n> index bf05a2dcf1..f682e7c534 100644\n> --- a/transport-helper.c\n> +++ b/transport-helper.c\n> @@ -882,7 +882,8 @@ static int push_refs_with_push(struct transport *transport,\n> \t\t\tstruct strbuf cas = STRBUF_INIT;\n> \t\t\tstrbuf_addf(&cas, \"%s:%s\",\n> \t\t\t\t ref->name, oid_to_hex(&ref->old_oid_expect));\n> -\t\t\tstring_list_append(&cas_options, strbuf_detach(&cas, NULL));\n> +\t\t\tstring_list_append_nodup(&cas_options,\n> +\t\t\t\t\t\t strbuf_detach(&cas, NULL));\n> \t\t}\n> \t}\n> \tif (buf.len == 0) {\n> @@ -897,6 +898,7 @@ static int push_refs_with_push(struct transport *transport,\n> \tstrbuf_addch(&buf, '\\n');\n> \tsendline(data, &buf);\n> \tstrbuf_release(&buf);\n> +\tstring_list_release(&cas_options, 0);\n\nThere is no such function; you meant _clear() perhaps?\n"},{"id":"334485","messageId":"20ca4c68-1101-0309-966e-cd0615931f1e@web.de","threadId":"47400","inReplyTo":"xmqq609h83gg.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] fmt-merge-msg: avoid leaking strbuf in shortlog()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2017-12-08T20:10:26Z","receivedAt":"2017-12-08T20:10:40Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 08.12.2017 um 19:44 schrieb Junio C Hamano:\n> René Scharfe writes:\n> \n>> Am 07.12.2017 um 22:27 schrieb Jeff King:\n>>> Grepping for \"list_append.*detach\" shows a few other possible cases in\n>>> transport-helper.c, which I think are leaks.\n>>\n>> -- >8 --\n>> Subject: [PATCH] transport-helper: plug strbuf and string_list leaks\n>>\n>> Transfer ownership of detached strbufs to string_lists of the\n>> duplicating variety by calling string_list_append_nodup() instead of\n>> string_list_append() to avoid duplicating and then leaking the buffer.\n>>\n>> While at it make sure to release the string_list when done;\n>> push_refs_with_export() already does that.\n>>\n>> Reported-by: Jeff King \n>> Signed-off-by: Rene Scharfe \n>> ---\n>> transport-helper.c | 7 +++++--\n>> 1 file changed, 5 insertions(+), 2 deletions(-)\n>>\n>> diff --git a/transport-helper.c b/transport-helper.c\n>> index bf05a2dcf1..f682e7c534 100644\n>> --- a/transport-helper.c\n>> +++ b/transport-helper.c\n>> @@ -882,7 +882,8 @@ static int push_refs_with_push(struct transport *transport,\n>> \t\t\tstruct strbuf cas = STRBUF_INIT;\n>> \t\t\tstrbuf_addf(&cas, \"%s:%s\",\n>> \t\t\t\t ref->name, oid_to_hex(&ref->old_oid_expect));\n>> -\t\t\tstring_list_append(&cas_options, strbuf_detach(&cas, NULL));\n>> +\t\t\tstring_list_append_nodup(&cas_options,\n>> +\t\t\t\t\t\t strbuf_detach(&cas, NULL));\n>> \t\t}\n>> \t}\n>> \tif (buf.len == 0) {\n>> @@ -897,6 +898,7 @@ static int push_refs_with_push(struct transport *transport,\n>> \tstrbuf_addch(&buf, '\\n');\n>> \tsendline(data, &buf);\n>> \tstrbuf_release(&buf);\n>> +\tstring_list_release(&cas_options, 0);\n> \n> There is no such function; you meant _clear() perhaps?\n\nYes, of course, I'm sorry. Not sure what happened there. O_o\n\nRené\n"},{"id":"334500","messageId":"20171208211137.GA7355@sigill.intra.peff.net","threadId":"47400","inReplyTo":"e1daa2b6-004b-ad66-b84d-84fa70a44baf@web.de","subject":"Re: [PATCH] fmt-merge-msg: avoid leaking strbuf in shortlog()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-12-08T21:11:38Z","receivedAt":"2017-12-08T21:11:44Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Dec 08, 2017 at 06:29:31PM +0100, René Scharfe wrote:\n\n> Am 07.12.2017 um 22:27 schrieb Jeff King:\n> > Grepping for \"list_append.*detach\" shows a few other possible cases in\n> > transport-helper.c, which I think are leaks.\n> \n> -- >8 --\n> Subject: [PATCH] transport-helper: plug strbuf and string_list leaks\n> \n> Transfer ownership of detached strbufs to string_lists of the\n> duplicating variety by calling string_list_append_nodup() instead of\n> string_list_append() to avoid duplicating and then leaking the buffer.\n\nThanks, this part looks obviously correct.\n\n> While at it make sure to release the string_list when done;\n> push_refs_with_export() already does that.\n\nThis one takes a bit more digging. I've been bitten before in Git's code\nby freeing what appeared to be a leak only to find out that we had\npassed the pointers off to some other data structure which expected them\nto persist.\n\nHere we feed them to set_helper_option(), which passes them to\nquote_c_style(), which makes a copy into a strbuf. So I think all is\nwell.\n\n-Peff\n"},{"id":"334504","messageId":"20171208211706.GB7355@sigill.intra.peff.net","threadId":"47400","inReplyTo":"1654a696-73d5-c9ef-0fc2-bd82aaf2cabb@web.de","subject":"Re: [PATCH] fmt-merge-msg: avoid leaking strbuf in shortlog()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-12-08T21:17:06Z","receivedAt":"2017-12-08T21:17:14Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Dec 08, 2017 at 06:29:34PM +0100, René Scharfe wrote:\n\n> > By the way, I think there's another quite subtle leak in this function.\n> > We do this:\n> > \n> > format_commit_message(commit, \"%s\", &sb, &ctx);\n> > strbuf_ltrim(&sb);\n> > \n> > and then only use \"sb\" if sb.len is non-zero. But we may have actually\n> > allocated to create our zero-length string (e.g., if we had a strbuf\n> > full of spaces and trimmed them all off). Since we reuse \"sb\" over and\n> > over as we loop, this will actually only leak once for the whole loop,\n> > not once per iteration. So it's probably not a big deal, but writing it\n> > with the explicit reset/release pattern fixes that (and is more\n> > idiomatic for our code base, I think).\n> \n> It's subtle, but I think it's not leaking, at least not in your example\n> case (and I can't think of another way). IIUC format_subject(), which\n> handles the \"%s\" part, doesn't touch sb if the subject is made up only\n> of whitespace.\n\nYeah, I suspected that may be the case. But IMHO it is a poor use of\nstrbufs if you have to dig that far to see whether the code leaks or\nnot. The whole point of strbufs is to make string handling and memory\nownership more obviously correct.\n\nJust skimming the history, I think it's mostly an artifact of the\nfunction was slowly converted over the years.\n\n-Peff\n"},{"id":"334506","messageId":"20171208212832.GC7355@sigill.intra.peff.net","threadId":"47400","inReplyTo":"xmqqd13p83sb.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] fmt-merge-msg: avoid leaking strbuf in shortlog()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-12-08T21:28:33Z","receivedAt":"2017-12-08T21:28:39Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Dec 08, 2017 at 10:37:08AM -0800, Junio C Hamano wrote:\n\n> > The two modes (dup/nodup) make string_list code tricky. Not sure\n> > how far we'd get with something simpler (e.g. an array of char pointers),\n> > but having the caller do all string allocations would make the code\n> > easier to analyze.\n> \n> Yes.\n> \n> It probably would have been more sensible if the API did not have\n> two modes (instead, have the caller pass whatever string to be\n> stored, *and* make the caller responsible for freeing them *if* it\n> passed an allocated string).\n\nI'd actually argue the other way: the simplest interface is one where\nthe string list owns all of its pointers. That keeps the\nownership/lifetime issues clear, and it's one less step for the caller\nto have to remember to do at the end (they do have to clear() the list,\nbut they must do that anyway to free the array of items).\n\nIt does mean that some callers may have to remember to free a temporary\nbuffer right after adding its contents to the list. But that's a lesser\nevil, I think, since the memory ownership issues are all clearly\nresolved at the time of add.\n\nThe big cost is just extra copies/allocations.\n\nI dunno. I actually do not mind the \"nodup\" version of append being used\non a \"dup\" list. It is just a way of letting each call decide on whether\nto hand over ownership, and I think most sites are pretty clear.\n\n> For the push_refs_with_push() patch you sent, another possible fix\n> would be to make cas_options a nodup kind so that the result of\n> strbuf_detach() does not get an extra strdup to be lost when placed\n> in cas_options. With the current string-list API that would not\n> quite work, because freeing done in _release() is tied to the\n> \"dup/nodup\" ness of the string list. I think there even is a\n> codepath that initializes a string_list as nodup kind, stuffs string\n> in it giving the ownership, and then flips it into dup kind just\n> before calling _release() only to have it free the strings, or\n> something silly/ugly like that.\n\nYes, the first grep hit for NODUP is bisect_clean_state(), which does\nthis. I think it would be more clear if we could do:\n\ndiff --git a/bisect.c b/bisect.c\nindex 0fca17c02b..7c59408a13 100644\n--- a/bisect.c\n+++ b/bisect.c\n@@ -1060,8 +1060,7 @@ static int mark_for_removal(const char *refname, const struct object_id *oid,\n \t\t\t int flag, void *cb_data)\n {\n \tstruct string_list *refs = cb_data;\n-\tchar *ref = xstrfmt(\"refs/bisect%s\", refname);\n-\tstring_list_append(refs, ref);\n+\tstring_list_appendf(refs, \"refs/bisect%s\", refname);\n \treturn 0;\n }\n \n@@ -1070,11 +1069,10 @@ int bisect_clean_state(void)\n \tint result = 0;\n \n \t/* There may be some refs packed during bisection */\n-\tstruct string_list refs_for_removal = STRING_LIST_INIT_NODUP;\n+\tstruct string_list refs_for_removal = STRING_LIST_INIT_DUP;\n \tfor_each_ref_in(\"refs/bisect\", mark_for_removal, (void *) &refs_for_removal);\n \tstring_list_append(&refs_for_removal, xstrdup(\"BISECT_HEAD\"));\n \tresult = delete_refs(\"bisect: remove\", &refs_for_removal, REF_NO_DEREF);\n-\trefs_for_removal.strdup_strings = 1;\n \tstring_list_clear(&refs_for_removal, 0);\n \tunlink_or_warn(git_path_bisect_expected_rev());\n \tunlink_or_warn(git_path_bisect_ancestors_ok());\n\n\nHaving a \"format into a string\" wrapper doesn't cover _every_ string you\nmight want to add to a list, but my experience with argv_array_pushf\nleads me to believe that it covers quite a lot of cases.\n\nI dunno. I am not so bothered by the current dual-nature that I think it\nis worth going on a crusade to eliminate one side. But I'm OK if\nsomebody else wants to do so.\n\n-Peff\n"},{"id":"334987","messageId":"f1584860-d0d6-db82-0a49-021924c3e2b7@web.de","threadId":"47400","inReplyTo":"20171208212832.GC7355@sigill.intra.peff.net","subject":"Re: [PATCH] fmt-merge-msg: avoid leaking strbuf in shortlog()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2017-12-18T19:18:17Z","receivedAt":"2017-12-18T19:18:33Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 08.12.2017 um 22:28 schrieb Jeff King:\n> On Fri, Dec 08, 2017 at 10:37:08AM -0800, Junio C Hamano wrote:\n> \n>>> The two modes (dup/nodup) make string_list code tricky. Not sure\n>>> how far we'd get with something simpler (e.g. an array of char pointers),\n>>> but having the caller do all string allocations would make the code\n>>> easier to analyze.\n>>\n>> Yes.\n>>\n>> It probably would have been more sensible if the API did not have\n>> two modes (instead, have the caller pass whatever string to be\n>> stored, *and* make the caller responsible for freeing them *if* it\n>> passed an allocated string).\n> \n> I'd actually argue the other way: the simplest interface is one where\n> the string list owns all of its pointers. That keeps the\n> ownership/lifetime issues clear, and it's one less step for the caller\n> to have to remember to do at the end (they do have to clear() the list,\n> but they must do that anyway to free the array of items).\n> \n> It does mean that some callers may have to remember to free a temporary\n> buffer right after adding its contents to the list. But that's a lesser\n> evil, I think, since the memory ownership issues are all clearly\n> resolved at the time of add.\n> \n> The big cost is just extra copies/allocations.\n\nAn interface requiring callers to allocate can be used to implement a\nwrapper that does all allocations for them -- the other way around is\nharder. It can be used to avoid object duplication, but duplicates\nfunctions. No idea if that's worth it.\n \n>> For the push_refs_with_push() patch you sent, another possible fix\n>> would be to make cas_options a nodup kind so that the result of\n>> strbuf_detach() does not get an extra strdup to be lost when placed\n>> in cas_options. With the current string-list API that would not\n>> quite work, because freeing done in _release() is tied to the\n>> \"dup/nodup\" ness of the string list. I think there even is a\n>> codepath that initializes a string_list as nodup kind, stuffs string\n>> in it giving the ownership, and then flips it into dup kind just\n>> before calling _release() only to have it free the strings, or\n>> something silly/ugly like that.\n> \n> Yes, the first grep hit for NODUP is bisect_clean_state(), which does\n> this. I think it would be more clear if we could do:\n> \n> diff --git a/bisect.c b/bisect.c\n> index 0fca17c02b..7c59408a13 100644\n> --- a/bisect.c\n> +++ b/bisect.c\n> @@ -1060,8 +1060,7 @@ static int mark_for_removal(const char *refname, const struct object_id *oid,\n> \t\t\t int flag, void *cb_data)\n> {\n> \tstruct string_list *refs = cb_data;\n> -\tchar *ref = xstrfmt(\"refs/bisect%s\", refname);\n> -\tstring_list_append(refs, ref);\n> +\tstring_list_appendf(refs, \"refs/bisect%s\", refname);\n> \treturn 0;\n> }\n> \n> @@ -1070,11 +1069,10 @@ int bisect_clean_state(void)\n> \tint result = 0;\n> \n> \t/* There may be some refs packed during bisection */\n> -\tstruct string_list refs_for_removal = STRING_LIST_INIT_NODUP;\n> +\tstruct string_list refs_for_removal = STRING_LIST_INIT_DUP;\n> \tfor_each_ref_in(\"refs/bisect\", mark_for_removal, (void *) &refs_for_removal);\n> \tstring_list_append(&refs_for_removal, xstrdup(\"BISECT_HEAD\"));\n\nThe xstrdup() here would have to go.\n\n> \tresult = delete_refs(\"bisect: remove\", &refs_for_removal, REF_NO_DEREF);\n> -\trefs_for_removal.strdup_strings = 1;\n> \tstring_list_clear(&refs_for_removal, 0);\n> \tunlink_or_warn(git_path_bisect_expected_rev());\n> \tunlink_or_warn(git_path_bisect_ancestors_ok());\n> \n> \n> Having a \"format into a string\" wrapper doesn't cover _every_ string you\n> might want to add to a list, but my experience with argv_array_pushf\n> leads me to believe that it covers quite a lot of cases.\n\nIt would fit in with the rest of the API -- we have string_list_append()\nas a wrapper for string_list_append_nodup()+xstrdup() already. We also\nhave similar functions for strbuf and argv_array. I find it a bit sad\nto reimplement xstrfmt() yet again instead of using it directly, though.\n\nRené\n"},{"id":"335015","messageId":"20171219113855.GA24558@sigill.intra.peff.net","threadId":"47400","inReplyTo":"f1584860-d0d6-db82-0a49-021924c3e2b7@web.de","subject":"Re: [PATCH] fmt-merge-msg: avoid leaking strbuf in shortlog()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-12-19T11:38:56Z","receivedAt":"2017-12-19T11:39:06Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Dec 18, 2017 at 08:18:17PM +0100, René Scharfe wrote:\n\n> > I'd actually argue the other way: the simplest interface is one where\n> > the string list owns all of its pointers. That keeps the\n> > ownership/lifetime issues clear, and it's one less step for the caller\n> > to have to remember to do at the end (they do have to clear() the list,\n> > but they must do that anyway to free the array of items).\n> > \n> > It does mean that some callers may have to remember to free a temporary\n> > buffer right after adding its contents to the list. But that's a lesser\n> > evil, I think, since the memory ownership issues are all clearly\n> > resolved at the time of add.\n> > \n> > The big cost is just extra copies/allocations.\n> \n> An interface requiring callers to allocate can be used to implement a\n> wrapper that does all allocations for them -- the other way around is\n> harder. It can be used to avoid object duplication, but duplicates\n> functions. No idea if that's worth it.\n\nSure, but would anybody actually want to _use_ the non-wrapped version?\nThat's the same duality we have now with string_list.\n\n> > Having a \"format into a string\" wrapper doesn't cover _every_ string you\n> > might want to add to a list, but my experience with argv_array_pushf\n> > leads me to believe that it covers quite a lot of cases.\n> \n> It would fit in with the rest of the API -- we have string_list_append()\n> as a wrapper for string_list_append_nodup()+xstrdup() already. We also\n> have similar functions for strbuf and argv_array. I find it a bit sad\n> to reimplement xstrfmt() yet again instead of using it directly, though.\n\nI dunno, I think could provide some safety and some clarity. IOW, why\n_don't_ we like:\n\n string_list_append_nodup(list, xstrfmt(fmt, ...));\n\n? I think because:\n\n 1. It's a bit long and ugly.\n\n 2. It requires a magic \"nodup\", because we're violating the memory\n ownership boundary.\n\n 3. It doesn't provide any safety for the case where strdup_strings is\n not set, making it easy to leak accidentally.\n\nDoing:\n\n string_list_appendf(list, fmt, ...);\n\npushes the memory ownership semantics \"under the hood\" of the\nstring_list API. And as opposed to being a simple wrapper, it could\nassert that strdup_strings is set (we already do some similar assertions\nin the split functions).\n\n-Peff\n"},{"id":"335033","messageId":"f79ae9a3-de47-c41e-fea6-6a0fedf4ef63@web.de","threadId":"47400","inReplyTo":"20171219113855.GA24558@sigill.intra.peff.net","subject":"Re: [PATCH] fmt-merge-msg: avoid leaking strbuf in shortlog()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2017-12-19T18:26:06Z","receivedAt":"2017-12-19T18:26:25Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 19.12.2017 um 12:38 schrieb Jeff King:\n> On Mon, Dec 18, 2017 at 08:18:17PM +0100, René Scharfe wrote:\n> \n>>> I'd actually argue the other way: the simplest interface is one where\n>>> the string list owns all of its pointers. That keeps the\n>>> ownership/lifetime issues clear, and it's one less step for the caller\n>>> to have to remember to do at the end (they do have to clear() the list,\n>>> but they must do that anyway to free the array of items).\n>>>\n>>> It does mean that some callers may have to remember to free a temporary\n>>> buffer right after adding its contents to the list. But that's a lesser\n>>> evil, I think, since the memory ownership issues are all clearly\n>>> resolved at the time of add.\n>>>\n>>> The big cost is just extra copies/allocations.\n>>\n>> An interface requiring callers to allocate can be used to implement a\n>> wrapper that does all allocations for them -- the other way around is\n>> harder. It can be used to avoid object duplication, but duplicates\n>> functions. No idea if that's worth it.\n> \n> Sure, but would anybody actually want to _use_ the non-wrapped version?\n\nNot sure, but cases that currently use STRING_LIST_INIT_NODUP probably\napply. Apropos: apply.c::write_out_results() looks like it might, too.\n\nAnother question is how much it would cost to let them duplicate strings\nas well in order to simplify the code.\n\n> That's the same duality we have now with string_list.\n\nHmm, I thought we *were* discussing string_list?\n\n>>> Having a \"format into a string\" wrapper doesn't cover _every_ string you\n>>> might want to add to a list, but my experience with argv_array_pushf\n>>> leads me to believe that it covers quite a lot of cases.\n>>\n>> It would fit in with the rest of the API -- we have string_list_append()\n>> as a wrapper for string_list_append_nodup()+xstrdup() already. We also\n>> have similar functions for strbuf and argv_array. I find it a bit sad\n>> to reimplement xstrfmt() yet again instead of using it directly, though.\n> \n> I dunno, I think could provide some safety and some clarity. IOW, why\n> _don't_ we like:\n> \n> string_list_append_nodup(list, xstrfmt(fmt, ...));\n> \n> ? I think because:\n> \n> 1. It's a bit long and ugly.\n> \n> 2. It requires a magic \"nodup\", because we're violating the memory\n> ownership boundary.\n> \n> 3. It doesn't provide any safety for the case where strdup_strings is\n> not set, making it easy to leak accidentally.\n\nRight, and at least 2 and 3 would be solved by having distinct types for\nthe plain and the duplicating variants. The plain one would always\n\"nodup\" and would have no flags that need to be checked.\n\n> Doing:\n> \n> string_list_appendf(list, fmt, ...);\n> \n> pushes the memory ownership semantics \"under the hood\" of the\n> string_list API. And as opposed to being a simple wrapper, it could\n> assert that strdup_strings is set (we already do some similar assertions\n> in the split functions).\n\nYes, that check would guard against leaks.\n\nHaving few functions that can be combined looks like a cleaner interface\nto me than having additional shortcuts for specific combinations -- less\nduplication, smaller surface.\n\nThat said I'm not against adding string_list_appendf(); we already have\nsimilar functions for other types.\n\nRené\n"},{"id":"335074","messageId":"20171220130501.GC17569@sigill.intra.peff.net","threadId":"47400","inReplyTo":"f79ae9a3-de47-c41e-fea6-6a0fedf4ef63@web.de","subject":"Re: [PATCH] fmt-merge-msg: avoid leaking strbuf in shortlog()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-12-20T13:05:02Z","receivedAt":"2017-12-20T13:05:12Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Dec 19, 2017 at 07:26:06PM +0100, René Scharfe wrote:\n\n> > That's the same duality we have now with string_list.\n> \n> Hmm, I thought we *were* discussing string_list?\n\nRight, I guess what I was wondering is if a wrapper over string_list\nreally ends up any better than having the dual-natured string_list.\n\nIf they both use the same struct, then your wrappers are all just\nfunctions. And isn't that more or less what we have now?\n\nIf they're actually different structs, then that complicates call\nsignatures for functions that take a list (unless we are getting into\npolymorphism, they need to specify one of the types, even if they don't\nparticularly care whether it's an allocated list or not).\n\n-Peff\n"}]}