{"thread":{"id":"46791","subject":"[PATCH] diff-lib: clear `pending` object-array in `index_differs_from()`","startedAt":"2017-09-20T19:48:11Z","lastAt":"2017-10-01T15:04:07Z","messageCount":27,"participants":["Martin Ågren","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"328489","messageId":"1505936846-2195-2-git-send-email-martin.agren@gmail.com","threadId":"46791","inReplyTo":null,"subject":"[PATCH] diff-lib: clear `pending` object-array in `index_differs_from()`","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2017-09-20T19:47:24Z","receivedAt":"2017-09-20T19:48:11Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"Instead of conditionally freeing `rev.pending.objects`, just call\n`object_array_clear()` on `rev.pending`. This means we don't poke as\nmuch into the implementation, which is already a good thing, but also\nthat we free the individual entries as well, thereby fixing a\nmemory-leak.\n\nSigned-off-by: Martin Ågren <martin.agren@gmail.com>\n---\n diff-lib.c | 3 +--\n 1 file changed, 1 insertion(+), 2 deletions(-)\n\ndiff --git a/diff-lib.c b/diff-lib.c\nindex 2a52b07..4e0980c 100644\n--- a/diff-lib.c\n+++ b/diff-lib.c\n@@ -549,7 +549,6 @@ int index_differs_from(const char *def, int diff_flags,\n \trev.diffopt.flags |= diff_flags;\n \trev.diffopt.ita_invisible_in_index = ita_invisible_in_index;\n \trun_diff_index(&rev, 1);\n-\tif (rev.pending.alloc)\n-\t\tfree(rev.pending.objects);\n+\tobject_array_clear(&rev.pending);\n \treturn (DIFF_OPT_TST(&rev.diffopt, HAS_CHANGES) != 0);\n }\n-- \n2.14.1\n\n"},{"id":"328494","messageId":"20170920200229.bc4yniz6otng2zyz@sigill.intra.peff.net","threadId":"46791","inReplyTo":"1505936846-2195-2-git-send-email-martin.agren@gmail.com","subject":"Re: [PATCH] diff-lib: clear `pending` object-array in `index_differs_from()`","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-09-20T20:02:29Z","receivedAt":"2017-09-20T20:02:36Z","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:24PM +0200, Martin Ågren wrote:\n\n> Instead of conditionally freeing `rev.pending.objects`, just call\n> `object_array_clear()` on `rev.pending`. This means we don't poke as\n> much into the implementation, which is already a good thing, but also\n> that we free the individual entries as well, thereby fixing a\n> memory-leak.\n> \n> Signed-off-by: Martin Ågren <martin.agren@gmail.com>\n> ---\n>  diff-lib.c | 3 +--\n>  1 file changed, 1 insertion(+), 2 deletions(-)\n> \n> diff --git a/diff-lib.c b/diff-lib.c\n> index 2a52b07..4e0980c 100644\n> --- a/diff-lib.c\n> +++ b/diff-lib.c\n> @@ -549,7 +549,6 @@ int index_differs_from(const char *def, int diff_flags,\n>  \trev.diffopt.flags |= diff_flags;\n>  \trev.diffopt.ita_invisible_in_index = ita_invisible_in_index;\n>  \trun_diff_index(&rev, 1);\n> -\tif (rev.pending.alloc)\n> -\t\tfree(rev.pending.objects);\n> +\tobject_array_clear(&rev.pending);\n\nLooks good. A similar bug was the exact reason for adding the function\nin 46be82312. I did a grep for 'free.*\\.objects' to see if there were\nother cases.\n\nThere are some hits. E.g., the one in orphaned_commit_warning(). It does\nsomething funny with setting revs.leak_pending. But I _think_ after the\nwhole thing is done and we call free(refs.objects), that probably ought\nto be an object_array_clear().\n\nAs I suspect you're working your way through leak-checker results, I'm\nOK if you want to punt on digging into more cases for now and just fix\nones that the tool has identified as real leaks.\n\n-Peff\n"},{"id":"328532","messageId":"CAN0heSqVxBreaZN9x1XtX2OpeAoVZNzixr+vzyCWwe8UKtdtgQ@mail.gmail.com","threadId":"46791","inReplyTo":"20170920200229.bc4yniz6otng2zyz@sigill.intra.peff.net","subject":"Re: [PATCH] diff-lib: clear `pending` object-array in `index_differs_from()`","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2017-09-21T03:56:21Z","receivedAt":"2017-09-21T03:56:26Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On 20 September 2017 at 22:02, Jeff King <peff@peff.net> wrote:\n> On Wed, Sep 20, 2017 at 09:47:24PM +0200, Martin Ågren wrote:\n>\n>> Instead of conditionally freeing `rev.pending.objects`, just call\n>> `object_array_clear()` on `rev.pending`. This means we don't poke as\n>> much into the implementation, which is already a good thing, but also\n>> that we free the individual entries as well, thereby fixing a\n>> memory-leak.\n\n> Looks good. A similar bug was the exact reason for adding the function\n> in 46be82312. I did a grep for 'free.*\\.objects' to see if there were\n> other cases.\n\nAh. I grepped for \"pending.objects\", but didn't go more general than that.\n\n> There are some hits. E.g., the one in orphaned_commit_warning(). It does\n> something funny with setting revs.leak_pending. But I _think_ after the\n> whole thing is done and we call free(refs.objects), that probably ought\n> to be an object_array_clear().\n\nI'll look into that one in the evening.\n\n> As I suspect you're working your way through leak-checker results, I'm\n> OK if you want to punt on digging into more cases for now and just fix\n> ones that the tool has identified as real leaks.\n\nI am indeed going through ASan results. Your UNLEAK helps immensely!\n(I'm collecting UNLEAKs on the side. I see now that UNLEAK is in master,\nso I should probably submit those where I believe things are\n\"UNLEAK-complete\".)\n\nThanks for looking at this.\n\nMartin\n"},{"id":"328539","messageId":"20170921045200.bw2yezcwmlxvemwn@sigill.intra.peff.net","threadId":"46791","inReplyTo":"CAN0heSqVxBreaZN9x1XtX2OpeAoVZNzixr+vzyCWwe8UKtdtgQ@mail.gmail.com","subject":"Re: [PATCH] diff-lib: clear `pending` object-array in `index_differs_from()`","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-09-21T04:52:00Z","receivedAt":"2017-09-21T04:52:06Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Sep 21, 2017 at 05:56:21AM +0200, Martin Ågren wrote:\n\n> > Looks good. A similar bug was the exact reason for adding the function\n> > in 46be82312. I did a grep for 'free.*\\.objects' to see if there were\n> > other cases.\n> \n> Ah. I grepped for \"pending.objects\", but didn't go more general than that.\n\nI did at first, too. My optimism lessened when I ran the more general\ngrep. :)\n\n> > As I suspect you're working your way through leak-checker results, I'm\n> > OK if you want to punt on digging into more cases for now and just fix\n> > ones that the tool has identified as real leaks.\n> \n> I am indeed going through ASan results. Your UNLEAK helps immensely!\n> (I'm collecting UNLEAKs on the side. I see now that UNLEAK is in master,\n> so I should probably submit those where I believe things are\n> \"UNLEAK-complete\".)\n\nThanks! That's exactly what I'd hoped might happen when I started on\nUNLEAK.\n\n-Peff\n"},{"id":"328699","messageId":"cover.1506120291.git.martin.agren@gmail.com","threadId":"46791","inReplyTo":"20170920200229.bc4yniz6otng2zyz@sigill.intra.peff.net","subject":"[PATCH v2 0/6] reroll ma/plugleaks; more `object_array`-fixes","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2017-09-22T23:34:48Z","receivedAt":"2017-09-22T23:35:18Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On 20 September 2017 at 22:02, Jeff King <peff@peff.net> wrote:\n> On Wed, Sep 20, 2017 at 09:47:24PM +0200, Martin Ågren wrote:\n>\n>> Instead of conditionally freeing `rev.pending.objects`, just call\n>> `object_array_clear()` on `rev.pending`. This means we don't poke as\n>> much into the implementation, which is already a good thing, but also\n>> that we free the individual entries as well, thereby fixing a\n>> memory-leak.\n>\n> Looks good. A similar bug was the exact reason for adding the function\n> in 46be82312. I did a grep for 'free.*\\.objects' to see if there were\n> other cases.\n>\n> There are some hits. E.g., the one in orphaned_commit_warning(). It does\n> something funny with setting revs.leak_pending. But I _think_ after the\n> whole thing is done and we call free(refs.objects), that probably ought\n> to be an object_array_clear().\n\nLooking into this, I found various classes of leaks and oddities. I\nended up replacing this patch with four patches.\n\nSince Junio collected my \"independent\" patches into ma/leakplugs, this\nis a re-roll of that whole topic. I got the first two patches from\nJunio's tree. The only difference there is \"builtin/\" at the very start\nof the first commit message.\n\nThe third patch handles the places where we use `leak_pending`. The\nfourth addresses the rest of what your grep found and includes the\noriginal patch from this thread. While working on those, I felt that\nsome callers fiddle a bit too much with the internals of `object_array`.\nOne pattern leaks, another only looks like it. That resulted in the last\ntwo patches.\n\nSince ma/plugleaks is branched off maint, I did the same with this\nseries. It applies on master and next and has a minor conflict on pu\n(`handle_commit()` got a new argument).\n\nMartin\n\nMartin Ågren (6):\n  builtin/commit: fix memory leak in `prepare_index()`\n  commit: fix memory leak in `reduce_heads()`\n  leak_pending: use `object_array_clear()`, not `free()`\n  object_array: use `object_array_clear()`, not `free()`\n  object_array: add and use `object_array_pop()`\n  pack-bitmap[-write]: use `object_array_clear()`, don't leak\n\n bisect.c              |  3 ++-\n builtin/checkout.c    |  9 ++++++++-\n builtin/commit.c      | 15 ++++++++++-----\n builtin/fast-export.c |  3 +--\n builtin/fsck.c        |  7 +------\n builtin/reflog.c      |  6 +++---\n bundle.c              |  9 ++++++++-\n commit.c              |  1 +\n diff-lib.c            |  3 +--\n object.c              | 13 +++++++++++++\n object.h              |  7 +++++++\n pack-bitmap-write.c   |  4 +---\n pack-bitmap.c         | 10 +++-------\n revision.h            | 11 +++++++++++\n shallow.c             |  2 +-\n submodule.c           |  4 ++--\n upload-pack.c         |  2 +-\n 17 files changed, 74 insertions(+), 35 deletions(-)\n\n-- \n2.14.1.727.g9ddaf86\n\n"},{"id":"328700","messageId":"48b08b7c776220ae8bf1144e297f9ee304f18d27.1506120292.git.martin.agren@gmail.com","threadId":"46791","inReplyTo":"cover.1506120291.git.martin.agren@gmail.com","subject":"[PATCH v2 1/6] builtin/commit: fix memory leak in `prepare_index()`","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2017-09-22T23:34:49Z","receivedAt":"2017-09-22T23:35:23Z","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>\nReviewed-by: Jeff King <peff@peff.net>\nSigned-off-by: Junio C Hamano <gitster@pobox.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 1a0da71a4..ce76b6f96 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -336,7 +336,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@@ -381,7 +381,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@@ -404,7 +405,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@@ -430,7 +432,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@@ -461,7 +464,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@@ -491,6 +493,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.727.g9ddaf86\n\n"},{"id":"328701","messageId":"75117f05084a342ec5dd53d613cfaf2fbfb13054.1506120292.git.martin.agren@gmail.com","threadId":"46791","inReplyTo":"cover.1506120291.git.martin.agren@gmail.com","subject":"[PATCH v2 2/6] commit: fix memory leak in `reduce_heads()`","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2017-09-22T23:34:50Z","receivedAt":"2017-09-22T23:35:28Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"We don't free the temporary scratch space we use with\n`remove_redundant()`. Free it similar to how we do it in\n`get_merge_bases_many_0()`.\n\nSigned-off-by: Martin Ågren <martin.agren@gmail.com>\nReviewed-by: Jeff King <peff@peff.net>\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n commit.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/commit.c b/commit.c\nindex d3150d627..f73976bcc 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -1080,6 +1080,7 @@ struct commit_list *reduce_heads(struct commit_list *heads)\n \tnum_head = remove_redundant(array, num_head);\n \tfor (i = 0; i < num_head; i++)\n \t\ttail = &commit_list_insert(array[i], tail)->next;\n+\tfree(array);\n \treturn result;\n }\n \n-- \n2.14.1.727.g9ddaf86\n\n"},{"id":"328702","messageId":"d4df674923b0dbc5c2e232cdc56a75205653d275.1506120292.git.martin.agren@gmail.com","threadId":"46791","inReplyTo":"cover.1506120291.git.martin.agren@gmail.com","subject":"[PATCH v2 3/6] leak_pending: use `object_array_clear()`, not `free()`","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2017-09-22T23:34:51Z","receivedAt":"2017-09-22T23:35:32Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"Setting `leak_pending = 1` tells `prepare_revision_walk()` not to\nrelease the `pending` array, and makes that the caller's responsibility.\nSee 4a43d374f (revision: add leak_pending flag, 2011-10-01) and\n353f5657a (bisect: use leak_pending flag, 2011-10-01).\n\nCommit 1da1e07c8 (clean up name allocation in prepare_revision_walk,\n2014-10-15) fixed a memory leak in `prepare_revision_walk()` by\nswitching from `free()` to `object_array_clear()`. However, where we use\nthe `leak_pending`-mechanism, we're still only calling `free()`.\n\nUse `object_array_clear()` instead. Copy some helpful comments from\n353f5657a to the other callers that we update to clarify the memory\nresponsibilities, and to highlight that the commits are not affected\nwhen we clear the array -- it is indeed correct to both tidy up the\ncommit flags and clear the object array.\n\nDocument `leak_pending` in revision.h to help future users get this\nright.\n\nSigned-off-by: Martin Ågren <martin.agren@gmail.com>\n---\n bisect.c           |  3 ++-\n builtin/checkout.c |  9 ++++++++-\n bundle.c           |  9 ++++++++-\n revision.h         | 11 +++++++++++\n 4 files changed, 29 insertions(+), 3 deletions(-)\n\ndiff --git a/bisect.c b/bisect.c\nindex a9fd9fbc6..fc797f6ae 100644\n--- a/bisect.c\n+++ b/bisect.c\n@@ -826,7 +826,8 @@ static int check_ancestors(const char *prefix)\n \n \t/* Clean up objects used, as they will be reused. */\n \tclear_commit_marks_for_object_array(&pending_copy, ALL_REV_FLAGS);\n-\tfree(pending_copy.objects);\n+\n+\tobject_array_clear(&pending_copy);\n \n \treturn res;\n }\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex 2d75ac66c..52f1b6770 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -796,9 +796,14 @@ static void orphaned_commit_warning(struct commit *old, struct commit *new)\n \tfor_each_ref(add_pending_uninteresting_ref, &revs);\n \tadd_pending_oid(&revs, \"HEAD\", &new->object.oid, UNINTERESTING);\n \n+\t/* Save pending objects, so they can be cleaned up later. */\n \trefs = revs.pending;\n \trevs.leak_pending = 1;\n \n+\t/*\n+\t * prepare_revision_walk (together with .leak_pending = 1) makes us\n+\t * the sole owner of the list of pending objects.\n+\t */\n \tif (prepare_revision_walk(&revs))\n \t\tdie(_(\"internal error in revision walk\"));\n \tif (!(old->object.flags & UNINTERESTING))\n@@ -806,8 +811,10 @@ static void orphaned_commit_warning(struct commit *old, struct commit *new)\n \telse\n \t\tdescribe_detached_head(_(\"Previous HEAD position was\"), old);\n \n+\t/* Clean up objects used, as they will be reused. */\n \tclear_commit_marks_for_object_array(&refs, ALL_REV_FLAGS);\n-\tfree(refs.objects);\n+\n+\tobject_array_clear(&refs);\n }\n \n static int switch_branches(const struct checkout_opts *opts,\ndiff --git a/bundle.c b/bundle.c\nindex d15db03c8..c092d5d68 100644\n--- a/bundle.c\n+++ b/bundle.c\n@@ -157,9 +157,14 @@ int verify_bundle(struct bundle_header *header, int verbose)\n \treq_nr = revs.pending.nr;\n \tsetup_revisions(2, argv, &revs, NULL);\n \n+\t/* Save pending objects, so they can be cleaned up later. */\n \trefs = revs.pending;\n \trevs.leak_pending = 1;\n \n+\t/*\n+\t * prepare_revision_walk (together with .leak_pending = 1) makes us\n+\t * the sole owner of the list of pending objects.\n+\t */\n \tif (prepare_revision_walk(&revs))\n \t\tdie(_(\"revision walk setup failed\"));\n \n@@ -176,8 +181,10 @@ int verify_bundle(struct bundle_header *header, int verbose)\n \t\t\t\trefs.objects[i].name);\n \t\t}\n \n+\t/* Clean up objects used, as they will be reused. */\n \tclear_commit_marks_for_object_array(&refs, ALL_REV_FLAGS);\n-\tfree(refs.objects);\n+\n+\tobject_array_clear(&refs);\n \n \tif (verbose) {\n \t\tstruct ref_list *r;\ndiff --git a/revision.h b/revision.h\nindex bc18487d6..3162cc78e 100644\n--- a/revision.h\n+++ b/revision.h\n@@ -149,6 +149,17 @@ struct rev_info {\n \t\t\tdate_mode_explicit:1,\n \t\t\tpreserve_subject:1;\n \tunsigned int\tdisable_stdin:1;\n+\t/*\n+\t * Set `leak_pending` to prevent `prepare_revision_walk()` from clearing\n+\t * the array of pending objects (`pending`). It will still forget about\n+\t * the array and its entries, so they really are leaked. This can be\n+\t * useful if the `struct object_array` `pending` is copied before\n+\t * calling `prepare_revision_walk()`. By setting `leak_pending`, you\n+\t * effectively claim ownership of the old array, so you should most\n+\t * likely call `object_array_clear(&pending_copy)` once you are done.\n+\t * Observe that this is about ownership of the array and its entries,\n+\t * not the commits referenced by those entries.\n+\t */\n \tunsigned int\tleak_pending:1;\n \t/* --show-linear-break */\n \tunsigned int\ttrack_linear:1,\n-- \n2.14.1.727.g9ddaf86\n\n"},{"id":"328703","messageId":"f325af4048bc14d6194da169b02de7d18fff8471.1506120292.git.martin.agren@gmail.com","threadId":"46791","inReplyTo":"cover.1506120291.git.martin.agren@gmail.com","subject":"[PATCH v2 4/6] object_array: use `object_array_clear()`, not `free()`","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2017-09-22T23:34:52Z","receivedAt":"2017-09-22T23:35:36Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"Instead of freeing `foo.objects` for an object array `foo` (sometimes\nconditionally), call `object_array_clear(&foo)`. This means we don't\npoke as much into the implementation, which is already a good thing, but\nalso that we release the individual entries as well, thereby fixing at\nleast one memory-leak (in diff-lib.c).\n\nIf someone is holding on to a pointer to an element's `name` or `path`,\nthat is now a dangling pointer, i.e., we'd be turning an unpleasant\nsituation into an outright bug. To the best of my understanding no such\nlong-term pointers are being taken.\n\nThe way we handle `study` in builting/reflog.c still looks like it might\nleak. That will be addressed in the next commit.\n\nSigned-off-by: Martin Ågren <martin.agren@gmail.com>\n---\n builtin/reflog.c | 4 ++--\n diff-lib.c       | 3 +--\n submodule.c      | 4 ++--\n upload-pack.c    | 2 +-\n 4 files changed, 6 insertions(+), 7 deletions(-)\n\ndiff --git a/builtin/reflog.c b/builtin/reflog.c\nindex e237d927a..6b34f23e7 100644\n--- a/builtin/reflog.c\n+++ b/builtin/reflog.c\n@@ -182,8 +182,8 @@ static int commit_is_complete(struct commit *commit)\n \t\t\tfound.objects[i].item->flags |= SEEN;\n \t}\n \t/* free object arrays */\n-\tfree(study.objects);\n-\tfree(found.objects);\n+\tobject_array_clear(&study);\n+\tobject_array_clear(&found);\n \treturn !is_incomplete;\n }\n \ndiff --git a/diff-lib.c b/diff-lib.c\nindex 2a52b0795..4e0980caa 100644\n--- a/diff-lib.c\n+++ b/diff-lib.c\n@@ -549,7 +549,6 @@ int index_differs_from(const char *def, int diff_flags,\n \trev.diffopt.flags |= diff_flags;\n \trev.diffopt.ita_invisible_in_index = ita_invisible_in_index;\n \trun_diff_index(&rev, 1);\n-\tif (rev.pending.alloc)\n-\t\tfree(rev.pending.objects);\n+\tobject_array_clear(&rev.pending);\n \treturn (DIFF_OPT_TST(&rev.diffopt, HAS_CHANGES) != 0);\n }\ndiff --git a/submodule.c b/submodule.c\nindex 36f45f5a5..79fd01f7b 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -1728,7 +1728,7 @@ static int find_first_merges(struct object_array *result, const char *path,\n \t\t\tadd_object_array(merges.objects[i].item, NULL, result);\n \t}\n \n-\tfree(merges.objects);\n+\tobject_array_clear(&merges);\n \treturn result->nr;\n }\n \n@@ -1833,7 +1833,7 @@ int merge_submodule(struct object_id *result, const char *path,\n \t\t\tprint_commit((struct commit *) merges.objects[i].item);\n \t}\n \n-\tfree(merges.objects);\n+\tobject_array_clear(&merges);\n \treturn 0;\n }\n \ndiff --git a/upload-pack.c b/upload-pack.c\nindex 7efff2fbf..ec0eee8fc 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -888,7 +888,7 @@ static void receive_needs(void)\n \t\t}\n \n \tshallow_nr += shallows.nr;\n-\tfree(shallows.objects);\n+\tobject_array_clear(&shallows);\n }\n \n /* return non-zero if the ref is hidden, otherwise 0 */\n-- \n2.14.1.727.g9ddaf86\n\n"},{"id":"328704","messageId":"80eaae517f73f57137db6adfcaef2e8ce16576c1.1506120292.git.martin.agren@gmail.com","threadId":"46791","inReplyTo":"cover.1506120291.git.martin.agren@gmail.com","subject":"[PATCH v2 5/6] object_array: add and use `object_array_pop()`","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2017-09-22T23:34:53Z","receivedAt":"2017-09-22T23:35:38Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"In a couple of places, we pop objects off an object array `foo` by\ndecreasing `foo.nr`. We access `foo.nr` in many places, but most if not\nall other times we do so read-only, e.g., as we iterate over the array.\nBut when we change `foo.nr` behind the array's back, it feels a bit\nnasty and looks like it might leak memory.\n\nLeaks happen if the popped element has an allocated `name` or `path`.\nAt the moment, that is not the case. Still, 1) the object array might\ngain more fields that want to be freed, 2) a code path where we pop\nmight start using names or paths, 3) one of these code paths might be\ncopied to somewhere where we do, and 4) using a dedicated function for\npopping is conceptually cleaner.\n\nIntroduce and use `object_array_pop()` instead. Release memory in the\nnew function. Document that popping an object leaves the associated\nelements in limbo.\n\nThe converted places were identified by grepping for \"\\.nr\\>\" and\nlooking for \"--\".\n\nMake the new function return NULL on an empty array. This is consistent\nwith `pop_commit()` and allows the following:\n\n\twhile ((o = object_array_pop(&foo)) != NULL) {\n\t\t// do something\n\t}\n\nBut as noted above, we don't need to go out of our way to avoid reading\n`foo.nr`. This is probably more readable:\n\n\twhile (foo.nr) {\n\t\t... o = object_array_pop(&foo);\n\t\t// do something\n\t}\n\nThe name of `object_array_pop()` does not quite align with\n`add_object_array()`. That is unfortunate. On the other hand, it matches\n`object_array_clear()`. Arguably it's `add_...` that is the odd one out,\nsince it reads like it's used to \"add\" an \"object array\". For that\nreason, side with `object_array_clear()`.\n\nSigned-off-by: Martin Ågren <martin.agren@gmail.com>\n---\n builtin/fast-export.c |  3 +--\n builtin/fsck.c        |  7 +------\n builtin/reflog.c      |  2 +-\n object.c              | 13 +++++++++++++\n object.h              |  7 +++++++\n shallow.c             |  2 +-\n 6 files changed, 24 insertions(+), 10 deletions(-)\n\ndiff --git a/builtin/fast-export.c b/builtin/fast-export.c\nindex d412c0a8f..cff8d0fc5 100644\n--- a/builtin/fast-export.c\n+++ b/builtin/fast-export.c\n@@ -634,11 +634,10 @@ static void handle_tail(struct object_array *commits, struct rev_info *revs)\n {\n \tstruct commit *commit;\n \twhile (commits->nr) {\n-\t\tcommit = (struct commit *)commits->objects[commits->nr - 1].item;\n+\t\tcommit = (struct commit *)object_array_pop(commits);\n \t\tif (has_unshown_parent(commit))\n \t\t\treturn;\n \t\thandle_commit(commit, revs);\n-\t\tcommits->nr--;\n \t}\n }\n \ndiff --git a/builtin/fsck.c b/builtin/fsck.c\nindex d18244ab5..7d4ad0273 100644\n--- a/builtin/fsck.c\n+++ b/builtin/fsck.c\n@@ -181,12 +181,7 @@ static int traverse_reachable(void)\n \tif (show_progress)\n \t\tprogress = start_progress_delay(_(\"Checking connectivity\"), 0, 0, 2);\n \twhile (pending.nr) {\n-\t\tstruct object_array_entry *entry;\n-\t\tstruct object *obj;\n-\n-\t\tentry = pending.objects + --pending.nr;\n-\t\tobj = entry->item;\n-\t\tresult |= traverse_one_object(obj);\n+\t\tresult |= traverse_one_object(object_array_pop(&pending));\n \t\tdisplay_progress(progress, ++nr);\n \t}\n \tstop_progress(&progress);\ndiff --git a/builtin/reflog.c b/builtin/reflog.c\nindex 6b34f23e7..2067cca5b 100644\n--- a/builtin/reflog.c\n+++ b/builtin/reflog.c\n@@ -126,7 +126,7 @@ static int commit_is_complete(struct commit *commit)\n \t\tstruct commit *c;\n \t\tstruct commit_list *parent;\n \n-\t\tc = (struct commit *)study.objects[--study.nr].item;\n+\t\tc = (struct commit *)object_array_pop(&study);\n \t\tif (!c->object.parsed && !parse_object(&c->object.oid))\n \t\t\tc->object.flags |= INCOMPLETE;\n \ndiff --git a/object.c b/object.c\nindex 321d7e920..b9a4a0e50 100644\n--- a/object.c\n+++ b/object.c\n@@ -353,6 +353,19 @@ static void object_array_release_entry(struct object_array_entry *ent)\n \tfree(ent->path);\n }\n \n+struct object *object_array_pop(struct object_array *array)\n+{\n+\tstruct object *ret;\n+\n+\tif (!array->nr)\n+\t\treturn NULL;\n+\n+\tret = array->objects[array->nr - 1].item;\n+\tobject_array_release_entry(&array->objects[array->nr - 1]);\n+\tarray->nr--;\n+\treturn ret;\n+}\n+\n void object_array_filter(struct object_array *array,\n \t\t\t object_array_each_func_t want, void *cb_data)\n {\ndiff --git a/object.h b/object.h\nindex 0a419ba8d..b7629fe92 100644\n--- a/object.h\n+++ b/object.h\n@@ -115,6 +115,13 @@ int object_list_contains(struct object_list *list, struct object *obj);\n /* Object array handling .. */\n void add_object_array(struct object *obj, const char *name, struct object_array *array);\n void add_object_array_with_path(struct object *obj, const char *name, struct object_array *array, unsigned mode, const char *path);\n+/*\n+ * Returns NULL if the array is empty. Otherwise, returns the last object\n+ * after removing its entry from the array. Other resources associated\n+ * with that object are left in an unspecified state and should not be\n+ * examined.\n+ */\n+struct object *object_array_pop(struct object_array *array);\n \n typedef int (*object_array_each_func_t)(struct object_array_entry *, void *);\n \ndiff --git a/shallow.c b/shallow.c\nindex 54359d549..901ac9791 100644\n--- a/shallow.c\n+++ b/shallow.c\n@@ -99,7 +99,7 @@ struct commit_list *get_shallow_commits(struct object_array *heads, int depth,\n \t\t\t\tcur_depth = 0;\n \t\t\t} else {\n \t\t\t\tcommit = (struct commit *)\n-\t\t\t\t\tstack.objects[--stack.nr].item;\n+\t\t\t\t\tobject_array_pop(&stack);\n \t\t\t\tcur_depth = *(int *)commit->util;\n \t\t\t}\n \t\t}\n-- \n2.14.1.727.g9ddaf86\n\n"},{"id":"328705","messageId":"30407fada0ee65ca61e5cc52b1dafc708ab90883.1506120292.git.martin.agren@gmail.com","threadId":"46791","inReplyTo":"cover.1506120291.git.martin.agren@gmail.com","subject":"[PATCH v2 6/6] pack-bitmap[-write]: use `object_array_clear()`, don't leak","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2017-09-22T23:34:54Z","receivedAt":"2017-09-22T23:35:41Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"Instead of setting the fields of rev->pending to 0/NULL, thereby leaking\nmemory, call `object_array_clear(&rev->pending)`.\n\nIn pack-bitmap.c, we make copies of those fields as `pending_nr` and\n`pending_e`. We never update the aliases and the original fields never\nchange, so the aliases are not really needed and just make it harder\nthan necessary to understand the code. While we're here, remove the\naliases to make the code easier to follow.\n\nSigned-off-by: Martin Ågren <martin.agren@gmail.com>\n---\n pack-bitmap-write.c |  4 +---\n pack-bitmap.c       | 10 +++-------\n 2 files changed, 4 insertions(+), 10 deletions(-)\n\ndiff --git a/pack-bitmap-write.c b/pack-bitmap-write.c\nindex 8e47a96b3..a8df5ce2a 100644\n--- a/pack-bitmap-write.c\n+++ b/pack-bitmap-write.c\n@@ -297,9 +297,7 @@ void bitmap_writer_build(struct packing_data *to_pack)\n \n \t\t\ttraverse_commit_list(&revs, show_commit, show_object, base);\n \n-\t\t\trevs.pending.nr = 0;\n-\t\t\trevs.pending.alloc = 0;\n-\t\t\trevs.pending.objects = NULL;\n+\t\t\tobject_array_clear(&revs.pending);\n \n \t\t\tstored->bitmap = bitmap_to_ewah(base);\n \t\t\tneed_reset = 0;\ndiff --git a/pack-bitmap.c b/pack-bitmap.c\nindex 327634cd7..0a49c1595 100644\n--- a/pack-bitmap.c\n+++ b/pack-bitmap.c\n@@ -653,8 +653,6 @@ static int in_bitmapped_pack(struct object_list *roots)\n int prepare_bitmap_walk(struct rev_info *revs)\n {\n \tunsigned int i;\n-\tunsigned int pending_nr = revs->pending.nr;\n-\tstruct object_array_entry *pending_e = revs->pending.objects;\n \n \tstruct object_list *wants = NULL;\n \tstruct object_list *haves = NULL;\n@@ -669,8 +667,8 @@ int prepare_bitmap_walk(struct rev_info *revs)\n \t\t\treturn -1;\n \t}\n \n-\tfor (i = 0; i < pending_nr; ++i) {\n-\t\tstruct object *object = pending_e[i].item;\n+\tfor (i = 0; i < revs->pending.nr; ++i) {\n+\t\tstruct object *object = revs->pending.objects[i].item;\n \n \t\tif (object->type == OBJ_NONE)\n \t\t\tparse_object_or_die(&object->oid, NULL);\n@@ -714,9 +712,7 @@ int prepare_bitmap_walk(struct rev_info *revs)\n \tif (!bitmap_git.loaded && load_pack_bitmap() < 0)\n \t\treturn -1;\n \n-\trevs->pending.nr = 0;\n-\trevs->pending.alloc = 0;\n-\trevs->pending.objects = NULL;\n+\tobject_array_clear(&revs->pending);\n \n \tif (haves) {\n \t\trevs->ignore_missing_links = 1;\n-- \n2.14.1.727.g9ddaf86\n\n"},{"id":"328711","messageId":"20170923034717.i65irpbmlxtzwo3h@sigill.intra.peff.net","threadId":"46791","inReplyTo":"d4df674923b0dbc5c2e232cdc56a75205653d275.1506120292.git.martin.agren@gmail.com","subject":"Re: [PATCH v2 3/6] leak_pending: use `object_array_clear()`, not `free()`","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-09-23T03:47:17Z","receivedAt":"2017-09-23T03:47:24Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Sep 23, 2017 at 01:34:51AM +0200, Martin Ågren wrote:\n\n> Setting `leak_pending = 1` tells `prepare_revision_walk()` not to\n> release the `pending` array, and makes that the caller's responsibility.\n> See 4a43d374f (revision: add leak_pending flag, 2011-10-01) and\n> 353f5657a (bisect: use leak_pending flag, 2011-10-01).\n> \n> Commit 1da1e07c8 (clean up name allocation in prepare_revision_walk,\n> 2014-10-15) fixed a memory leak in `prepare_revision_walk()` by\n> switching from `free()` to `object_array_clear()`. However, where we use\n> the `leak_pending`-mechanism, we're still only calling `free()`.\n\nThanks for digging up the history here. Reviewing those it seems clear\nthat doing a full object_array_clear() is the right thing.\n\n> Use `object_array_clear()` instead. Copy some helpful comments from\n> 353f5657a to the other callers that we update to clarify the memory\n> responsibilities, and to highlight that the commits are not affected\n> when we clear the array -- it is indeed correct to both tidy up the\n> commit flags and clear the object array.\n> \n> Document `leak_pending` in revision.h to help future users get this\n> right.\n\nThis all looks good to me. Since the main use of \"leak_pending\" seems to\nbe to clear commit marks, I have a feeling that a better API would have\nbeen something like:\n\n  /* normal setup and use */\n  init_revisions(&rev);\n  setup_revisions(&rev, ...);\n  prepare_revision_walk(&rev);\n  while (get_revision(&rev))\n\t...;\n\n  /* optional, but sensible if there may be other callers */\n  clear_commit_marks(&rev);\n\n  /* actually free any held memory */\n  clear_revisions(&rev);\n\nwhich would imply rev_info holding onto the \"old\" pending list\nautomatically until we decommission the rev_info in the final step.\n\nBut that's obviously a much bigger change. What you have here looks like\na nice clean fix to the leak problems.\n\n-Peff\n"},{"id":"328712","messageId":"20170923040406.d6sw52ov2amjzkl4@sigill.intra.peff.net","threadId":"46791","inReplyTo":"f325af4048bc14d6194da169b02de7d18fff8471.1506120292.git.martin.agren@gmail.com","subject":"Re: [PATCH v2 4/6] object_array: use `object_array_clear()`, not `free()`","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-09-23T04:04:06Z","receivedAt":"2017-09-23T04:04:15Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Sep 23, 2017 at 01:34:52AM +0200, Martin Ågren wrote:\n\n> Instead of freeing `foo.objects` for an object array `foo` (sometimes\n> conditionally), call `object_array_clear(&foo)`. This means we don't\n> poke as much into the implementation, which is already a good thing, but\n> also that we release the individual entries as well, thereby fixing at\n> least one memory-leak (in diff-lib.c).\n> \n> If someone is holding on to a pointer to an element's `name` or `path`,\n> that is now a dangling pointer, i.e., we'd be turning an unpleasant\n> situation into an outright bug. To the best of my understanding no such\n> long-term pointers are being taken.\n\nIt would be nice if we had a good way to verify this automatically, but\nI couldn't think of one. It passes the test suite with ASan, but of\ncourse our coverage is not 100%.\n\nWe do know a few things:\n\n  1. Any caller which keeps a pointer to the object-entries themselves\n     is already broken, since we free that already. We only have to care\n     about the name and path strings.\n\n  2. Any caller which uses add_object_array() has a NULL path, so we\n     don't have to worry about leaving that dangling.\n\n  3. Most of the callers outside of revision.c use a NULL name argument,\n     as well.\n\nSo let's see:\n\n> diff --git a/builtin/reflog.c b/builtin/reflog.c\n> index e237d927a..6b34f23e7 100644\n> --- a/builtin/reflog.c\n> +++ b/builtin/reflog.c\n> @@ -182,8 +182,8 @@ static int commit_is_complete(struct commit *commit)\n>  \t\t\tfound.objects[i].item->flags |= SEEN;\n>  \t}\n>  \t/* free object arrays */\n> -\tfree(study.objects);\n> -\tfree(found.objects);\n> +\tobject_array_clear(&study);\n> +\tobject_array_clear(&found);\n>  \treturn !is_incomplete;\n>  }\n\nThese ones always have NULL names and paths, so are good.\n\n(An orthogonal low-hanging fruit: these probably ought to use\nOBJECT_ARRAY_INIT instead of memset. Maybe a good #leftoverbits for some\nof the Outreachy applicants).\n\n> diff --git a/diff-lib.c b/diff-lib.c\n> index 2a52b0795..4e0980caa 100644\n> --- a/diff-lib.c\n> +++ b/diff-lib.c\n> @@ -549,7 +549,6 @@ int index_differs_from(const char *def, int diff_flags,\n>  \trev.diffopt.flags |= diff_flags;\n>  \trev.diffopt.ita_invisible_in_index = ita_invisible_in_index;\n>  \trun_diff_index(&rev, 1);\n> -\tif (rev.pending.alloc)\n> -\t\tfree(rev.pending.objects);\n> +\tobject_array_clear(&rev.pending);\n>  \treturn (DIFF_OPT_TST(&rev.diffopt, HAS_CHANGES) != 0);\n>  }\n\nThis one gets the entries from setup_revisions(), so they may have\nactual names. It calls diff_flush() before the clear, though, so I'm\npretty sure we would have dropped any queued entries (and I'm also\npretty sure that the queued entries make their own copies of any names\nanyway). So this one isn't trivial, but I think it's fine.\n\n> diff --git a/submodule.c b/submodule.c\n> index 36f45f5a5..79fd01f7b 100644\n> --- a/submodule.c\n> +++ b/submodule.c\n> @@ -1728,7 +1728,7 @@ static int find_first_merges(struct object_array *result, const char *path,\n>  \t\t\tadd_object_array(merges.objects[i].item, NULL, result);\n>  \t}\n>  \n> -\tfree(merges.objects);\n> +\tobject_array_clear(&merges);\n>  \treturn result->nr;\n>  }\n\nAnother always-NULL case.\n\n> @@ -1833,7 +1833,7 @@ int merge_submodule(struct object_id *result, const char *path,\n>  \t\t\tprint_commit((struct commit *) merges.objects[i].item);\n>  \t}\n>  \n> -\tfree(merges.objects);\n> +\tobject_array_clear(&merges);\n>  \treturn 0;\n>  }\n\nThis one is fed by the \"result\" array of find_first_merges(), which is\nalso always-NULL.\n\n> diff --git a/upload-pack.c b/upload-pack.c\n> index 7efff2fbf..ec0eee8fc 100644\n> --- a/upload-pack.c\n> +++ b/upload-pack.c\n> @@ -888,7 +888,7 @@ static void receive_needs(void)\n>  \t\t}\n>  \n>  \tshallow_nr += shallows.nr;\n> -\tfree(shallows.objects);\n> +\tobject_array_clear(&shallows);\n>  }\n\nAlso always NULL.\n\nSo I think all of these cases are good (and weren't actually leaking in\nthe first place, since the only thing to get rid of was the entries\nthemselves).\n\n> The way we handle `study` in builting/reflog.c still looks like it might\n> leak. That will be addressed in the next commit.\n\nThis confused me for a minute, since the leak is not visible in the\ncontext. ;) But reading the next commit, it makes sense.\n\n-Peff\n"},{"id":"328713","messageId":"20170923042757.ozl4qnmrsnd64mfc@sigill.intra.peff.net","threadId":"46791","inReplyTo":"80eaae517f73f57137db6adfcaef2e8ce16576c1.1506120292.git.martin.agren@gmail.com","subject":"Re: [PATCH v2 5/6] object_array: add and use `object_array_pop()`","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-09-23T04:27:57Z","receivedAt":"2017-09-23T04:28:04Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Sep 23, 2017 at 01:34:53AM +0200, Martin Ågren wrote:\n\n> In a couple of places, we pop objects off an object array `foo` by\n> decreasing `foo.nr`. We access `foo.nr` in many places, but most if not\n> all other times we do so read-only, e.g., as we iterate over the array.\n> But when we change `foo.nr` behind the array's back, it feels a bit\n> nasty and looks like it might leak memory.\n> \n> Leaks happen if the popped element has an allocated `name` or `path`.\n> At the moment, that is not the case. Still, 1) the object array might\n> gain more fields that want to be freed, 2) a code path where we pop\n> might start using names or paths, 3) one of these code paths might be\n> copied to somewhere where we do, and 4) using a dedicated function for\n> popping is conceptually cleaner.\n\nAll good reasons, I think.\n\n> Introduce and use `object_array_pop()` instead. Release memory in the\n> new function. Document that popping an object leaves the associated\n> elements in limbo.\n\nThe interface looks appropriate for all of the current cases. Though I\ndo suspect there's a bit of catch-22 here. If a caller _did_ care about\nthe \"name\" and \"path\" fields, then this pop function would be\ninappropriate, because it returns only the object field.\n\nSo in the most general form, you'd want:\n\n  while (foo.nr) {\n\t  struct object_array_entry *e = object_array_pop(&foo);\n\n\t  ... do stuff with e->name, etc ...\n\n\t  object_array_release_entry(e);\n  }\n\nBut that is certainly more cumbersome for these callers. I think we can\npunt on that until it becomes necessary (which likely is never).\n\n> Make the new function return NULL on an empty array. This is consistent\n> with `pop_commit()` and allows the following:\n> \n> \twhile ((o = object_array_pop(&foo)) != NULL) {\n> \t\t// do something\n> \t}\n> \n> But as noted above, we don't need to go out of our way to avoid reading\n> `foo.nr`. This is probably more readable:\n> \n> \twhile (foo.nr) {\n> \t\t... o = object_array_pop(&foo);\n> \t\t// do something\n> \t}\n\nAgreed that the latter is more readable (though I am also happy that the\npop function does something sensible for an empty array).\n\n> The name of `object_array_pop()` does not quite align with\n> `add_object_array()`. That is unfortunate. On the other hand, it matches\n> `object_array_clear()`. Arguably it's `add_...` that is the odd one out,\n> since it reads like it's used to \"add\" an \"object array\". For that\n> reason, side with `object_array_clear()`.\n\nYes, we're dreadfully inconsistent here. I tend to prefer noun_verb()\nwhen \"noun\" is a struct we're operating on. But we have quite a bit of\nverb_noun(). I find that noun_verb() is a bit more discoverable (e.g.,\ntab completion does something sensible), but I'm not sure if it's worth\ntrying to do a mass-conversion.\n\nPerhaps it's something that should be mentioned in CodingGuidelines, if\nit isn't already.\n\n> Signed-off-by: Martin Ågren <martin.agren@gmail.com>\n> ---\n>  builtin/fast-export.c |  3 +--\n>  builtin/fsck.c        |  7 +------\n>  builtin/reflog.c      |  2 +-\n>  object.c              | 13 +++++++++++++\n>  object.h              |  7 +++++++\n>  shallow.c             |  2 +-\n>  6 files changed, 24 insertions(+), 10 deletions(-)\n\nThe patch itself looks good, with one tiny nit:\n\n> diff --git a/object.h b/object.h\n> index 0a419ba8d..b7629fe92 100644\n> --- a/object.h\n> +++ b/object.h\n> @@ -115,6 +115,13 @@ int object_list_contains(struct object_list *list, struct object *obj);\n>  /* Object array handling .. */\n>  void add_object_array(struct object *obj, const char *name, struct object_array *array);\n>  void add_object_array_with_path(struct object *obj, const char *name, struct object_array *array, unsigned mode, const char *path);\n> +/*\n> + * Returns NULL if the array is empty. Otherwise, returns the last object\n> + * after removing its entry from the array. Other resources associated\n> + * with that object are left in an unspecified state and should not be\n> + * examined.\n> + */\n> +struct object *object_array_pop(struct object_array *array);\n\nI'm very happy to see a comment over the declaration here. But I think\nit's a bit more readable if we put a blank line between the prior\nfunction and the start of that comment.\n\n-Peff\n"},{"id":"328714","messageId":"20170923043532.in6pcrf7roxhfrrq@sigill.intra.peff.net","threadId":"46791","inReplyTo":"30407fada0ee65ca61e5cc52b1dafc708ab90883.1506120292.git.martin.agren@gmail.com","subject":"Re: [PATCH v2 6/6] pack-bitmap[-write]: use `object_array_clear()`, don't leak","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-09-23T04:35:32Z","receivedAt":"2017-09-23T04:35:39Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Sep 23, 2017 at 01:34:54AM +0200, Martin Ågren wrote:\n\n> Instead of setting the fields of rev->pending to 0/NULL, thereby leaking\n> memory, call `object_array_clear(&rev->pending)`.\n\nI wondered if these cases might sometimes work on an uninitialized\nstruct (so setting the fields unconditionally without looking at them\nwould be the right thing). But nope, they are always initialized by the\nrevision machinery. So freeing is the right thing.\n\nLike the earlier patch, we'd want to be sure there are no dangling\npointers here to the name/path fields. But I looked over the code and I\nfeel confident there are not (famous last words...).\n\n> In pack-bitmap.c, we make copies of those fields as `pending_nr` and\n> `pending_e`. We never update the aliases and the original fields never\n> change, so the aliases are not really needed and just make it harder\n> than necessary to understand the code. While we're here, remove the\n> aliases to make the code easier to follow.\n\nAgreed. Sometimes aliasing like this helps readability when the original\nvariable is wordy to access. But I don't think that is the case here.\n\nIt _can_ also allow the compiler to optimize harder (e.g., by putting\n\"pending_nr\" into a register, because it's not 100% sure that\nrevs->pending.nr is unchanged by function calls within the loop). But\nunless we know something is a tight loop, I don't think that level of\nmicro-optimization is worth the readability hit.\n\n> Signed-off-by: Martin Ågren <martin.agren@gmail.com>\n> ---\n>  pack-bitmap-write.c |  4 +---\n>  pack-bitmap.c       | 10 +++-------\n>  2 files changed, 4 insertions(+), 10 deletions(-)\n\nPatch itself is as advertised. Looks good.\n\n-Peff\n"},{"id":"328715","messageId":"20170923043701.4s3xuytp5hdjwmsy@sigill.intra.peff.net","threadId":"46791","inReplyTo":"cover.1506120291.git.martin.agren@gmail.com","subject":"Re: [PATCH v2 0/6] reroll ma/plugleaks; more `object_array`-fixes","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-09-23T04:37:01Z","receivedAt":"2017-09-23T04:37:07Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Sep 23, 2017 at 01:34:48AM +0200, Martin Ågren wrote:\n\n> Martin Ågren (6):\n>   builtin/commit: fix memory leak in `prepare_index()`\n>   commit: fix memory leak in `reduce_heads()`\n>   leak_pending: use `object_array_clear()`, not `free()`\n>   object_array: use `object_array_clear()`, not `free()`\n>   object_array: add and use `object_array_pop()`\n>   pack-bitmap[-write]: use `object_array_clear()`, don't leak\n\nAll six look good to me. Thanks again for poking into this. I'm afraid\nto ask how far we have left to go on running the test suite all the way\nthrough with leak-checking turned on. :)\n\n-Peff\n"},{"id":"328722","messageId":"CAN0heSrZjK_fYpXDKfxuGwRYputy9f4GViy1UK+eHTj9ctVa7Q@mail.gmail.com","threadId":"46791","inReplyTo":"20170923040406.d6sw52ov2amjzkl4@sigill.intra.peff.net","subject":"Re: [PATCH v2 4/6] object_array: use `object_array_clear()`, not `free()`","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2017-09-23T09:41:57Z","receivedAt":"2017-09-23T09:42:05Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On 23 September 2017 at 06:04, Jeff King <peff@peff.net> wrote:\n> On Sat, Sep 23, 2017 at 01:34:52AM +0200, Martin Ågren wrote:\n>\n>> The way we handle `study` in builting/reflog.c still looks like it might\n>> leak. That will be addressed in the next commit.\n>\n> This confused me for a minute, since the leak is not visible in the\n> context. ;) But reading the next commit, it makes sense.\n\nHmm, I added this so that a careful reader would /not/ get confused and\ngo \"wait, doesn't this look a bit fishy?\". But I also didn't want to get\ninto too many details, so of course it ended up confusing anyway. :-)\n"},{"id":"328726","messageId":"CAN0heSpMqRHuQ98AC3Qne=0ygSHxFXQ4buLp0Lvtf19uUo8adQ@mail.gmail.com","threadId":"46791","inReplyTo":"20170923042757.ozl4qnmrsnd64mfc@sigill.intra.peff.net","subject":"Re: [PATCH v2 5/6] object_array: add and use `object_array_pop()`","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2017-09-23T09:49:16Z","receivedAt":"2017-09-23T09:49:22Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On 23 September 2017 at 06:27, Jeff King <peff@peff.net> wrote:\n> On Sat, Sep 23, 2017 at 01:34:53AM +0200, Martin Ågren wrote:\n>\n>> Introduce and use `object_array_pop()` instead. Release memory in the\n>> new function. Document that popping an object leaves the associated\n>> elements in limbo.\n>\n> The interface looks appropriate for all of the current cases. Though I\n> do suspect there's a bit of catch-22 here. If a caller _did_ care about\n> the \"name\" and \"path\" fields, then this pop function would be\n> inappropriate, because it returns only the object field.\n>\n> So in the most general form, you'd want:\n>\n>   while (foo.nr) {\n>           struct object_array_entry *e = object_array_pop(&foo);\n>\n>           ... do stuff with e->name, etc ...\n>\n>           object_array_release_entry(e);\n>   }\n>\n> But that is certainly more cumbersome for these callers. I think we can\n> punt on that until it becomes necessary (which likely is never).\n\nI considered something like this. I felt that making the function\n`object_array_release_entr()` available to the general public would also\ncarry some risk. Right now, it's only object.c that needs to get things\nright (never release twice, never release without removing, never be\nclever, ..)\n\n>> The name of `object_array_pop()` does not quite align with\n>> `add_object_array()`. That is unfortunate. On the other hand, it matches\n>> `object_array_clear()`. Arguably it's `add_...` that is the odd one out,\n>> since it reads like it's used to \"add\" an \"object array\". For that\n>> reason, side with `object_array_clear()`.\n>\n> Yes, we're dreadfully inconsistent here. I tend to prefer noun_verb()\n> when \"noun\" is a struct we're operating on. But we have quite a bit of\n> verb_noun(). I find that noun_verb() is a bit more discoverable (e.g.,\n> tab completion does something sensible), but I'm not sure if it's worth\n> trying to do a mass-conversion.\n>\n> Perhaps it's something that should be mentioned in CodingGuidelines, if\n> it isn't already.\n\nWhen writing the above paragaph (so yes, after I had already chosen the\nname), I tried to find something, but couldn't. Admittedly, I just had a\ncursory look.\n\n> The patch itself looks good, with one tiny nit:\n>\n>> diff --git a/object.h b/object.h\n>> index 0a419ba8d..b7629fe92 100644\n>> --- a/object.h\n>> +++ b/object.h\n>> @@ -115,6 +115,13 @@ int object_list_contains(struct object_list *list, struct object *obj);\n>>  /* Object array handling .. */\n>>  void add_object_array(struct object *obj, const char *name, struct object_array *array);\n>>  void add_object_array_with_path(struct object *obj, const char *name, struct object_array *array, unsigned mode, const char *path);\n>> +/*\n>> + * Returns NULL if the array is empty. Otherwise, returns the last object\n>> + * after removing its entry from the array. Other resources associated\n>> + * with that object are left in an unspecified state and should not be\n>> + * examined.\n>> + */\n>> +struct object *object_array_pop(struct object_array *array);\n>\n> I'm very happy to see a comment over the declaration here. But I think\n> it's a bit more readable if we put a blank line between the prior\n> function and the start of that comment.\n\nYes, that looks strange. :( I could re-roll and/or ask Junio to fiddle\nwith it. On closer look, this file is pretty close to documenting all\nfunctions and there are some other comment-formatting issues. So here's\na promise that I'll get back to clean this up more generally in the not\ntoo distant future. Would that be an acceptable punt? :-?\n\nMartin\n"},{"id":"328727","messageId":"CAN0heSrPBDi4q9fqr=qoTbjRHpFS_VEL0O8=MFFJ4DLCuJwahA@mail.gmail.com","threadId":"46791","inReplyTo":"20170923043701.4s3xuytp5hdjwmsy@sigill.intra.peff.net","subject":"Re: [PATCH v2 0/6] reroll ma/plugleaks; more `object_array`-fixes","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2017-09-23T09:54:31Z","receivedAt":"2017-09-23T09:54:37Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On 23 September 2017 at 06:37, Jeff King <peff@peff.net> wrote:\n> On Sat, Sep 23, 2017 at 01:34:48AM +0200, Martin Ågren wrote:\n>\n>> Martin Ågren (6):\n>>   builtin/commit: fix memory leak in `prepare_index()`\n>>   commit: fix memory leak in `reduce_heads()`\n>>   leak_pending: use `object_array_clear()`, not `free()`\n>>   object_array: use `object_array_clear()`, not `free()`\n>>   object_array: add and use `object_array_pop()`\n>>   pack-bitmap[-write]: use `object_array_clear()`, don't leak\n>\n> All six look good to me. Thanks again for poking into this. I'm afraid\n> to ask how far we have left to go on running the test suite all the way\n> through with leak-checking turned on. :)\n\nThanks for reviewing.\n\nUnfortunately, I have not figured out how to get LSan to simply report\nthe leaks and continue. Its default behavior is to abort if there are\nleaks. That's useful for finding the first leaking test, but not much\nelse. (Later tests might depend on that test doing everything it should,\nso now those later tests will start failing and/or executing different\ncode paths.)\n\nI can tell LeakSanitizer to exit with an exit code instead, but then all\nleaking git-processes exit with the same exit code. That also interferes\nwith the tests.\n\nWhat I would like is for the git-process to exit with the same exit\nstatus it would have had without the leak-checking. That would make it\npossible to run the whole test suite, collect all leaks, identify\nduplicates, sort them, categorize them, prioritize them, track how we\nare progressing...\n\nI did spend some time looking into this before I gave up. I'd love to be\ntold I've missed something obvious.\n\nMartin\n"},{"id":"328732","messageId":"20170923154702.ltyg6ctqlcup6ars@sigill.intra.peff.net","threadId":"46791","inReplyTo":"CAN0heSpMqRHuQ98AC3Qne=0ygSHxFXQ4buLp0Lvtf19uUo8adQ@mail.gmail.com","subject":"Re: [PATCH v2 5/6] object_array: add and use `object_array_pop()`","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-09-23T15:47:02Z","receivedAt":"2017-09-23T15:47:10Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Sep 23, 2017 at 11:49:16AM +0200, Martin Ågren wrote:\n\n> >>  void add_object_array_with_path(struct object *obj, const char *name, struct object_array *array, unsigned mode, const char *path);\n> >> +/*\n> >> + * Returns NULL if the array is empty. Otherwise, returns the last object\n> >> + * after removing its entry from the array. Other resources associated\n> >> + * with that object are left in an unspecified state and should not be\n> >> + * examined.\n> >> + */\n> >> +struct object *object_array_pop(struct object_array *array);\n> >\n> > I'm very happy to see a comment over the declaration here. But I think\n> > it's a bit more readable if we put a blank line between the prior\n> > function and the start of that comment.\n> \n> Yes, that looks strange. :( I could re-roll and/or ask Junio to fiddle\n> with it. On closer look, this file is pretty close to documenting all\n> functions and there are some other comment-formatting issues. So here's\n> a promise that I'll get back to clean this up more generally in the not\n> too distant future. Would that be an acceptable punt? :-?\n\nYeah, I don't think it is a show-stopper (and I think there is a\nreasonable chance Junio will just mark it up as he applies).\n\n-Peff\n"},{"id":"328733","messageId":"20170923161316.yntnccqrhv5d3jza@sigill.intra.peff.net","threadId":"46791","inReplyTo":"CAN0heSrPBDi4q9fqr=qoTbjRHpFS_VEL0O8=MFFJ4DLCuJwahA@mail.gmail.com","subject":"Re: [PATCH v2 0/6] reroll ma/plugleaks; more `object_array`-fixes","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-09-23T16:13:16Z","receivedAt":"2017-09-23T16:13:48Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Sep 23, 2017 at 11:54:31AM +0200, Martin Ågren wrote:\n\n> Unfortunately, I have not figured out how to get LSan to simply report\n> the leaks and continue. Its default behavior is to abort if there are\n> leaks. That's useful for finding the first leaking test, but not much\n> else. (Later tests might depend on that test doing everything it should,\n> so now those later tests will start failing and/or executing different\n> code paths.)\n> \n> I can tell LeakSanitizer to exit with an exit code instead, but then all\n> leaking git-processes exit with the same exit code. That also interferes\n> with the tests.\n> \n> What I would like is for the git-process to exit with the same exit\n> status it would have had without the leak-checking. That would make it\n> possible to run the whole test suite, collect all leaks, identify\n> duplicates, sort them, categorize them, prioritize them, track how we\n> are progressing...\n\nDoesn't:\n\n  LSAN_OPTIONS=abort_on_error=0:exitcode=0\n\ndo what you want? I get:\n\n  $ export LSAN_OPTIONS=abort_on_error=0:exitcode=0\n\n  [no leaks, no error]\n  $ ./git rev-parse HEAD; echo $?\n  0\n\n  [no leaks, error]\n  $ ./git rev-parse broken; echo $?\n  ...\n  128\n\n  [leaks, no error]\n  $ ./git --no-pager show; echo $?\n  ...\n  SUMMARY: LeakSanitizer: 40 byte(s) leaked in 4 allocation(s).\n  0\n\n  [leaks, error]\n  $ ./git --no-pager show broken; echo $?\n  ...\n  SUMMARY: LeakSanitizer: 35 byte(s) leaked in 3 allocation(s).\n  128\n\nIn theory you should be able to just add \"log_path=/tmp/lsan/output\" to\nthat, which should put all the logs in a convenient place (and stop the\nextra output from confusing any tests which capture stderr). But I can't\nseem to get log_path to do anything, contrary to the documentation.\n\nDoing log_to_syslog=1 does work for me. I'm not sure if it's a bug or\nI'm holding it wrong. But it does seem like it should be possible to do\nwhat you want.\n\n-Peff\n"},{"id":"328734","messageId":"20170923163817.7ltmkav2ytk7n43k@sigill.intra.peff.net","threadId":"46791","inReplyTo":"20170923161316.yntnccqrhv5d3jza@sigill.intra.peff.net","subject":"Re: [PATCH v2 0/6] reroll ma/plugleaks; more `object_array`-fixes","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-09-23T16:38:18Z","receivedAt":"2017-09-23T16:38:25Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Sep 23, 2017 at 12:13:16PM -0400, Jeff King wrote:\n\n> In theory you should be able to just add \"log_path=/tmp/lsan/output\" to\n> that, which should put all the logs in a convenient place (and stop the\n> extra output from confusing any tests which capture stderr). But I can't\n> seem to get log_path to do anything, contrary to the documentation.\n> \n> Doing log_to_syslog=1 does work for me. I'm not sure if it's a bug or\n> I'm holding it wrong. But it does seem like it should be possible to do\n> what you want.\n\nHrm. log_path doesn't seem to work for me directly from LSAN_OPTIONS.\nBut if you compile with ASan, it _does_ work from there. That seems like\na bug from my reading of the documentation, but maybe I'm missing\nsomething.\n\nAnyway, doing:\n\n  ASAN_OPTIONS=detect_leaks=1:abort_on_error=0:exitcode=0:log_path=/tmp/lsan/output \\\n  make SANITIZE=address,leak test\n\nshould pass the whole suite and give you a host of files to analyze.\n\nI'm not sure of the best way to count things. Surely many of these are\nthe same leak over and over. One way to visualize is to look at the\nunique entries at each level of the call stack:\n\n  show() {\n    level=$1\n    perl -lne 'print $1 if /#'$level' .*? in (.*)/' * |\n    sort | uniq -c | sort -rn\n  }\n\nLevel 0 isn't that interesting, it's just malloc:\n\n  $ show 0\n    27217 realloc (/usr/lib/x86_64-linux-gnu/libasan.so.4+0xd9f20)\n    23297 __interceptor_strdup (/usr/lib/x86_64-linux-gnu/libasan.so.4+0x74490)\n    19370 malloc (/usr/lib/x86_64-linux-gnu/libasan.so.4+0xd9b70)\n    12105 __interceptor_calloc (/usr/lib/x86_64-linux-gnu/libasan.so.4+0xd9d48)\n\nAnd level 1 is just our allocators:\n\n  $ show 1\n    28321 xrealloc /home/peff/compile/git/wrapper.c:138\n    25199 xstrdup /home/peff/compile/git/wrapper.c:44\n    18856 do_xmalloc /home/peff/compile/git/wrapper.c:60\n    12664 xcalloc /home/peff/compile/git/wrapper.c:160\n      660 __getdelim (/lib/x86_64-linux-gnu/libc.so.6+0x682b7)\n\nBut level 2 starts to get interesting (and is too big to copy here).\nSome of them are direct sources of leaks. But others like strbuf_grow()\nare yet another level of indirection, and you'd want to jump to level 3\nor 4.\n\nIf you want the \"real\" source of the leak, probably you'd have to mark\ncertain functions as uninteresting and keep walking each stack trace\nuntil you find an interesting function to report.\n\n-Peff\n"},{"id":"328762","messageId":"xmqqh8vsh963.fsf@gitster.mtv.corp.google.com","threadId":"46791","inReplyTo":"cover.1506120291.git.martin.agren@gmail.com","subject":"Re: [PATCH v2 0/6] reroll ma/plugleaks; more `object_array`-fixes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-09-24T07:01:40Z","receivedAt":"2017-09-24T07:01:47Z","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 Junio collected my \"independent\" patches into ma/leakplugs, this\n> is a re-roll of that whole topic. I got the first two patches from\n> Junio's tree. The only difference there is \"builtin/\" at the very start\n> of the first commit message.\n\nThanks, all 6 look sensible.\n\n"},{"id":"328774","messageId":"CAN0heSqQ3Etj1mkGhuft8JzKA8SNhpF0jcnVEdFeKX5NHpyqTw@mail.gmail.com","threadId":"46791","inReplyTo":"20170923163817.7ltmkav2ytk7n43k@sigill.intra.peff.net","subject":"Re: [PATCH v2 0/6] reroll ma/plugleaks; more `object_array`-fixes","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2017-09-24T19:59:28Z","receivedAt":"2017-09-24T19:59:36Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On 23 September 2017 at 18:38, Jeff King <peff@peff.net> wrote:\n> On Sat, Sep 23, 2017 at 12:13:16PM -0400, Jeff King wrote:\n>\n>> In theory you should be able to just add \"log_path=/tmp/lsan/output\" to\n>> that, which should put all the logs in a convenient place (and stop the\n>> extra output from confusing any tests which capture stderr). But I can't\n>> seem to get log_path to do anything, contrary to the documentation.\n>>\n>> Doing log_to_syslog=1 does work for me. I'm not sure if it's a bug or\n>> I'm holding it wrong. But it does seem like it should be possible to do\n>> what you want.\n>\n> Hrm. log_path doesn't seem to work for me directly from LSAN_OPTIONS.\n> But if you compile with ASan, it _does_ work from there. That seems like\n> a bug from my reading of the documentation, but maybe I'm missing\n> something.\n>\n> Anyway, doing:\n>\n>   ASAN_OPTIONS=detect_leaks=1:abort_on_error=0:exitcode=0:log_path=/tmp/lsan/output \\\n>   make SANITIZE=address,leak test\n>\n> should pass the whole suite and give you a host of files to analyze.\n\nThanks. My reading of the documentation was off. Turns out exitcode=0\ndoes not set the exit code to 0, but rather turns it off. Duh. It also\ndidn't help that I tested with (my) gcc, which seems to have some\nissues. With clang, things work fine. Thanks for looking into this.\n\n> I'm not sure of the best way to count things.\n\nRight. It's a tricky problem. And in the end, all we find out is where\nwe allocate and how we got there. Exactly where we lose/leak that piece\nof allocated memory is another question...\n"},{"id":"328775","messageId":"CAN0heSqAi1ryRyXLqKV+pfj1eHCuqFRhu4o+a+222vSsQZwymg@mail.gmail.com","threadId":"46791","inReplyTo":"xmqqh8vsh963.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v2 0/6] reroll ma/plugleaks; more `object_array`-fixes","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2017-09-24T20:00:21Z","receivedAt":"2017-09-24T20:01:14Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On 24 September 2017 at 09:01, Junio C Hamano <gitster@pobox.com> wrote:\n> Martin Ågren <martin.agren@gmail.com> writes:\n>\n>> Since Junio collected my \"independent\" patches into ma/leakplugs, this\n>> is a re-roll of that whole topic. I got the first two patches from\n>> Junio's tree. The only difference there is \"builtin/\" at the very start\n>> of the first commit message.\n>\n> Thanks, all 6 look sensible.\n\nThanks for queuing and thanks for adding the empty line in patch 5.\n"},{"id":"328855","messageId":"20170925160835.aoomjaqrn2o2aosi@sigill.intra.peff.net","threadId":"46791","inReplyTo":"CAN0heSqQ3Etj1mkGhuft8JzKA8SNhpF0jcnVEdFeKX5NHpyqTw@mail.gmail.com","subject":"Re: [PATCH v2 0/6] reroll ma/plugleaks; more `object_array`-fixes","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-09-25T16:08:35Z","receivedAt":"2017-09-25T16:08:42Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Sep 24, 2017 at 09:59:28PM +0200, Martin Ågren wrote:\n\n> > Anyway, doing:\n> >\n> >   ASAN_OPTIONS=detect_leaks=1:abort_on_error=0:exitcode=0:log_path=/tmp/lsan/output \\\n> >   make SANITIZE=address,leak test\n> >\n> > should pass the whole suite and give you a host of files to analyze.\n> \n> Thanks. My reading of the documentation was off. Turns out exitcode=0\n> does not set the exit code to 0, but rather turns it off. Duh.\n\nActually, the docs are quite confusing. The LSan \"exitcode\" option\ndefaults to 23, and that is the exit code you get. So I think it\nprobably is interpreted as the code to exit with, or 0 for \"use the\noriginal exit code\".\n\n> > I'm not sure of the best way to count things.\n> \n> Right. It's a tricky problem. And in the end, all we find out is where\n> we allocate and how we got there. Exactly where we lose/leak that piece\n> of allocated memory is another question...\n\nFor hunting a particular trace, I think you have to walk up the list of\ncalled functions and see where the pointers go out of scope. I'm not\nsure how to make that easier (in theory a compiler-instrumentation like\nLSan could do it by performing a leak-check when pointers go out of\nscope. But it would also need to know about copies you've made of\npointers, so I imagine it would be extremely slow to run).\n\nBut at least on the topic of \"how many unique leaks are there\", I wrote\nthe script below to try to give some basic answers. It just finds the\nfirst non-boring entry in each stack trace and reports that. Where\n\"boring\" is really \"this function is not expected to free, but hands off\nmemory ownership to somebody else\".\n\nYou can use it to do:\n\n  perl leaks.pl /tmp/lsan/output.* | sort | uniq -c | sort -rn | head\n\nto see places that leak a lot. These are either boring calls that need\nto be annotated, or are high-value targets for de-leaking.\n\nI notice ref-filter.c has quite a few high entries on the list. I'm not\nsure yet which case it falls into. :)\n\nThe other interesting thing is seeing how many \"unique\" leaks there are:\n\n  perl leaks.pl /tmp/lsan/output.* | sort -u | wc -l\n\nI get a bit over 800 with a run of the test suite. Which is a lot, but\nfewer than I expected. And I'm sure quite a few of them are really\n\"duplicates\" that can be eliminated in chunks.\n\nSo I don't know how useful any of that will be, but it at least should\ngive _some_ metric that should be diminishing as we fix leaks.\n\n-- >8 --\n#!/usr/bin/perl\n\nmy $boring = join('|',\n  # These are allocation functions that get called from a lot of places.\n  qw(\n        __interceptor_strdup\n        __interceptor_calloc\n        realloc\n        malloc\n        xstrdup\n        xcalloc\n        strbuf_\n        xmemdupz\n        xstrvfmt\n        xstrfmt\n        xstrndup\n  ),\n  # These are really just the revision machinery not getting cleaned up;\n  # for many we'd probably want to just UNLEAK() at the apex caller\n  qw(\n        add_rev_cmdline\n        add_object_array_with_path\n        add_pending_object_with_path\n        add_pending_object_with_mode\n        add_pending_object\n        handle_revision_arg\n        setup_revisions\n  ),\n  # More allocators that drop memory ownership\n  qw(\n        alloc_ref_with_prefix\n        alloc_ref\n        copy_ref\n        commit_list_insert\n        copy_pathspec\n  ),\n);\nmy $boring_re = qr/^$boring/;\nmy $skipping;\n\nwhile (<>) {\n  if (/^\\s*#[0-9]+ 0x[0-9a-f]+ in (.*)/) {\n    next if $skipping; # we already reported this trace\n    next if $1 =~ $boring_re;\n\n    print $1, \"\\n\";\n    $skipping = 1;\n  } else {\n    $skipping = 0;\n  }\n}\n"},{"id":"329300","messageId":"CAN0heSrZVJpc-usGZZeyF7juKDZOvnS8m5p8pT4fuF1CYoQ9UA@mail.gmail.com","threadId":"46791","inReplyTo":"20170925160835.aoomjaqrn2o2aosi@sigill.intra.peff.net","subject":"Re: [PATCH v2 0/6] reroll ma/plugleaks; more `object_array`-fixes","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2017-10-01T15:04:00Z","receivedAt":"2017-10-01T15:04:07Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On 25 September 2017 at 18:08, Jeff King <peff@peff.net> wrote:\n> On Sun, Sep 24, 2017 at 09:59:28PM +0200, Martin Ågren wrote:\n>\n>> > I'm not sure of the best way to count things.\n>>\n>\n> But at least on the topic of \"how many unique leaks are there\", I wrote\n> the script below to try to give some basic answers. It just finds the\n> first non-boring entry in each stack trace and reports that. Where\n> \"boring\" is really \"this function is not expected to free, but hands off\n> memory ownership to somebody else\".\n\nThanks. I combined your script with this:\n\n-- >8 --\n#!/usr/bin/perl -w\n\n# Extract the stacktraces and identify them\n# by their SHA hashes (these identifiers are\n# not guaranteed to be stable across\n# re-compilations of the Git binaries).\n\nuse Digest::SHA qw(sha1 sha1_hex);\n\nmy $ctx = Digest::SHA->new(\"SHA-1\");\nmy $stage = 0;\n\nwhile (<>) {\n  my $collect = 0;\n  if ($stage == 0 && /irect leak of \\d+ byte.*allocated from:$/) {\n    $stage++;\n    $collect = 1;\n  } elsif($stage == 1 && /^\\s*\\#\\d+\\s+/) {\n    $collect = 1;\n  } elsif ($stage == 1 && /^\\s*$/) {\n    $digest = $ctx->hexdigest;\n    printf \"Stacktrace-hash: %s\\n\", $digest;\n    $ctx = Digest::SHA->new(\"SHA-1\");\n    $stage = 0;\n  } elsif ($stage == 1) {\n    print \"warning: unidentified string '$_'\\n\";\n  }\n  if ($collect) {\n    $ctx->add_bits($_);\n    print;\n  }\n}\n-- >8 --\n\nThen I report various ad-hoc metrics:\n\n-- >8 --\n#!/bin/bash\n\nfor d in \"$@\"\ndo\n  echo $d\n\n  echo -n \"  direct leaks: \"\n  grep \"Direct leak\" \"$d\"/* | wc -l\n\n  echo -n \"  indirect leaks: \"\n  grep \"Indirect leak\" \"$d\"/* | wc -l\n\n  echo -n \"  allocating places: \"\n  perl leaks.pl \"$d\"/* | sort -u | wc -l\n\n  echo -n \"  most common allocating place: \"\n  perl leaks.pl \"$d\"/* | sort \\\n       | uniq -c | sort -nr | head -1 | awk '{print $1;}'\n\n  echo -n \"  size of leak-reports: \"\n  cat \"$d\"/* | wc -l\n\n  echo -n \"  unique leaking stacktraces: \"\n  perl extract-traces.pl \"$d\"/* | grep \"Stacktrace-hash\" | sort -u | wc -l\n\n  echo -n \"  most common stacktrace: \"\n  perl extract-traces.pl \"$d\"/* | grep \"Stacktrace-hash\" | sort \\\n       | uniq -c | sort -nr | head -1 | awk '{print $1;}'\ndone\n-- >8 --\n\nIf PIDs of leaking processes collide, reports are lost. Something like\nthis as root helps: `echo 4194303 > /proc/sys/kernel/pid_max`\n\nStill, the numbers vary for back-to-back runs. Here are two runs on\nmaster and two runs on master plus the lockfile-patches I just sent.\n(I don't run all tests.)\n\nlsan_ea220ee4\n  direct leaks: 127165\n  indirect leaks: 83897\n  allocating places: 504\n  most common allocating place: 10212\n  size of leak-reports: 3662204\n  unique leaking stacktraces: 83265\n  most common stacktrace: 55\nlsan_ea220ee4-rerun\n  direct leaks: 127172\n  indirect leaks: 83903\n  allocating places: 504\n  most common allocating place: 10212\n  size of leak-reports: 3662334\n  unique leaking stacktraces: 83644\n  most common stacktrace: 57\nlsan_ea220ee4+lockfile_fixes\n  direct leaks: 118678\n  indirect leaks: 83908\n  allocating places: 493\n  most common allocating place: 10212\n  size of leak-reports: 3545563\n  unique leaking stacktraces: 99834\n  most common stacktrace: 32\nlsan_ea220ee4+lockfile_fixes-rerun\n  direct leaks: 118678\n  indirect leaks: 83902\n  allocating places: 491\n  most common allocating place: 10212\n  size of leak-reports: 3545463\n  unique leaking stacktraces: 82171\n  most common stacktrace: 40\n\n> So I don't know how useful any of that will be, but it at least should\n> give _some_ metric that should be diminishing as we fix leaks.\n\nIndeed.\n\nMartin\n"}]}