{"thread":{"id":"46790","subject":"[PATCH] commit: fix memory leak in `prepare_index()`","startedAt":"2017-09-20T19:47:59Z","lastAt":"2017-09-20T19:55:31Z","messageCount":2,"participants":["Martin Ågren","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"328488","messageId":"1505936846-2195-1-git-send-email-martin.agren@gmail.com","threadId":"46790","inReplyTo":null,"subject":"[PATCH] commit: fix memory leak in `prepare_index()`","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2017-09-20T19:47:23Z","receivedAt":"2017-09-20T19:47:59Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"Release `pathspec` and the string list `partial`.\n\nWhen we clear the string list, make sure we do not free the `util`\npointers. That would result in double-freeing, since we set them up as\n`item->util = item` in `list_paths()`.\n\nInitialize the string list early, so that we can always release it. That\nintroduces some unnecessary overhead in various code paths, but means\nthere is one and only one way out of the function. If we ever accumulate\nmore things we need to free, it should be straightforward to do so.\n\nSigned-off-by: Martin Ågren <martin.agren@gmail.com>\n---\n builtin/commit.c | 15 ++++++++++-----\n 1 file changed, 10 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex 58f9747..721e5f2 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -335,7 +335,7 @@ static void refresh_cache_or_die(int refresh_flags)\n static const char *prepare_index(int argc, const char **argv, const char *prefix,\n \t\t\t\t const struct commit *current_head, int is_status)\n {\n-\tstruct string_list partial;\n+\tstruct string_list partial = STRING_LIST_INIT_DUP;\n \tstruct pathspec pathspec;\n \tint refresh_flags = REFRESH_QUIET;\n \tconst char *ret;\n@@ -380,7 +380,8 @@ static const char *prepare_index(int argc, const char **argv, const char *prefix\n \t\t\twarning(_(\"Failed to update main cache tree\"));\n \n \t\tcommit_style = COMMIT_NORMAL;\n-\t\treturn get_lock_file_path(&index_lock);\n+\t\tret = get_lock_file_path(&index_lock);\n+\t\tgoto out;\n \t}\n \n \t/*\n@@ -403,7 +404,8 @@ static const char *prepare_index(int argc, const char **argv, const char *prefix\n \t\tif (write_locked_index(&the_index, &index_lock, CLOSE_LOCK))\n \t\t\tdie(_(\"unable to write new_index file\"));\n \t\tcommit_style = COMMIT_NORMAL;\n-\t\treturn get_lock_file_path(&index_lock);\n+\t\tret = get_lock_file_path(&index_lock);\n+\t\tgoto out;\n \t}\n \n \t/*\n@@ -429,7 +431,8 @@ static const char *prepare_index(int argc, const char **argv, const char *prefix\n \t\t\trollback_lock_file(&index_lock);\n \t\t}\n \t\tcommit_style = COMMIT_AS_IS;\n-\t\treturn get_index_file();\n+\t\tret = get_index_file();\n+\t\tgoto out;\n \t}\n \n \t/*\n@@ -460,7 +463,6 @@ static const char *prepare_index(int argc, const char **argv, const char *prefix\n \t\t\tdie(_(\"cannot do a partial commit during a cherry-pick.\"));\n \t}\n \n-\tstring_list_init(&partial, 1);\n \tif (list_paths(&partial, !current_head ? NULL : \"HEAD\", prefix, &pathspec))\n \t\texit(1);\n \n@@ -490,6 +492,9 @@ static const char *prepare_index(int argc, const char **argv, const char *prefix\n \tdiscard_cache();\n \tret = get_lock_file_path(&false_lock);\n \tread_cache_from(ret);\n+out:\n+\tstring_list_clear(&partial, 0);\n+\tclear_pathspec(&pathspec);\n \treturn ret;\n }\n \n-- \n2.14.1\n\n"},{"id":"328492","messageId":"20170920195525.3fx5oq7i4arjnun4@sigill.intra.peff.net","threadId":"46790","inReplyTo":"1505936846-2195-1-git-send-email-martin.agren@gmail.com","subject":"Re: [PATCH] commit: fix memory leak in `prepare_index()`","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-09-20T19:55:25Z","receivedAt":"2017-09-20T19:55:31Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Sep 20, 2017 at 09:47:23PM +0200, Martin Ågren wrote:\n\n> Release `pathspec` and the string list `partial`.\n\nMakes sense.\n\n> When we clear the string list, make sure we do not free the `util`\n> pointers. That would result in double-freeing, since we set them up as\n> `item->util = item` in `list_paths()`.\n\nAlso makes sense (and is kind of weird; it looks like we're just using\nit as a magic flag. But that's outside the scope of your patch).\n\n> Initialize the string list early, so that we can always release it. That\n> introduces some unnecessary overhead in various code paths, but means\n> there is one and only one way out of the function. If we ever accumulate\n> more things we need to free, it should be straightforward to do so.\n\nThe overhead is fine. It's just writing the struct entries, not\nallocating anything. I wondered if the pathspec needed a similar\ninitialization (since you can't tell just from reading the context), but\nno, it's initialized as the first thing in the function.\n\n> Signed-off-by: Martin Ågren <martin.agren@gmail.com>\n> ---\n>  builtin/commit.c | 15 ++++++++++-----\n>  1 file changed, 10 insertions(+), 5 deletions(-)\n\nLooks good to me. Thanks.\n\n-Peff\n"}]}