{"thread":{"id":"46044","subject":"[PATCH] connect.c: fix leak in parse_one_symref_info()","startedAt":"2017-05-25T19:33:15Z","lastAt":"2017-05-25T19:33:15Z","messageCount":1,"participants":["Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"320729","messageId":"20170525193304.fhtmywv4xisclhii@sigill.intra.peff.net","threadId":"46044","inReplyTo":null,"subject":"[PATCH] connect.c: fix leak in parse_one_symref_info()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-05-25T19:33:05Z","receivedAt":"2017-05-25T19:33:15Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"If we successfully parse a symref value like\n\"HEAD:refs/heads/master\", we add the result to a string\nlist. But because the string list is marked\nSTRING_LIST_INIT_DUP, the string list code will make a copy\nof the string and add the copy.\n\nThis patch fixes it by adding the entry with\nstring_list_append_nodup(), which lets the string list take\nownership of our newly allocated string. There are two\nalternatives that seem like they would work, but aren't the\nright solution.\n\nThe first is to initialize the list with the \"NODUP\"\ninitializer. That would avoid the copy, but then the string\nlist would not realize that it owns the strings. When we\neventually call string_list_clear(), it would not free the\nstrings, causing a leak.\n\nThe second option would be to use the normal\nstring_list_append(), but free the local copy in our\nfunction. We can't do this because the local copy actually\ncontains _two_ strings; the symref name and its target. We\npoint to the target pointer via the \"util\" field, and its\nmemory must last as long as the string list does.\n\nYou may also wonder whether it's safe to ever free the local\ncopy, since the target points into it. The answer is yes,\nbecause we duplicate it in annotaate_refs_with_symref_info\nbefore clearing the string list.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nPhew. For a one-line leak fix, that sure was complicated.\n\nI doubt it matters much in practice, because servers send only a single\nHEAD, so we just leak one string. But I happened to notice it while\nlooking at the unborn-HEAD thing.\n\n connect.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/connect.c b/connect.c\nindex cd21a1b6f..c72b1d115 100644\n--- a/connect.c\n+++ b/connect.c\n@@ -71,7 +71,7 @@ static void parse_one_symref_info(struct string_list *symref, const char *val, i\n \t    check_refname_format(target, REFNAME_ALLOW_ONELEVEL))\n \t\t/* \"symref=bogus:pair */\n \t\tgoto reject;\n-\titem = string_list_append(symref, sym);\n+\titem = string_list_append_nodup(symref, sym);\n \titem->util = target;\n \treturn;\n reject:\n-- \n2.13.0.496.ge44ba89db\n"}]}