{"thread":{"id":"47088","subject":"[PATCH] reduce_heads: fix memory leaks","startedAt":"2017-11-01T09:03:54Z","lastAt":"2017-11-08T02:40:25Z","messageCount":12,"participants":["Martin Ågren","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"331552","messageId":"20171101090326.8091-1-martin.agren@gmail.com","threadId":"47088","inReplyTo":null,"subject":"[PATCH] reduce_heads: fix memory leaks","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2017-11-01T09:03:26Z","receivedAt":"2017-11-01T09:03:54Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"We currently have seven callers of `reduce_heads(foo)`. Six of them do\nnot use the original list `foo` again, and actually, all six of those\nend up leaking it.\n\nIntroduce and use `reduce_heads_replace(&foo)` as a leak-free version of\n`foo = reduce_heads(foo)` to fix several of these. Fix the remaining\nleaks using `free_commit_list()`.\n\nWhile we're here, document `reduce_heads()` and mark it as `extern`.\n\nSigned-off-by: Martin Ågren <martin.agren@gmail.com>\n---\n builtin/commit.c        |  2 +-\n builtin/fmt-merge-msg.c |  2 +-\n builtin/merge-base.c    |  6 +++++-\n builtin/merge.c         |  1 +\n builtin/pull.c          |  4 +++-\n commit.c                |  7 +++++++\n commit.h                | 18 +++++++++++++++++-\n 7 files changed, 35 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex d75b3805e..11c474018 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -1728,7 +1728,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n \t\t\t\tallow_fast_forward = 0;\n \t\t}\n \t\tif (allow_fast_forward)\n-\t\t\tparents = reduce_heads(parents);\n+\t\t\treduce_heads_replace(&parents);\n \t} else {\n \t\tif (!reflog_msg)\n \t\t\treflog_msg = (whence == FROM_CHERRY_PICK)\ndiff --git a/builtin/fmt-merge-msg.c b/builtin/fmt-merge-msg.c\nindex e99b5ddbf..27a2361e9 100644\n--- a/builtin/fmt-merge-msg.c\n+++ b/builtin/fmt-merge-msg.c\n@@ -571,7 +571,7 @@ static void find_merge_parents(struct merge_parents *result,\n \thead_commit = lookup_commit(head);\n \tif (head_commit)\n \t\tcommit_list_insert(head_commit, &parents);\n-\tparents = reduce_heads(parents);\n+\treduce_heads_replace(&parents);\n \n \twhile (parents) {\n \t\tstruct commit *cmit = pop_commit(&parents);\ndiff --git a/builtin/merge-base.c b/builtin/merge-base.c\nindex 6dbd167d3..b1b7590c4 100644\n--- a/builtin/merge-base.c\n+++ b/builtin/merge-base.c\n@@ -59,6 +59,8 @@ static int handle_independent(int count, const char **args)\n \t\tcommit_list_insert(get_commit_reference(args[i]), &revs);\n \n \tresult = reduce_heads(revs);\n+\tfree_commit_list(revs);\n+\n \tif (!result)\n \t\treturn 1;\n \n@@ -78,7 +80,9 @@ static int handle_octopus(int count, const char **args, int show_all)\n \tfor (i = count - 1; i >= 0; i--)\n \t\tcommit_list_insert(get_commit_reference(args[i]), &revs);\n \n-\tresult = reduce_heads(get_octopus_merge_bases(revs));\n+\tresult = get_octopus_merge_bases(revs);\n+\tfree_commit_list(revs);\n+\treduce_heads_replace(&result);\n \n \tif (!result)\n \t\treturn 1;\ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex ab5ffe85e..fbbf2a9e5 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -999,6 +999,7 @@ static struct commit_list *reduce_parents(struct commit *head_commit,\n \n \t/* Find what parents to record by checking independent ones. */\n \tparents = reduce_heads(remoteheads);\n+\tfree_commit_list(remoteheads);\n \n \tremoteheads = NULL;\n \tremotes = &remoteheads;\ndiff --git a/builtin/pull.c b/builtin/pull.c\nindex 6f772e8a2..5eeaa8c68 100644\n--- a/builtin/pull.c\n+++ b/builtin/pull.c\n@@ -745,8 +745,10 @@ static int get_octopus_merge_base(struct object_id *merge_base,\n \tif (!is_null_oid(fork_point))\n \t\tcommit_list_insert(lookup_commit_reference(fork_point), &revs);\n \n-\tresult = reduce_heads(get_octopus_merge_bases(revs));\n+\tresult = get_octopus_merge_bases(revs);\n \tfree_commit_list(revs);\n+\treduce_heads_replace(&result);\n+\n \tif (!result)\n \t\treturn 1;\n \ndiff --git a/commit.c b/commit.c\nindex 1e0e63379..cab8d4455 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -1090,6 +1090,13 @@ struct commit_list *reduce_heads(struct commit_list *heads)\n \treturn result;\n }\n \n+void reduce_heads_replace(struct commit_list **heads)\n+{\n+\tstruct commit_list *result = reduce_heads(*heads);\n+\tfree_commit_list(*heads);\n+\t*heads = result;\n+}\n+\n static const char gpg_sig_header[] = \"gpgsig\";\n static const int gpg_sig_header_len = sizeof(gpg_sig_header) - 1;\n \ndiff --git a/commit.h b/commit.h\nindex 6d769590f..99a3fea68 100644\n--- a/commit.h\n+++ b/commit.h\n@@ -313,7 +313,23 @@ extern int interactive_add(int argc, const char **argv, const char *prefix, int\n extern int run_add_interactive(const char *revision, const char *patch_mode,\n \t\t\t       const struct pathspec *pathspec);\n \n-struct commit_list *reduce_heads(struct commit_list *heads);\n+/*\n+ * Takes a list of commits and returns a new list where those\n+ * have been removed that can be reached from other commits in\n+ * the list. It is useful for, e.g., reducing the commits\n+ * randomly thrown at the git-merge command and removing\n+ * redundant commits that the user shouldn't have given to it.\n+ *\n+ * This function destroys the STALE bit of the commit objects'\n+ * flags.\n+ */\n+extern struct commit_list *reduce_heads(struct commit_list *heads);\n+\n+/*\n+ * Like `reduce_heads()`, except it replaces the list. Use this\n+ * instead of `foo = reduce_heads(foo);` to avoid memory leaks.\n+ */\n+extern void reduce_heads_replace(struct commit_list **heads);\n \n struct commit_extra_header {\n \tstruct commit_extra_header *next;\n-- \n2.15.0.415.gac1375d7e\n\n"},{"id":"331640","messageId":"xmqq7ev9s7bp.fsf@gitster.mtv.corp.google.com","threadId":"47088","inReplyTo":"20171101090326.8091-1-martin.agren@gmail.com","subject":"Re: [PATCH] reduce_heads: fix memory leaks","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-11-02T03:11:38Z","receivedAt":"2017-11-02T03:11:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Martin Ågren <martin.agren@gmail.com> writes:\n\n> diff --git a/builtin/merge-base.c b/builtin/merge-base.c\n> index 6dbd167d3..b1b7590c4 100644\n> --- a/builtin/merge-base.c\n> +++ b/builtin/merge-base.c\n> @@ -59,6 +59,8 @@ static int handle_independent(int count, const char **args)\n>  \t\tcommit_list_insert(get_commit_reference(args[i]), &revs);\n>  \n>  \tresult = reduce_heads(revs);\n> +\tfree_commit_list(revs);\n> +\n>  \tif (!result)\n>  \t\treturn 1;\n\nThe post-context of this hunk continues like so:\n\n\twhile (result) {\n\t\tprintf(\"%s\\n\", oid_to_hex(&result->item->object.oid));\n\t\tresult = result->next;\n\t}\n\treturn 0;\n}\n\nand we end up leaking \"result\".  This function is directly called from\ncmd_merge_base() and its value is returned to main(), so leaking it\nis not that a grave offence, but that excuse applies equally well to\nrevs.  \n\nI can see you are shooting for minimum change in this patch, but if\nwe were writing this code in a codebase where reduce_heads_replace()\nis already available, I would imagine that we wouldn't use two separate\nvariables, perhaps?\n\n> diff --git a/commit.c b/commit.c\n> index 1e0e63379..cab8d4455 100644\n> --- a/commit.c\n> +++ b/commit.c\n> @@ -1090,6 +1090,13 @@ struct commit_list *reduce_heads(struct commit_list *heads)\n>  \treturn result;\n>  }\n>  \n> +void reduce_heads_replace(struct commit_list **heads)\n> +{\n> +\tstruct commit_list *result = reduce_heads(*heads);\n> +\tfree_commit_list(*heads);\n> +\t*heads = result;\n> +}\n> +\n\nLooks good.\n\n> diff --git a/commit.h b/commit.h\n> index 6d769590f..99a3fea68 100644\n> --- a/commit.h\n> +++ b/commit.h\n> @@ -313,7 +313,23 @@ extern int interactive_add(int argc, const char **argv, const char *prefix, int\n>  extern int run_add_interactive(const char *revision, const char *patch_mode,\n>  \t\t\t       const struct pathspec *pathspec);\n>  \n> -struct commit_list *reduce_heads(struct commit_list *heads);\n> +/*\n> + * Takes a list of commits and returns a new list where those\n> + * have been removed that can be reached from other commits in\n> + * the list. It is useful for, e.g., reducing the commits\n> + * randomly thrown at the git-merge command and removing\n> + * redundant commits that the user shouldn't have given to it.\n> + *\n> + * This function destroys the STALE bit of the commit objects'\n> + * flags.\n> + */\n> +extern struct commit_list *reduce_heads(struct commit_list *heads);\n> +\n> +/*\n> + * Like `reduce_heads()`, except it replaces the list. Use this\n> + * instead of `foo = reduce_heads(foo);` to avoid memory leaks.\n> + */\n> +extern void reduce_heads_replace(struct commit_list **heads);\n\nLooks excellent.\n\nThanks.\n"},{"id":"331666","messageId":"CAN0heSpgUBy4a6iok4MoDqJ__hZtrXvfApcxjHuS0vOBKSuShg@mail.gmail.com","threadId":"47088","inReplyTo":"xmqq7ev9s7bp.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] reduce_heads: fix memory leaks","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2017-11-02T10:45:30Z","receivedAt":"2017-11-02T10:45:36Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On 2 November 2017 at 04:11, Junio C Hamano <gitster@pobox.com> wrote:\n> Martin Ågren <martin.agren@gmail.com> writes:\n>\n>> diff --git a/builtin/merge-base.c b/builtin/merge-base.c\n>> index 6dbd167d3..b1b7590c4 100644\n>> --- a/builtin/merge-base.c\n>> +++ b/builtin/merge-base.c\n>> @@ -59,6 +59,8 @@ static int handle_independent(int count, const char **args)\n>>               commit_list_insert(get_commit_reference(args[i]), &revs);\n>>\n>>       result = reduce_heads(revs);\n>> +     free_commit_list(revs);\n>> +\n>>       if (!result)\n>>               return 1;\n>\n> The post-context of this hunk continues like so:\n>\n>         while (result) {\n>                 printf(\"%s\\n\", oid_to_hex(&result->item->object.oid));\n>                 result = result->next;\n>         }\n>         return 0;\n> }\n>\n> and we end up leaking \"result\".  This function is directly called from\n> cmd_merge_base() and its value is returned to main(), so leaking it\n> is not that a grave offence, but that excuse applies equally well to\n> revs.\n\nGood catch. I even have a patch to address the leak of `result`, except\nI seem to have sorted it into another pile. For this series I just\ngrepped for \"reduce_heads\" and didn't stop to think about using UNLEAK,\nnor about the leaking of `result`.\n\n> I can see you are shooting for minimum change in this patch, but if\n> we were writing this code in a codebase where reduce_heads_replace()\n> is already available, I would imagine that we wouldn't use two separate\n> variables, perhaps?\n\nThe way my other patch addresses the leaking of `result` is that it\nrewrites the loop to avoid losing the original value of `result`, so\nthat it can be UNLEAK-ed at the very end. (That makes it obvious where\nthe leak happens, compared to adding an UNLEAK a few lines up.) If I do\n`reduce_heads_replace(&revs)`, I'll need to touch the loop anyway, and\nthen I could probably just as well UNLEAK a little while at it.\n\nI'll get to this within the next couple of days, then I'll see what it\nlooks like.\n\nThanks for your feedback.\n"},{"id":"331879","messageId":"cover.1509908607.git.martin.agren@gmail.com","threadId":"47088","inReplyTo":"CAN0heSpgUBy4a6iok4MoDqJ__hZtrXvfApcxjHuS0vOBKSuShg@mail.gmail.com","subject":"[PATCH v2 0/2] Re: reduce_heads: fix memory leaks","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2017-11-05T20:26:29Z","receivedAt":"2017-11-05T20:26:54Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"Since v1 [1], I've added a preparatory patch to UNLEAK some variables.\nThat sets the stage slightly better for patch 2.\n\nJunio, you placed v1 on maint. Because UNLEAK is not in maint, this is\nbased on master and maint misses out on this v2. If you have any advice\nfor how I should (not) do series with UNLEAK in them, I'm all ears.\n\nMartin\n\n[1] https://public-inbox.org/git/20171101090326.8091-1-martin.agren@gmail.com/\n\nMartin Ågren (2):\n  builtin/merge-base: UNLEAK commit lists\n  reduce_heads: fix memory leaks\n\n commit.h                | 18 +++++++++++++++++-\n builtin/commit.c        |  2 +-\n builtin/fmt-merge-msg.c |  2 +-\n builtin/merge-base.c    | 40 +++++++++++++++++++++-------------------\n builtin/merge.c         |  1 +\n builtin/pull.c          |  5 ++++-\n commit.c                |  7 +++++++\n 7 files changed, 52 insertions(+), 23 deletions(-)\n\n-- \n2.15.0.415.gac1375d7e\n\n"},{"id":"331880","messageId":"b06f593d3f8b0ad53754eeb394f77e7c3ee916bd.1509908607.git.martin.agren@gmail.com","threadId":"47088","inReplyTo":"cover.1509908607.git.martin.agren@gmail.com","subject":"[PATCH v2 1/2] builtin/merge-base: UNLEAK commit lists","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2017-11-05T20:26:30Z","receivedAt":"2017-11-05T20:26:58Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"In several functions, we iterate through a commit list by assigning\n`result = result->next`. As a consequence, we lose the original pointer\nand eventually leak the list.\n\nThese are immediate helpers to `cmd_merge_base()` which is just about to\nreturn, so we can use UNLEAK. For example, we could `UNLEAK(result)`\nbefore we start iterating. That would be a one-liner change per\nfunction. Instead, leave the lists alone and iterate using a dedicated\npointer. Then UNLEAK immediately before returning.\n\nAfter this change, it is clearer that the leaks happen as we return, and\nnot as we process the list. That is, we could just as well have used\n`free_commit_list()`. Also, leaving a \"result\" unchanged as we display\nit feels (marginally) better.\n\nIn `handle_independent()` we can drop `result` while we're here and\nreuse the `revs`-variable instead. That matches several other users of\n`reduce_heads()`. The memory-leak that this hides will be addressed in\nthe next commit.\n\nSigned-off-by: Martin Ågren <martin.agren@gmail.com>\n---\n builtin/merge-base.c | 36 ++++++++++++++++++------------------\n 1 file changed, 18 insertions(+), 18 deletions(-)\n\ndiff --git a/builtin/merge-base.c b/builtin/merge-base.c\nindex 6dbd167d3..fd0eba14b 100644\n--- a/builtin/merge-base.c\n+++ b/builtin/merge-base.c\n@@ -9,20 +9,20 @@\n \n static int show_merge_base(struct commit **rev, int rev_nr, int show_all)\n {\n-\tstruct commit_list *result;\n+\tstruct commit_list *result, *r;\n \n \tresult = get_merge_bases_many_dirty(rev[0], rev_nr - 1, rev + 1);\n \n \tif (!result)\n \t\treturn 1;\n \n-\twhile (result) {\n-\t\tprintf(\"%s\\n\", oid_to_hex(&result->item->object.oid));\n+\tfor (r = result; r; r = r->next) {\n+\t\tprintf(\"%s\\n\", oid_to_hex(&r->item->object.oid));\n \t\tif (!show_all)\n-\t\t\treturn 0;\n-\t\tresult = result->next;\n+\t\t\tbreak;\n \t}\n \n+\tUNLEAK(result);\n \treturn 0;\n }\n \n@@ -51,28 +51,28 @@ static struct commit *get_commit_reference(const char *arg)\n \n static int handle_independent(int count, const char **args)\n {\n-\tstruct commit_list *revs = NULL;\n-\tstruct commit_list *result;\n+\tstruct commit_list *revs = NULL, *rev;\n \tint i;\n \n \tfor (i = count - 1; i >= 0; i--)\n \t\tcommit_list_insert(get_commit_reference(args[i]), &revs);\n \n-\tresult = reduce_heads(revs);\n-\tif (!result)\n+\trevs = reduce_heads(revs);\n+\n+\tif (!revs)\n \t\treturn 1;\n \n-\twhile (result) {\n-\t\tprintf(\"%s\\n\", oid_to_hex(&result->item->object.oid));\n-\t\tresult = result->next;\n-\t}\n+\tfor (rev = revs; rev; rev = rev->next)\n+\t\tprintf(\"%s\\n\", oid_to_hex(&rev->item->object.oid));\n+\n+\tUNLEAK(revs);\n \treturn 0;\n }\n \n static int handle_octopus(int count, const char **args, int show_all)\n {\n \tstruct commit_list *revs = NULL;\n-\tstruct commit_list *result;\n+\tstruct commit_list *result, *rev;\n \tint i;\n \n \tfor (i = count - 1; i >= 0; i--)\n@@ -83,13 +83,13 @@ static int handle_octopus(int count, const char **args, int show_all)\n \tif (!result)\n \t\treturn 1;\n \n-\twhile (result) {\n-\t\tprintf(\"%s\\n\", oid_to_hex(&result->item->object.oid));\n+\tfor (rev = result; rev; rev = rev->next) {\n+\t\tprintf(\"%s\\n\", oid_to_hex(&rev->item->object.oid));\n \t\tif (!show_all)\n-\t\t\treturn 0;\n-\t\tresult = result->next;\n+\t\t\tbreak;\n \t}\n \n+\tUNLEAK(result);\n \treturn 0;\n }\n \n-- \n2.15.0.415.gac1375d7e\n\n"},{"id":"331881","messageId":"e67b7c112b4b7b59e75e0b67e78c0f7f7ea06d84.1509908607.git.martin.agren@gmail.com","threadId":"47088","inReplyTo":"cover.1509908607.git.martin.agren@gmail.com","subject":"[PATCH v2 2/2] reduce_heads: fix memory leaks","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2017-11-05T20:26:31Z","receivedAt":"2017-11-05T20:27:04Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"We currently have seven callers of `reduce_heads(foo)`. Six of them do\nnot use the original list `foo` again, and actually, all six of those\nend up leaking it.\n\nIntroduce and use `reduce_heads_replace(&foo)` as a leak-free version of\n`foo = reduce_heads(foo)` to fix several of these. Fix the remaining\nleaks using `free_commit_list()`.\n\nWhile we're here, document `reduce_heads()` and mark it as `extern`.\n\nSigned-off-by: Martin Ågren <martin.agren@gmail.com>\n---\n commit.h                | 18 +++++++++++++++++-\n builtin/commit.c        |  2 +-\n builtin/fmt-merge-msg.c |  2 +-\n builtin/merge-base.c    |  6 ++++--\n builtin/merge.c         |  1 +\n builtin/pull.c          |  5 ++++-\n commit.c                |  7 +++++++\n 7 files changed, 35 insertions(+), 6 deletions(-)\n\ndiff --git a/commit.h b/commit.h\nindex 6d769590f..99a3fea68 100644\n--- a/commit.h\n+++ b/commit.h\n@@ -313,7 +313,23 @@ extern int interactive_add(int argc, const char **argv, const char *prefix, int\n extern int run_add_interactive(const char *revision, const char *patch_mode,\n \t\t\t       const struct pathspec *pathspec);\n \n-struct commit_list *reduce_heads(struct commit_list *heads);\n+/*\n+ * Takes a list of commits and returns a new list where those\n+ * have been removed that can be reached from other commits in\n+ * the list. It is useful for, e.g., reducing the commits\n+ * randomly thrown at the git-merge command and removing\n+ * redundant commits that the user shouldn't have given to it.\n+ *\n+ * This function destroys the STALE bit of the commit objects'\n+ * flags.\n+ */\n+extern struct commit_list *reduce_heads(struct commit_list *heads);\n+\n+/*\n+ * Like `reduce_heads()`, except it replaces the list. Use this\n+ * instead of `foo = reduce_heads(foo);` to avoid memory leaks.\n+ */\n+extern void reduce_heads_replace(struct commit_list **heads);\n \n struct commit_extra_header {\n \tstruct commit_extra_header *next;\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex d75b3805e..11c474018 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -1728,7 +1728,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n \t\t\t\tallow_fast_forward = 0;\n \t\t}\n \t\tif (allow_fast_forward)\n-\t\t\tparents = reduce_heads(parents);\n+\t\t\treduce_heads_replace(&parents);\n \t} else {\n \t\tif (!reflog_msg)\n \t\t\treflog_msg = (whence == FROM_CHERRY_PICK)\ndiff --git a/builtin/fmt-merge-msg.c b/builtin/fmt-merge-msg.c\nindex e99b5ddbf..27a2361e9 100644\n--- a/builtin/fmt-merge-msg.c\n+++ b/builtin/fmt-merge-msg.c\n@@ -571,7 +571,7 @@ static void find_merge_parents(struct merge_parents *result,\n \thead_commit = lookup_commit(head);\n \tif (head_commit)\n \t\tcommit_list_insert(head_commit, &parents);\n-\tparents = reduce_heads(parents);\n+\treduce_heads_replace(&parents);\n \n \twhile (parents) {\n \t\tstruct commit *cmit = pop_commit(&parents);\ndiff --git a/builtin/merge-base.c b/builtin/merge-base.c\nindex fd0eba14b..0178ca772 100644\n--- a/builtin/merge-base.c\n+++ b/builtin/merge-base.c\n@@ -57,7 +57,7 @@ static int handle_independent(int count, const char **args)\n \tfor (i = count - 1; i >= 0; i--)\n \t\tcommit_list_insert(get_commit_reference(args[i]), &revs);\n \n-\trevs = reduce_heads(revs);\n+\treduce_heads_replace(&revs);\n \n \tif (!revs)\n \t\treturn 1;\n@@ -78,7 +78,9 @@ static int handle_octopus(int count, const char **args, int show_all)\n \tfor (i = count - 1; i >= 0; i--)\n \t\tcommit_list_insert(get_commit_reference(args[i]), &revs);\n \n-\tresult = reduce_heads(get_octopus_merge_bases(revs));\n+\tresult = get_octopus_merge_bases(revs);\n+\tfree_commit_list(revs);\n+\treduce_heads_replace(&result);\n \n \tif (!result)\n \t\treturn 1;\ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex ab5ffe85e..fbbf2a9e5 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -999,6 +999,7 @@ static struct commit_list *reduce_parents(struct commit *head_commit,\n \n \t/* Find what parents to record by checking independent ones. */\n \tparents = reduce_heads(remoteheads);\n+\tfree_commit_list(remoteheads);\n \n \tremoteheads = NULL;\n \tremotes = &remoteheads;\ndiff --git a/builtin/pull.c b/builtin/pull.c\nindex 6f772e8a2..4edab228e 100644\n--- a/builtin/pull.c\n+++ b/builtin/pull.c\n@@ -745,12 +745,15 @@ static int get_octopus_merge_base(struct object_id *merge_base,\n \tif (!is_null_oid(fork_point))\n \t\tcommit_list_insert(lookup_commit_reference(fork_point), &revs);\n \n-\tresult = reduce_heads(get_octopus_merge_bases(revs));\n+\tresult = get_octopus_merge_bases(revs);\n \tfree_commit_list(revs);\n+\treduce_heads_replace(&result);\n+\n \tif (!result)\n \t\treturn 1;\n \n \toidcpy(merge_base, &result->item->object.oid);\n+\tfree_commit_list(result);\n \treturn 0;\n }\n \ndiff --git a/commit.c b/commit.c\nindex 1e0e63379..cab8d4455 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -1090,6 +1090,13 @@ struct commit_list *reduce_heads(struct commit_list *heads)\n \treturn result;\n }\n \n+void reduce_heads_replace(struct commit_list **heads)\n+{\n+\tstruct commit_list *result = reduce_heads(*heads);\n+\tfree_commit_list(*heads);\n+\t*heads = result;\n+}\n+\n static const char gpg_sig_header[] = \"gpgsig\";\n static const int gpg_sig_header_len = sizeof(gpg_sig_header) - 1;\n \n-- \n2.15.0.415.gac1375d7e\n\n"},{"id":"331892","messageId":"xmqqa800kyks.fsf@gitster.mtv.corp.google.com","threadId":"47088","inReplyTo":"b06f593d3f8b0ad53754eeb394f77e7c3ee916bd.1509908607.git.martin.agren@gmail.com","subject":"Re: [PATCH v2 1/2] builtin/merge-base: UNLEAK commit lists","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-11-06T01:03:47Z","receivedAt":"2017-11-06T01:03:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Martin Ågren <martin.agren@gmail.com> writes:\n\n> In several functions, we iterate through a commit list by assigning\n> `result = result->next`. As a consequence, we lose the original pointer\n> and eventually leak the list.\n>\n> These are immediate helpers to `cmd_merge_base()` which is just about to\n> return, so we can use UNLEAK. For example, we could `UNLEAK(result)`\n> before we start iterating. That would be a one-liner change per\n> function. Instead, leave the lists alone and iterate using a dedicated\n> pointer. Then UNLEAK immediately before returning.\n\nHmm, I cannot shake this feeling that this goes somewhat opposite to\nthe spirit of UNLEAK(), which I view as \"It is too cumbersome and\nmakes the resulting code ugly if we try to make everything properly\nfreed, so mark what we know we will leak upfront\".  The result of\nthis patch feels more like \"Even though we took pains to restructure\nthe code so that we could call free_commit_list() to properly free\nthings, we use UNLEAK() and do not actually bother to free.\"\n\nHavin said that, I do not think the resulting code has become uglier\nor the conversion process (both writing and reviewing) were too\ncumbersome for this particular case, and marking where we could call\nfree_commit_list() with UNLEAK() like this patch does may make\nsense.  If somebody someday wants to call some of these helpers in\nother contexts repeatedly, they may have an easier time.\n"},{"id":"331893","messageId":"xmqq60aoky5z.fsf@gitster.mtv.corp.google.com","threadId":"47088","inReplyTo":"cover.1509908607.git.martin.agren@gmail.com","subject":"Re: [PATCH v2 0/2] Re: reduce_heads: fix memory leaks","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-11-06T01:12:40Z","receivedAt":"2017-11-06T01:12:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Martin Ågren <martin.agren@gmail.com> writes:\n\n> Since v1 [1], I've added a preparatory patch to UNLEAK some variables.\n> That sets the stage slightly better for patch 2.\n>\n> Junio, you placed v1 on maint. Because UNLEAK is not in maint, this is\n> based on master and maint misses out on this v2. If you have any advice\n> for how I should (not) do series with UNLEAK in them, I'm all ears.\n\nAs far as we know, nobody reported that these leaks made Git run out\nof memory while running merge-base and prevented them from getting\ndesired result, so it is not worth the effort to make (part of) them\nmergeable to 'maint'.  I forked the branch from 'maint' only because\nit was a fix and it was not harder than forking from 'master'.\n\nIf 2/2 (which was 1/1 in the v1) were fixes to a very grave error,\nthen I might have suggested to do the 2/2 on maint first and call\nthat topic ${some_grave_error}_fix-maint; then fork another topic\n${some_grave_error}_fix at master, merge the _fix-maint topic in,\nand then do the 1/2 on top.\n"},{"id":"331915","messageId":"20171106110549.dkv725bjikcms3aj@sigill.intra.peff.net","threadId":"47088","inReplyTo":"b06f593d3f8b0ad53754eeb394f77e7c3ee916bd.1509908607.git.martin.agren@gmail.com","subject":"Re: [PATCH v2 1/2] builtin/merge-base: UNLEAK commit lists","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-11-06T11:05:50Z","receivedAt":"2017-11-06T11:05:56Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Nov 05, 2017 at 09:26:30PM +0100, Martin Ågren wrote:\n\n> In several functions, we iterate through a commit list by assigning\n> `result = result->next`. As a consequence, we lose the original pointer\n> and eventually leak the list.\n> \n> These are immediate helpers to `cmd_merge_base()` which is just about to\n> return, so we can use UNLEAK. For example, we could `UNLEAK(result)`\n> before we start iterating. That would be a one-liner change per\n> function. Instead, leave the lists alone and iterate using a dedicated\n> pointer. Then UNLEAK immediately before returning.\n> \n> After this change, it is clearer that the leaks happen as we return, and\n> not as we process the list. That is, we could just as well have used\n> `free_commit_list()`. Also, leaving a \"result\" unchanged as we display\n> it feels (marginally) better.\n\nI think it would be OK to show that we are consuming the list as we go,\nlike:\n\n  while ((commit = pop_commit(&result))\n\t...do the thing ...\n\nbut like you I think I prefer the read-only iteration followed by a\nseparate deallocation/leak phase.\n\nLike Junio, though, I kind of wonder if just calling free_commit_list()\nwould be the most readable thing.\n\nThe \"other\" benefit of UNLEAK() is that it has zero runtime cost. I'm\nnot sure if that matters much here, so I'd tend to choose the thing that\nis most readable / understandable. These aren't cmd_* functions, so it's\nnot _immediately_ obvious that it is OK for them to leak. But it's also\npretty easy to see that they are called as the final element of\ncmd_merge_base(). So I'm on the fence as far as this being a good use of\nUNLEAK() or not.\n\n-Peff\n"},{"id":"332032","messageId":"a5f5a259f4cbe3661eb1960e83e9bcce5080b580.1510083859.git.martin.agren@gmail.com","threadId":"47088","inReplyTo":"20171106110549.dkv725bjikcms3aj@sigill.intra.peff.net","subject":"[PATCH v3 1/2] builtin/merge-base: free commit lists","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2017-11-07T20:39:44Z","receivedAt":"2017-11-07T20:40:00Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"In several functions, we iterate through a commit list by assigning\n`result = result->next`. As a consequence, we lose the original pointer\nand eventually leak the list.\n\nRewrite the loops so that we keep the original pointers, then call\n`free_commit_list()`. Various alternatives were considered:\n\n1) Use `UNLEAK(result)` before the loop. Simple change, but not very\npretty. These would definitely be new lows among our usages of UNLEAK.\n2) Use `pop_commit()` when looping. Slightly less simple change, but it\nfeels slightly preferable to first display the list, then free it.\n3) As in this patch, but with `UNLEAK()` instead of freeing. We'd still\ngo through all the trouble of refactoring the loop, and because it's not\nsuper-obvious that we're about to exit, let's just free the lists -- it\nprobably doesn't affect the runtime much.\n\nIn `handle_independent()` we can drop `result` while we're here and\nreuse the `revs`-variable instead. That matches several other users of\n`reduce_heads()`. The memory-leak that this hides will be addressed in\nthe next commit.\n\nSigned-off-by: Martin Ågren <martin.agren@gmail.com>\n---\nv3: Like v2 but s/UNLEAK/free_commit_list/ and rebased onto maint.\n\n> Like Junio, though, I kind of wonder if just calling free_commit_list()\n> would be the most readable thing.\n\nThanks Junio and Peff for insightful considerations around UNLEAK vs\nfree. I'd rather miss one or two opportunities too UNLEAK than use it\ntoo often. I've got a couple of patches in the pipeline that will be\nbetter thanks to your comments. Thanks.\n\n builtin/merge-base.c | 36 ++++++++++++++++++------------------\n 1 file changed, 18 insertions(+), 18 deletions(-)\n\ndiff --git a/builtin/merge-base.c b/builtin/merge-base.c\nindex 6dbd167d3b..e17835fabb 100644\n--- a/builtin/merge-base.c\n+++ b/builtin/merge-base.c\n@@ -9,20 +9,20 @@\n \n static int show_merge_base(struct commit **rev, int rev_nr, int show_all)\n {\n-\tstruct commit_list *result;\n+\tstruct commit_list *result, *r;\n \n \tresult = get_merge_bases_many_dirty(rev[0], rev_nr - 1, rev + 1);\n \n \tif (!result)\n \t\treturn 1;\n \n-\twhile (result) {\n-\t\tprintf(\"%s\\n\", oid_to_hex(&result->item->object.oid));\n+\tfor (r = result; r; r = r->next) {\n+\t\tprintf(\"%s\\n\", oid_to_hex(&r->item->object.oid));\n \t\tif (!show_all)\n-\t\t\treturn 0;\n-\t\tresult = result->next;\n+\t\t\tbreak;\n \t}\n \n+\tfree_commit_list(result);\n \treturn 0;\n }\n \n@@ -51,28 +51,28 @@ static struct commit *get_commit_reference(const char *arg)\n \n static int handle_independent(int count, const char **args)\n {\n-\tstruct commit_list *revs = NULL;\n-\tstruct commit_list *result;\n+\tstruct commit_list *revs = NULL, *rev;\n \tint i;\n \n \tfor (i = count - 1; i >= 0; i--)\n \t\tcommit_list_insert(get_commit_reference(args[i]), &revs);\n \n-\tresult = reduce_heads(revs);\n-\tif (!result)\n+\trevs = reduce_heads(revs);\n+\n+\tif (!revs)\n \t\treturn 1;\n \n-\twhile (result) {\n-\t\tprintf(\"%s\\n\", oid_to_hex(&result->item->object.oid));\n-\t\tresult = result->next;\n-\t}\n+\tfor (rev = revs; rev; rev = rev->next)\n+\t\tprintf(\"%s\\n\", oid_to_hex(&rev->item->object.oid));\n+\n+\tfree_commit_list(revs);\n \treturn 0;\n }\n \n static int handle_octopus(int count, const char **args, int show_all)\n {\n \tstruct commit_list *revs = NULL;\n-\tstruct commit_list *result;\n+\tstruct commit_list *result, *rev;\n \tint i;\n \n \tfor (i = count - 1; i >= 0; i--)\n@@ -83,13 +83,13 @@ static int handle_octopus(int count, const char **args, int show_all)\n \tif (!result)\n \t\treturn 1;\n \n-\twhile (result) {\n-\t\tprintf(\"%s\\n\", oid_to_hex(&result->item->object.oid));\n+\tfor (rev = result; rev; rev = rev->next) {\n+\t\tprintf(\"%s\\n\", oid_to_hex(&rev->item->object.oid));\n \t\tif (!show_all)\n-\t\t\treturn 0;\n-\t\tresult = result->next;\n+\t\t\tbreak;\n \t}\n \n+\tfree_commit_list(result);\n \treturn 0;\n }\n \n-- \n2.15.0.415.gac1375d7e\n\n"},{"id":"332033","messageId":"05f0d03b2cf54e3f8e742d8cc5dc36bfd35e4d70.1510083860.git.martin.agren@gmail.com","threadId":"47088","inReplyTo":"20171106110549.dkv725bjikcms3aj@sigill.intra.peff.net","subject":"[PATCH v3 2/2] reduce_heads: fix memory leaks","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2017-11-07T20:39:45Z","receivedAt":"2017-11-07T20:40:04Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"We currently have seven callers of `reduce_heads(foo)`. Six of them do\nnot use the original list `foo` again, and actually, all six of those\nend up leaking it.\n\nIntroduce and use `reduce_heads_replace(&foo)` as a leak-free version of\n`foo = reduce_heads(foo)` to fix several of these. Fix the remaining\nleaks using `free_commit_list()`.\n\nWhile we're here, document `reduce_heads()` and mark it as `extern`.\n\nSigned-off-by: Martin Ågren <martin.agren@gmail.com>\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\nv3: Like v2 but rebased onto maint.\n\n commit.h                | 18 +++++++++++++++++-\n builtin/commit.c        |  2 +-\n builtin/fmt-merge-msg.c |  2 +-\n builtin/merge-base.c    |  6 ++++--\n builtin/merge.c         |  1 +\n builtin/pull.c          |  5 ++++-\n commit.c                |  7 +++++++\n 7 files changed, 35 insertions(+), 6 deletions(-)\n\ndiff --git a/commit.h b/commit.h\nindex f3805fec6b..cfa2636e33 100644\n--- a/commit.h\n+++ b/commit.h\n@@ -313,7 +313,23 @@ extern int interactive_add(int argc, const char **argv, const char *prefix, int\n extern int run_add_interactive(const char *revision, const char *patch_mode,\n \t\t\t       const struct pathspec *pathspec);\n \n-struct commit_list *reduce_heads(struct commit_list *heads);\n+/*\n+ * Takes a list of commits and returns a new list where those\n+ * have been removed that can be reached from other commits in\n+ * the list. It is useful for, e.g., reducing the commits\n+ * randomly thrown at the git-merge command and removing\n+ * redundant commits that the user shouldn't have given to it.\n+ *\n+ * This function destroys the STALE bit of the commit objects'\n+ * flags.\n+ */\n+extern struct commit_list *reduce_heads(struct commit_list *heads);\n+\n+/*\n+ * Like `reduce_heads()`, except it replaces the list. Use this\n+ * instead of `foo = reduce_heads(foo);` to avoid memory leaks.\n+ */\n+extern void reduce_heads_replace(struct commit_list **heads);\n \n struct commit_extra_header {\n \tstruct commit_extra_header *next;\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex 1a0da71a43..73c4ec2b08 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -1719,7 +1719,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n \t\t\t\tallow_fast_forward = 0;\n \t\t}\n \t\tif (allow_fast_forward)\n-\t\t\tparents = reduce_heads(parents);\n+\t\t\treduce_heads_replace(&parents);\n \t} else {\n \t\tif (!reflog_msg)\n \t\t\treflog_msg = (whence == FROM_CHERRY_PICK)\ndiff --git a/builtin/fmt-merge-msg.c b/builtin/fmt-merge-msg.c\nindex e99b5ddbf9..27a2361e91 100644\n--- a/builtin/fmt-merge-msg.c\n+++ b/builtin/fmt-merge-msg.c\n@@ -571,7 +571,7 @@ static void find_merge_parents(struct merge_parents *result,\n \thead_commit = lookup_commit(head);\n \tif (head_commit)\n \t\tcommit_list_insert(head_commit, &parents);\n-\tparents = reduce_heads(parents);\n+\treduce_heads_replace(&parents);\n \n \twhile (parents) {\n \t\tstruct commit *cmit = pop_commit(&parents);\ndiff --git a/builtin/merge-base.c b/builtin/merge-base.c\nindex e17835fabb..24f6c71935 100644\n--- a/builtin/merge-base.c\n+++ b/builtin/merge-base.c\n@@ -57,7 +57,7 @@ static int handle_independent(int count, const char **args)\n \tfor (i = count - 1; i >= 0; i--)\n \t\tcommit_list_insert(get_commit_reference(args[i]), &revs);\n \n-\trevs = reduce_heads(revs);\n+\treduce_heads_replace(&revs);\n \n \tif (!revs)\n \t\treturn 1;\n@@ -78,7 +78,9 @@ static int handle_octopus(int count, const char **args, int show_all)\n \tfor (i = count - 1; i >= 0; i--)\n \t\tcommit_list_insert(get_commit_reference(args[i]), &revs);\n \n-\tresult = reduce_heads(get_octopus_merge_bases(revs));\n+\tresult = get_octopus_merge_bases(revs);\n+\tfree_commit_list(revs);\n+\treduce_heads_replace(&result);\n \n \tif (!result)\n \t\treturn 1;\ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex 23c53a3082..5cccf21c30 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -981,6 +981,7 @@ static struct commit_list *reduce_parents(struct commit *head_commit,\n \n \t/* Find what parents to record by checking independent ones. */\n \tparents = reduce_heads(remoteheads);\n+\tfree_commit_list(remoteheads);\n \n \tremoteheads = NULL;\n \tremotes = &remoteheads;\ndiff --git a/builtin/pull.c b/builtin/pull.c\nindex 9b86e519b1..a5dd6426d5 100644\n--- a/builtin/pull.c\n+++ b/builtin/pull.c\n@@ -741,12 +741,15 @@ static int get_octopus_merge_base(struct object_id *merge_base,\n \tif (!is_null_oid(fork_point))\n \t\tcommit_list_insert(lookup_commit_reference(fork_point), &revs);\n \n-\tresult = reduce_heads(get_octopus_merge_bases(revs));\n+\tresult = get_octopus_merge_bases(revs);\n \tfree_commit_list(revs);\n+\treduce_heads_replace(&result);\n+\n \tif (!result)\n \t\treturn 1;\n \n \toidcpy(merge_base, &result->item->object.oid);\n+\tfree_commit_list(result);\n \treturn 0;\n }\n \ndiff --git a/commit.c b/commit.c\nindex d3150d6270..7ff102c0df 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -1083,6 +1083,13 @@ struct commit_list *reduce_heads(struct commit_list *heads)\n \treturn result;\n }\n \n+void reduce_heads_replace(struct commit_list **heads)\n+{\n+\tstruct commit_list *result = reduce_heads(*heads);\n+\tfree_commit_list(*heads);\n+\t*heads = result;\n+}\n+\n static const char gpg_sig_header[] = \"gpgsig\";\n static const int gpg_sig_header_len = sizeof(gpg_sig_header) - 1;\n \n-- \n2.15.0.415.gac1375d7e\n\n"},{"id":"332061","messageId":"xmqqzi7xlcha.fsf@gitster.mtv.corp.google.com","threadId":"47088","inReplyTo":"a5f5a259f4cbe3661eb1960e83e9bcce5080b580.1510083859.git.martin.agren@gmail.com","subject":"Re: [PATCH v3 1/2] builtin/merge-base: free commit lists","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-11-08T02:40:17Z","receivedAt":"2017-11-08T02:40:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Martin Ågren <martin.agren@gmail.com> writes:\n\n> In several functions, we iterate through a commit list by assigning\n> `result = result->next`. As a consequence, we lose the original pointer\n> and eventually leak the list.\n>\n> Rewrite the loops so that we keep the original pointers, then call\n> `free_commit_list()`. Various alternatives were considered:\n\nSo, the reader expects to see a list of alternatives that we\nconsidered but did not use in this solution to follow.\n\n>\n> 1) Use `UNLEAK(result)` before the loop. Simple change, but not very\n> pretty. These would definitely be new lows among our usages of UNLEAK.\n\nI am not sure if I agree with the judgment, but we did reject it, so\ndescribing it as a candidate is good.\n\n> 2) Use `pop_commit()` when looping. Slightly less simple change, but it\n> feels slightly preferable to first display the list, then free it.\n\nOK.\n\n> 3) As in this patch, but with `UNLEAK()` instead of freeing. We'd still\n> go through all the trouble of refactoring the loop, and because it's not\n> super-obvious that we're about to exit, let's just free the lists -- it\n> probably doesn't affect the runtime much.\n\nThat does include what we did, too.  A rejected alternative is only\nthe first 3/4 of what this says.\n\n    3) As in this patch, but with `UNLEAK()` instead of freeing. We'd\n       still go through all the trouble of refactoring the loop, and the\n       use of UNLEAK() is left questionable because it's not very obvious\n       that we're about to exit.\n\nand then you can begin a separate paragraph, after the above lists,\ne.g.\n\n    Let's just free the lists -- it probably doesn't affect the\n    runtime much.\n\n> In `handle_independent()` we can drop `result` while we're here and\n> reuse the `revs`-variable instead. That matches several other users of\n> `reduce_heads()`. The memory-leak that this hides will be addressed in\n> the next commit.\n\nThe patch text looks agreeable.\n\nThanks.\n"}]}