{"thread":{"id":"49467","subject":"[PATCH 0/2] Clean up leaks in commit-graph.c","startedAt":"2018-10-02T14:58:47Z","lastAt":"2018-10-03T17:12:21Z","messageCount":20,"participants":["Derrick Stolee via GitGitGadget","Martin Ågren","Stefan Beller","Jeff King","Derrick Stolee","Martin Ågren via GitGitGadget"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"359422","messageId":"pull.42.git.gitgitgadget@gmail.com","threadId":"49467","inReplyTo":null,"subject":"[PATCH 0/2] Clean up leaks in commit-graph.c","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2018-10-02T14:58:43Z","receivedAt":"2018-10-02T14:58:47Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"While looking at the commit-graph code, I noticed some memory leaks. These\ncan be found by running\n\nvalgrind --leak-check=full ./git commit-graph write --reachable\n\nThe impact of these leaks are small, as we never call write_commit_graph\n_reachable in a loop, but it is best to be diligent here.\n\nWhile looking at memory consumption within write_commit_graph(), I noticed\nthat we initialize our oid list with \"object count / 4\", which seems to be a\nhuge over-count. I reduce this by a factor of eight.\n\nI built off of ab/commit-graph-progress, because my patch involves lines\nclose to those changes.\n\nThanks, -Stolee\n\nDerrick Stolee (2):\n commit-graph: clean up leaked memory during write\n commit-graph: reduce initial oid allocation\n\n commit-graph.c | 10 ++++++++--\n 1 file changed, 8 insertions(+), 2 deletions(-)\n\n\nbase-commit: 6b89a34c89fc763292f06012318b852b74825619\nPublished-As: https://github.com/gitgitgadget/git/releases/tags/pr-42%2Fderrickstolee%2Fcommit-graph-leak-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-42/derrickstolee/commit-graph-leak-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/42\n-- \ngitgitgadget\n"},{"id":"359423","messageId":"6906c25415eddf79cc3f71f905a77b140f2f66f0.1538492321.git.gitgitgadget@gmail.com","threadId":"49467","inReplyTo":"pull.42.git.gitgitgadget@gmail.com","subject":"[PATCH 1/2] commit-graph: clean up leaked memory during write","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2018-10-02T14:58:45Z","receivedAt":"2018-10-02T14:58:49Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nThe write_commit_graph() method in commit-graph.c leaks some lits\nand strings during execution. In addition, a list of strings is\nleaked in write_commit_graph_reachable(). Clean these up so our\nmemory checking is cleaner.\n\nRunning 'valgrind --leak-check=full git commit-graph write\n--reachable' demonstrates these leaks and how they are fixed after\nthis change.\n\nSigned-off-by: Derrick Stolee \n---\n commit-graph.c | 8 +++++++-\n 1 file changed, 7 insertions(+), 1 deletion(-)\n\ndiff --git a/commit-graph.c b/commit-graph.c\nindex 2a24eb8b5a..7226bd6b58 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -698,6 +698,8 @@ void write_commit_graph_reachable(const char *obj_dir, int append,\n \tstring_list_init(&list, 1);\n \tfor_each_ref(add_ref_to_list, &list);\n \twrite_commit_graph(obj_dir, NULL, &list, append, report_progress);\n+\n+\tstring_list_clear(&list, 0);\n }\n \n void write_commit_graph(const char *obj_dir,\n@@ -846,9 +848,11 @@ void write_commit_graph(const char *obj_dir,\n \tcompute_generation_numbers(&commits, report_progress);\n \n \tgraph_name = get_commit_graph_filename(obj_dir);\n-\tif (safe_create_leading_directories(graph_name))\n+\tif (safe_create_leading_directories(graph_name)) {\n+\t\tUNLEAK(graph_name);\n \t\tdie_errno(_(\"unable to create leading directories of %s\"),\n \t\t\t graph_name);\n+\t}\n \n \thold_lock_file_for_update(&lk, graph_name, LOCK_DIE_ON_ERROR);\n \tf = hashfd(lk.tempfile->fd, lk.tempfile->filename.buf);\n@@ -893,6 +897,8 @@ void write_commit_graph(const char *obj_dir,\n \tfinalize_hashfile(f, NULL, CSUM_HASH_IN_STREAM | CSUM_FSYNC);\n \tcommit_lock_file(&lk);\n \n+\tfree(graph_name);\n+\tfree(commits.list);\n \tfree(oids.list);\n \toids.alloc = 0;\n \toids.nr = 0;\n-- \ngitgitgadget\n\n"},{"id":"359424","messageId":"e29a0eaf032fa011e30a91d8f8c514eefe12d14c.1538492322.git.gitgitgadget@gmail.com","threadId":"49467","inReplyTo":"pull.42.git.gitgitgadget@gmail.com","subject":"[PATCH 2/2] commit-graph: reduce initial oid allocation","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2018-10-02T14:58:46Z","receivedAt":"2018-10-02T14:58:49Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nWhile writing a commit-graph file, we store the full list of\ncommits in a flat list. We use this list for sorting and ensuring\nwe are closed under reachability.\n\nThe initial allocation assumed that (at most) one in four objects\nis a commit. This is a dramatic over-count for many repos,\nespecially large ones. Since we grow the repo dynamically, reduce\nthis count by a factor of eight. We still set it to a minimum of\n1024 before allocating.\n\nSigned-off-by: Derrick Stolee \n---\n commit-graph.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/commit-graph.c b/commit-graph.c\nindex 7226bd6b58..a24cceb55f 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -721,7 +721,7 @@ void write_commit_graph(const char *obj_dir,\n \tstruct progress *progress = NULL;\n \n \toids.nr = 0;\n-\toids.alloc = approximate_object_count() / 4;\n+\toids.alloc = approximate_object_count() / 32;\n \toids.progress = NULL;\n \toids.progress_done = 0;\n \n-- \ngitgitgadget\n"},{"id":"359429","messageId":"CAN0heSomOen+mWg1CuPGUCJFgmDQUjGLtAdxpWLwrStRU4Me8g@mail.gmail.com","threadId":"49467","inReplyTo":"6906c25415eddf79cc3f71f905a77b140f2f66f0.1538492321.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/2] commit-graph: clean up leaked memory during write","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2018-10-02T15:40:01Z","receivedAt":"2018-10-02T15:40:16Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On Tue, 2 Oct 2018 at 17:01, Derrick Stolee via GitGitGadget\n wrote:\n> diff --git a/commit-graph.c b/commit-graph.c\n> index 2a24eb8b5a..7226bd6b58 100644\n> --- a/commit-graph.c\n> +++ b/commit-graph.c\n> @@ -698,6 +698,8 @@ void write_commit_graph_reachable(const char *obj_dir, int append,\n> string_list_init(&list, 1);\n> for_each_ref(add_ref_to_list, &list);\n> write_commit_graph(obj_dir, NULL, &list, append, report_progress);\n> +\n> + string_list_clear(&list, 0);\n> }\n\nNit: The blank line adds some asymmetry, IMVHO.\n\n> void write_commit_graph(const char *obj_dir,\n> @@ -846,9 +848,11 @@ void write_commit_graph(const char *obj_dir,\n> compute_generation_numbers(&commits, report_progress);\n>\n> graph_name = get_commit_graph_filename(obj_dir);\n> - if (safe_create_leading_directories(graph_name))\n> + if (safe_create_leading_directories(graph_name)) {\n> + UNLEAK(graph_name);\n> die_errno(_(\"unable to create leading directories of %s\"),\n> graph_name);\n> + }\n\nDo you really need this hunk? In my testing with LeakSanitizer and\nvalgrind, I don't need this hunk to be leak-free. Generally speaking, it\nseems impossible to UNLEAK when dying, since we don't know what we have\nallocated higher up in the call-stack.\n\nWithout this hunk, this patch can have my\n\nReviewed-by: Martin Ågren \n\nas I've verified the leaks before and after. With this hunk, I am\npuzzled and feel uneasy, both about having to UNLEAK before dying and\nabout having to UNLEAK outside of builtin/.\n\n> + free(graph_name);\n> + free(commits.list);\n> free(oids.list);\n> oids.alloc = 0;\n> oids.nr = 0;\n\nBoth `commits` and `oids` are on the stack here, so cleaning up one more\nthan the other is a bit asymmetrical. Also, if we try to zero the counts\n-- which seems unnecessary to me, but which is not new with this patch --\nwe should perhaps use `FREE_AND_NULL` too. But personally, I would just\nuse `free` and leave `nr` and `alloc` at whatever values they happen to\nhave.\n\nMartin\n"},{"id":"359441","messageId":"CAGZ79kYtNnNPMv0=rB4ie=tOoYEPPZ=CmYFg4PG2_j96vjODeQ@mail.gmail.com","threadId":"49467","inReplyTo":"CAN0heSomOen+mWg1CuPGUCJFgmDQUjGLtAdxpWLwrStRU4Me8g@mail.gmail.com","subject":"Re: [PATCH 1/2] commit-graph: clean up leaked memory during write","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-10-02T17:59:28Z","receivedAt":"2018-10-02T17:59:45Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, Oct 2, 2018 at 8:40 AM Martin Ågren wrote:\n>\n> On Tue, 2 Oct 2018 at 17:01, Derrick Stolee via GitGitGadget\n> wrote:\n> > diff --git a/commit-graph.c b/commit-graph.c\n> > index 2a24eb8b5a..7226bd6b58 100644\n> > --- a/commit-graph.c\n> > +++ b/commit-graph.c\n> > @@ -698,6 +698,8 @@ void write_commit_graph_reachable(const char *obj_dir, int append,\n> > string_list_init(&list, 1);\n> > for_each_ref(add_ref_to_list, &list);\n> > write_commit_graph(obj_dir, NULL, &list, append, report_progress);\n> > +\n> > + string_list_clear(&list, 0);\n> > }\n>\n> Nit: The blank line adds some asymmetry, IMVHO.\n\nI think these blank lines are super common, as in:\n\n {\n declarations;\n\n multiple;\n lines(of);\n code;\n\n cleanup;\n and_frees;\n }\n\n(c.f. display_table in column.c, which I admit to have\ncherry-picked as an example).\n\nWhile in nit territory, I would rather move the string list init\ninto the first block:\n\n {\n struct string_list list = STRING_LIST_INIT_DUP;\n\n for_each_ref(add_ref_to_list, &list);\n write_commit_graph(obj_dir, NULL, &list, append);\n\n string_list_clear(&list, 0);\n }\n\n\n\n\n>\n> > void write_commit_graph(const char *obj_dir,\n> > @@ -846,9 +848,11 @@ void write_commit_graph(const char *obj_dir,\n> > compute_generation_numbers(&commits, report_progress);\n> >\n> > graph_name = get_commit_graph_filename(obj_dir);\n> > - if (safe_create_leading_directories(graph_name))\n> > + if (safe_create_leading_directories(graph_name)) {\n> > + UNLEAK(graph_name);\n> > die_errno(_(\"unable to create leading directories of %s\"),\n> > graph_name);\n> > + }\n>\n> Do you really need this hunk?\n\ngraph_name is produced via xstrfmt in get_commit_graph_filename,\nso it needs to be free'd in any return/exit path.\n\n> In my testing with LeakSanitizer and\n> valgrind, I don't need this hunk to be leak-free.\n\n\n> Generally speaking, it\n> seems impossible to UNLEAK when dying, since we don't know what we have\n> allocated higher up in the call-stack.\n\nI do not understand; I thought UNLEAK was specifically for the purpose of\ndie() calls without imposing extra overhead; rereading 0e5bba53af\n(add UNLEAK annotation for reducing leak false positives, 2017-09-08)\ndoesn't provide an example for prematurely die()ing, only for regular\nprogram exit.\n\n> Reviewed-by: Martin Ågren \n>\n> as I've verified the leaks before and after. With this hunk, I am\n> puzzled and feel uneasy, both about having to UNLEAK before dying and\n> about having to UNLEAK outside of builtin/.\n\nI am not uneasy about an UNLEAK before dying, but about dying outside\nbuiltin/ in general (but having a die call accompanied by UNLEAK seems\nto be the right thing). Can you explain the worries you have regarding the\nallocations on the call stack, as xstrfmt is allocating on the heap and we\nonly UNLEAK the pointer to that?\n\nStefan\n"},{"id":"359448","messageId":"CAN0heSqOjYDXRf4KE_C0GDnFW8r4qVfWnUVuW-Q+4D87nhFURQ@mail.gmail.com","threadId":"49467","inReplyTo":"CAGZ79kYtNnNPMv0=rB4ie=tOoYEPPZ=CmYFg4PG2_j96vjODeQ@mail.gmail.com","subject":"Re: [PATCH 1/2] commit-graph: clean up leaked memory during write","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2018-10-02T19:08:57Z","receivedAt":"2018-10-02T19:09:11Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On Tue, 2 Oct 2018 at 19:59, Stefan Beller wrote:\n> > > +\n> > > + string_list_clear(&list, 0);\n> > > }\n> >\n> > Nit: The blank line adds some asymmetry, IMVHO.\n>\n> I think these blank lines are super common, as in:\n>\n> {\n> declarations;\n>\n> multiple;\n> lines(of);\n> code;\n>\n> cleanup;\n> and_frees;\n> }\n>\n> (c.f. display_table in column.c, which I admit to have\n> cherry-picked as an example).\n>\n> While in nit territory, I would rather move the string list init\n> into the first block:\n>\n> {\n> struct string_list list = STRING_LIST_INIT_DUP;\n>\n> for_each_ref(add_ref_to_list, &list);\n> write_commit_graph(obj_dir, NULL, &list, append);\n>\n> string_list_clear(&list, 0);\n> }\n\nNow this looks very symmetrical. :-)\n\n> > > void write_commit_graph(const char *obj_dir,\n> > > @@ -846,9 +848,11 @@ void write_commit_graph(const char *obj_dir,\n> > > compute_generation_numbers(&commits, report_progress);\n> > >\n> > > graph_name = get_commit_graph_filename(obj_dir);\n> > > - if (safe_create_leading_directories(graph_name))\n> > > + if (safe_create_leading_directories(graph_name)) {\n> > > + UNLEAK(graph_name);\n> > > die_errno(_(\"unable to create leading directories of %s\"),\n> > > graph_name);\n> > > + }\n> >\n> > Do you really need this hunk?\n>\n> graph_name is produced via xstrfmt in get_commit_graph_filename,\n> so it needs to be free'd in any return/exit path.\n\nAgreed. Although I am questioning that `die()` and its siblings count.\n\n> > In my testing with LeakSanitizer and\n> > valgrind, I don't need this hunk to be leak-free.\n>\n>\n> > Generally speaking, it\n> > seems impossible to UNLEAK when dying, since we don't know what we have\n> > allocated higher up in the call-stack.\n>\n> I do not understand; I thought UNLEAK was specifically for the purpose of\n> die() calls without imposing extra overhead; rereading 0e5bba53af\n> (add UNLEAK annotation for reducing leak false positives, 2017-09-08)\n> doesn't provide an example for prematurely die()ing, only for regular\n> program exit.\n>\n> > [...] With this hunk, I am\n> > puzzled and feel uneasy, both about having to UNLEAK before dying and\n> > about having to UNLEAK outside of builtin/.\n>\n> I am not uneasy about an UNLEAK before dying, but about dying outside\n> builtin/ in general\n\nYeah, not dying would be even better (out of scope for this patch).\n\n> (but having a die call accompanied by UNLEAK seems\n> to be the right thing). Can you explain the worries you have regarding the\n> allocations on the call stack, as xstrfmt is allocating on the heap and we\n> only UNLEAK the pointer to that?\n\nI think we agree that leaking things \"allocat[ed] on the call stack\"\nisn't much of a worry. The reason I mentioned the call stack is that\nwe've got any number of calls behind us on it, and we might have made\nall sorts of allocations on the heap, and at this point, we have no\nidea about what we should be UNLEAK-ing.\n\nMy worry is that one of these would seem to be true:\n\n* UNLEAK is unsuitable for the job. Whenever we have a `die()` as we do\n here, we can UNLEAK the variables we know of, but we can't do anything\n about the allocations we have made higher up the call-chain. Our test\n suite obviously provokes lots of calls to `die()` -- imagine that each\n of those leaves a few leaked allocations behind. We'd have a semi-huge\n number of leaks being reported. While we could mark with UNLEAK to\n reduce that number, we wouldn't be able to bring the number of leaks\n down to anywhere near manageable where we'd be able to find the last\n few true positives.\n\n* We add code with no purpose. In this case, we're not talking a lot of\n lines, but across the code base, if they bring no gain, they are bound\n to provide a negative net value given enough time.\n\nMartin\n"},{"id":"359455","messageId":"CAGZ79kb2pE3pFQx4A=oo-mYORjN1ubCgV6Gotc78i7d+BqZdBw@mail.gmail.com","threadId":"49467","inReplyTo":"CAN0heSqOjYDXRf4KE_C0GDnFW8r4qVfWnUVuW-Q+4D87nhFURQ@mail.gmail.com","subject":"Re: [PATCH 1/2] commit-graph: clean up leaked memory during write","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-10-02T19:44:09Z","receivedAt":"2018-10-02T19:44:25Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, Oct 2, 2018 at 12:09 PM Martin Ågren wrote:\n>\n> On Tue, 2 Oct 2018 at 19:59, Stefan Beller wrote:\n> > > > +\n> > > > + string_list_clear(&list, 0);\n> > > > }\n> > >\n> > > Nit: The blank line adds some asymmetry, IMVHO.\n> >\n> > I think these blank lines are super common, as in:\n> >\n> > {\n> > declarations;\n> >\n> > multiple;\n> > lines(of);\n> > code;\n> >\n> > cleanup;\n> > and_frees;\n> > }\n> >\n> > (c.f. display_table in column.c, which I admit to have\n> > cherry-picked as an example).\n> >\n> > While in nit territory, I would rather move the string list init\n> > into the first block:\n> >\n> > {\n> > struct string_list list = STRING_LIST_INIT_DUP;\n> >\n> > for_each_ref(add_ref_to_list, &list);\n> > write_commit_graph(obj_dir, NULL, &list, append);\n> >\n> > string_list_clear(&list, 0);\n> > }\n>\n> Now this looks very symmetrical. :-)\n>\n> > > > void write_commit_graph(const char *obj_dir,\n> > > > @@ -846,9 +848,11 @@ void write_commit_graph(const char *obj_dir,\n> > > > compute_generation_numbers(&commits, report_progress);\n> > > >\n> > > > graph_name = get_commit_graph_filename(obj_dir);\n> > > > - if (safe_create_leading_directories(graph_name))\n> > > > + if (safe_create_leading_directories(graph_name)) {\n> > > > + UNLEAK(graph_name);\n> > > > die_errno(_(\"unable to create leading directories of %s\"),\n> > > > graph_name);\n> > > > + }\n> > >\n> > > Do you really need this hunk?\n> >\n> > graph_name is produced via xstrfmt in get_commit_graph_filename,\n> > so it needs to be free'd in any return/exit path.\n>\n> Agreed. Although I am questioning that `die()` and its siblings count.\n>\n> > > In my testing with LeakSanitizer and\n> > > valgrind, I don't need this hunk to be leak-free.\n> >\n> >\n> > > Generally speaking, it\n> > > seems impossible to UNLEAK when dying, since we don't know what we have\n> > > allocated higher up in the call-stack.\n> >\n> > I do not understand; I thought UNLEAK was specifically for the purpose of\n> > die() calls without imposing extra overhead; rereading 0e5bba53af\n> > (add UNLEAK annotation for reducing leak false positives, 2017-09-08)\n> > doesn't provide an example for prematurely die()ing, only for regular\n> > program exit.\n> >\n> > > [...] With this hunk, I am\n> > > puzzled and feel uneasy, both about having to UNLEAK before dying and\n> > > about having to UNLEAK outside of builtin/.\n> >\n> > I am not uneasy about an UNLEAK before dying, but about dying outside\n> > builtin/ in general\n>\n> Yeah, not dying would be even better (out of scope for this patch).\n>\n> > (but having a die call accompanied by UNLEAK seems\n> > to be the right thing). Can you explain the worries you have regarding the\n> > allocations on the call stack, as xstrfmt is allocating on the heap and we\n> > only UNLEAK the pointer to that?\n>\n> I think we agree that leaking things \"allocat[ed] on the call stack\"\n> isn't much of a worry. The reason I mentioned the call stack is that\n> we've got any number of calls behind us on it, and we might have made\n> all sorts of allocations on the heap, and at this point, we have no\n> idea about what we should be UNLEAK-ing.\n\nWouldn't that be the responsibility of each function to make sure things\nare UNLEAK'd or free'd before the function is either over or stopped\nintermittently (by a subroutine dying) ?\n\nIn an ideal world we'd only ever exit/die in the functions high up\nthe call chain (which are in builtin/) and all other code would gracefully\nreturn error codes or messages instead or even cope with some failure\nconditions?\n\n> My worry is that one of these would seem to be true:\n>\n> * UNLEAK is unsuitable for the job. Whenever we have a `die()` as we do\n> here, we can UNLEAK the variables we know of, but we can't do anything\n> about the allocations we have made higher up the call-chain.\n\nIMHO that is the issue of the functions higher up the call chain and ought\nto not affect this patch. By doing the right thing here locally the code base\nwill approach a good state eventually.\n\n> Our test\n> suite obviously provokes lots of calls to `die()` -- imagine that each\n> of those leaves a few leaked allocations behind. We'd have a semi-huge\n> number of leaks being reported. While we could mark with UNLEAK to\n> reduce that number, we wouldn't be able to bring the number of leaks\n> down to anywhere near manageable where we'd be able to find the last\n> few true positives.\n\nMakes sense.\n\n> * We add code with no purpose. In this case, we're not talking a lot of\n> lines, but across the code base, if they bring no gain, they are bound\n> to provide a negative net value given enough time.\n\nI see. I did not estimate its negative impact to be high enough, as the\nUNLEAK near a die() call was obvious good thing (locally).\n\nI don't know what the best way to proceed is in this case.\n\nThanks,\nStefan\n"},{"id":"359478","messageId":"20181002223434.GA5588@sigill.intra.peff.net","threadId":"49467","inReplyTo":"CAGZ79kb2pE3pFQx4A=oo-mYORjN1ubCgV6Gotc78i7d+BqZdBw@mail.gmail.com","subject":"Re: [PATCH 1/2] commit-graph: clean up leaked memory during write","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-10-02T22:34:35Z","receivedAt":"2018-10-02T22:34:38Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Oct 02, 2018 at 12:44:09PM -0700, Stefan Beller wrote:\n\n> > My worry is that one of these would seem to be true:\n> >\n> > * UNLEAK is unsuitable for the job. Whenever we have a `die()` as we do\n> > here, we can UNLEAK the variables we know of, but we can't do anything\n> > about the allocations we have made higher up the call-chain.\n> \n> IMHO that is the issue of the functions higher up the call chain and ought\n> to not affect this patch. By doing the right thing here locally the code base\n> will approach a good state eventually.\n\nBut it's impossible. If I do this:\n\n foo = xstrdup(bar);\n subfunction(foo);\n\nthen I cannot protect myself from leaking \"foo\" when subfunction() calls\ndie(). It must be valid when I enter the function, and I have no\nopportunity to run code when it leaves (because it never does).\n\n> > * We add code with no purpose. In this case, we're not talking a lot of\n> > lines, but across the code base, if they bring no gain, they are bound\n> > to provide a negative net value given enough time.\n> \n> I see. I did not estimate its negative impact to be high enough, as the\n> UNLEAK near a die() call was obvious good thing (locally).\n> \n> I don't know what the best way to proceed is in this case.\n\nMy preference is to avoid them in the name of simplicity. If you're\nusing \"make SANITIZE=leak test\" to check for leaks, it will skip these\ncases. If you're using valgrind, I think these may be reported as\n\"reachable\". But that number already isn't useful for finding real\nleaks, because it includes cases like the \"foo\" above as well as\nprogram-lifetime globals.\n\nThe only argument (IMHO) for such an UNLEAK() is that it annotates the\nlocation for when somebody later changes the function to \"return -1\"\ninstead of dying. But if we are going to do such annotation, we may as\nwell actually call free(), which is what the \"return\" version will\nultimately have to do.\n\nI'd actually be _more_ favorable to calling free() instead of UNLEAK()\nthere, but I'm still mildly negative, just because it may go stale (and\nour leak-checking wouldn't usefully notice these cases). Anybody\nconverting that die() to a return needs to re-analyze the function for\nwhat might need to be released (and that includes non-memory bits like\ndescriptors, too).\n\n-Peff\n"},{"id":"359479","messageId":"20181002223738.GB5588@sigill.intra.peff.net","threadId":"49467","inReplyTo":"CAGZ79kYtNnNPMv0=rB4ie=tOoYEPPZ=CmYFg4PG2_j96vjODeQ@mail.gmail.com","subject":"Re: [PATCH 1/2] commit-graph: clean up leaked memory during write","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-10-02T22:37:38Z","receivedAt":"2018-10-02T22:37:41Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Oct 02, 2018 at 10:59:28AM -0700, Stefan Beller wrote:\n\n> > Generally speaking, it\n> > seems impossible to UNLEAK when dying, since we don't know what we have\n> > allocated higher up in the call-stack.\n> \n> I do not understand; I thought UNLEAK was specifically for the purpose of\n> die() calls without imposing extra overhead; rereading 0e5bba53af\n> (add UNLEAK annotation for reducing leak false positives, 2017-09-08)\n> doesn't provide an example for prematurely die()ing, only for regular\n> program exit.\n\nI responded elsewhere, but as the author of UNLEAK, let me comment here:\nit was intended only for program exit. That's why there are no such\nexamples. :)\n\nIf you're using it anywhere except the return from a cmd_* function, or\na static-local helper that's called from a cmd_*, you should probably\nactually be freeing the memory.\n\n-Peff\n"},{"id":"359480","messageId":"CAGZ79kaatUeHEGnW9HXv7Xgsj1Sf_M6hcgQ_5oVAOM85rqn34Q@mail.gmail.com","threadId":"49467","inReplyTo":"20181002223434.GA5588@sigill.intra.peff.net","subject":"Re: [PATCH 1/2] commit-graph: clean up leaked memory during write","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2018-10-02T22:44:56Z","receivedAt":"2018-10-02T22:45:10Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":">\n> My preference is to avoid them in the name of simplicity. If you're\n> using \"make SANITIZE=leak test\" to check for leaks, it will skip these\n> cases. If you're using valgrind, I think these may be reported as\n> \"reachable\". But that number already isn't useful for finding real\n> leaks, because it includes cases like the \"foo\" above as well as\n> program-lifetime globals.\n>\n> The only argument (IMHO) for such an UNLEAK() is that it annotates the\n> location for when somebody later changes the function to \"return -1\"\n> instead of dying. But if we are going to do such annotation, we may as\n> well actually call free(), which is what the \"return\" version will\n> ultimately have to do.\n\nHeh, that was part of my reasoning why we'd want to have *something*.\n\n> I'd actually be _more_ favorable to calling free() instead of UNLEAK()\n> there, but I'm still mildly negative, just because it may go stale (and\n> our leak-checking wouldn't usefully notice these cases). Anybody\n> converting that die() to a return needs to re-analyze the function for\n> what might need to be released (and that includes non-memory bits like\n> descriptors, too).\n\nSounds reasonable, so then the consensus (between Martin, you and me)\nis to drop the UNLEAK.\n"},{"id":"359511","messageId":"1686b309-e0af-5518-0fa5-4635b2f481b9@gmail.com","threadId":"49467","inReplyTo":"CAGZ79kaatUeHEGnW9HXv7Xgsj1Sf_M6hcgQ_5oVAOM85rqn34Q@mail.gmail.com","subject":"Re: [PATCH 1/2] commit-graph: clean up leaked memory during write","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2018-10-03T12:04:30Z","receivedAt":"2018-10-03T12:04:35Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 10/2/2018 6:44 PM, Stefan Beller wrote:\n>> My preference is to avoid them in the name of simplicity. If you're\n>> using \"make SANITIZE=leak test\" to check for leaks, it will skip these\n>> cases. If you're using valgrind, I think these may be reported as\n>> \"reachable\". But that number already isn't useful for finding real\n>> leaks, because it includes cases like the \"foo\" above as well as\n>> program-lifetime globals.\n>>\n>> The only argument (IMHO) for such an UNLEAK() is that it annotates the\n>> location for when somebody later changes the function to \"return -1\"\n>> instead of dying. But if we are going to do such annotation, we may as\n>> well actually call free(), which is what the \"return\" version will\n>> ultimately have to do.\n> Heh, that was part of my reasoning why we'd want to have *something*.\n>\n>> I'd actually be _more_ favorable to calling free() instead of UNLEAK()\n>> there, but I'm still mildly negative, just because it may go stale (and\n>> our leak-checking wouldn't usefully notice these cases). Anybody\n>> converting that die() to a return needs to re-analyze the function for\n>> what might need to be released (and that includes non-memory bits like\n>> descriptors, too).\n> Sounds reasonable, so then the consensus (between Martin, you and me)\n> is to drop the UNLEAK.\nThanks for the discussion here. I'll drop the UNLEAK for now and think \nabout how to remove the die() calls from commit-graph.c in a later series.\n\nThanks,\n-Stolee\n"},{"id":"359533","messageId":"cover.1538579441.git.martin.agren@gmail.com","threadId":"49467","inReplyTo":"1686b309-e0af-5518-0fa5-4635b2f481b9@gmail.com","subject":"[PATCH 0/2] commit-graph: more leak fixes","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2018-10-03T15:36:21Z","receivedAt":"2018-10-03T15:36:45Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"Hi Derrick,\n\nThese two patches on top of yours make the test suite (i.e., the subset\nof it that I run) leak-free with respect to builtin/commit-graph.c and\ncommit-graph.c.\n\nThe first could be squashed into your patch 1/2. It touches the same\nfunction, but it requires a different usage to trigger, so squashing it\nin would require broadening the scope. I understand if you don't want to\ndo that.\n\nIf you want to pick these up as part of your re-roll in any way, shape\nor form, go ahead. If not, they can go in separately, either in parallel\nor after your series lands. Whatever the destiny of this posting, I'll\nfollow through as appropriate.\n\nMartin\n\nMartin Ågren (2):\n commit-graph: free `struct packed_git` after closing it\n builtin/commit-graph.c: UNLEAK variables\n\n builtin/commit-graph.c | 11 ++++++-----\n commit-graph.c | 1 +\n 2 files changed, 7 insertions(+), 5 deletions(-)\n\n-- \n2.19.0.329.g76f2f5c1e3\n\n"},{"id":"359534","messageId":"84a5957ab8a38ac18c432843a3717c0afd0fbea9.1538579441.git.martin.agren@gmail.com","threadId":"49467","inReplyTo":"cover.1538579441.git.martin.agren@gmail.com","subject":"[PATCH 1/2] commit-graph: free `struct packed_git` after closing it","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2018-10-03T15:36:22Z","receivedAt":"2018-10-03T15:36:49Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"`close_pack(p)` does not free the memory which `p` points to, so follow\nup with a call to `free(p)`. All other users of `close_pack()` look ok.\n\nSigned-off-by: Martin Ågren \n---\n commit-graph.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/commit-graph.c b/commit-graph.c\nindex 3d644fddc0..9b481bcd06 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -766,6 +766,7 @@ void write_commit_graph(const char *obj_dir,\n \t\t\t\tdie(_(\"error opening index for %s\"), packname.buf);\n \t\t\tfor_each_object_in_pack(p, add_packed_commits, &oids, 0);\n \t\t\tclose_pack(p);\n+\t\t\tfree(p);\n \t\t}\n \t\tstop_progress(&oids.progress);\n \t\tstrbuf_release(&packname);\n-- \n2.19.0.329.g76f2f5c1e3\n\n"},{"id":"359535","messageId":"5d260588675f6446079efc05b00492f2c99948a3.1538579441.git.martin.agren@gmail.com","threadId":"49467","inReplyTo":"cover.1538579441.git.martin.agren@gmail.com","subject":"[PATCH 2/2] builtin/commit-graph.c: UNLEAK variables","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2018-10-03T15:36:23Z","receivedAt":"2018-10-03T15:36:52Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"`graph_verify()`, `graph_read()` and `graph_write()` do the hard work of\n`cmd_commit_graph()`. As soon as these return, so does\n`cmd_commit_graph()`.\n\n`strbuf_getline()` may allocate memory in the strbuf, yet return EOF.\nWe need to release the strbuf or UNLEAK it. Go for the latter since we\nare close to returning from `graph_write()`.\n\n`graph_write()` also fails to free the strings in the string list. They\nhave been added to the list with `strdup_strings` set to 0. We could\nflip `strdup_strings` before clearing the list, which is our usual hack\nin situations like this. But since we are about to exit, let's just\nUNLEAK the whole string list instead.\n\nUNLEAK `graph` in `graph_verify`. While at it, and for consistency,\nUNLEAK in `graph_read()` as well, and remove an unnecessary UNLEAK just\nbefore dying.\n\nSigned-off-by: Martin Ågren \n---\n builtin/commit-graph.c | 11 ++++++-----\n 1 file changed, 6 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/commit-graph.c b/builtin/commit-graph.c\nindex bc0fa9ba52..66f12eb009 100644\n--- a/builtin/commit-graph.c\n+++ b/builtin/commit-graph.c\n@@ -64,6 +64,7 @@ static int graph_verify(int argc, const char **argv)\n \tif (!graph)\n \t\treturn 0;\n \n+\tUNLEAK(graph);\n \treturn verify_commit_graph(the_repository, graph);\n }\n \n@@ -89,10 +90,8 @@ static int graph_read(int argc, const char **argv)\n \tgraph_name = get_commit_graph_filename(opts.obj_dir);\n \tgraph = load_commit_graph_one(graph_name);\n \n-\tif (!graph) {\n-\t\tUNLEAK(graph_name);\n+\tif (!graph)\n \t\tdie(\"graph file %s does not exist\", graph_name);\n-\t}\n \n \tFREE_AND_NULL(graph_name);\n \n@@ -115,7 +114,7 @@ static int graph_read(int argc, const char **argv)\n \t\tprintf(\" large_edges\");\n \tprintf(\"\\n\");\n \n-\tfree_commit_graph(graph);\n+\tUNLEAK(graph);\n \n \treturn 0;\n }\n@@ -166,6 +165,8 @@ static int graph_write(int argc, const char **argv)\n \t\t\tpack_indexes = &lines;\n \t\tif (opts.stdin_commits)\n \t\t\tcommit_hex = &lines;\n+\n+\t\tUNLEAK(buf);\n \t}\n \n \twrite_commit_graph(opts.obj_dir,\n@@ -174,7 +175,7 @@ static int graph_write(int argc, const char **argv)\n \t\t\t opts.append,\n \t\t\t 1);\n \n-\tstring_list_clear(&lines, 0);\n+\tUNLEAK(lines);\n \treturn 0;\n }\n \n-- \n2.19.0.329.g76f2f5c1e3\n\n"},{"id":"359539","messageId":"07aa3628-f651-3a8f-21b4-b66099de864c@gmail.com","threadId":"49467","inReplyTo":"cover.1538579441.git.martin.agren@gmail.com","subject":"Re: [PATCH 0/2] commit-graph: more leak fixes","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2018-10-03T16:19:49Z","receivedAt":"2018-10-03T16:19:57Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 10/3/2018 11:36 AM, Martin Ågren wrote:\n> Hi Derrick,\n>\n> These two patches on top of yours make the test suite (i.e., the subset\n> of it that I run) leak-free with respect to builtin/commit-graph.c and\n> commit-graph.c.\n\nThanks!\n\n> The first could be squashed into your patch 1/2. It touches the same\n> function, but it requires a different usage to trigger, so squashing it\n> in would require broadening the scope. I understand if you don't want to\n> do that.\nI'm fine with squashing it in with both our sign-offs. It is the same \nidea, it just requires a different set of arguments to hit it. I'll \nadjust the commit message as necessary.\n\n> If you want to pick these up as part of your re-roll in any way, shape\n> or form, go ahead. If not, they can go in separately, either in parallel\n> or after your series lands. Whatever the destiny of this posting, I'll\n> follow through as appropriate.\n\nI'll add your PATCH 2/2 to my v2. Thanks!\n"},{"id":"359540","messageId":"CAN0heSoBGAbUDq0PwWpwhcSQ3_h4zMJHoKcBePDu4Y_vXns8Mg@mail.gmail.com","threadId":"49467","inReplyTo":"07aa3628-f651-3a8f-21b4-b66099de864c@gmail.com","subject":"Re: [PATCH 0/2] commit-graph: more leak fixes","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2018-10-03T16:24:12Z","receivedAt":"2018-10-03T16:24:27Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On Wed, 3 Oct 2018 at 18:19, Derrick Stolee wrote:\n> I'm fine with squashing it in with both our sign-offs. It is the same\n> idea, it just requires a different set of arguments to hit it. I'll\n> adjust the commit message as necessary.\n\n> I'll add your PATCH 2/2 to my v2. Thanks!\n\nCool, thanks a lot.\n\nMartin\n"},{"id":"359553","messageId":"pull.42.v2.git.gitgitgadget@gmail.com","threadId":"49467","inReplyTo":"pull.42.git.gitgitgadget@gmail.com","subject":"[PATCH v2 0/3] Clean up leaks in commit-graph.c","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2018-10-03T17:12:14Z","receivedAt":"2018-10-03T17:12:17Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"While looking at the commit-graph code, I noticed some memory leaks. These\ncan be found by running\n\nvalgrind --leak-check=full ./git commit-graph write --reachable\n\nThe impact of these leaks are small, as we never call write_commit_graph\n_reachable in a loop, but it is best to be diligent here.\n\nWhile looking at memory consumption within write_commit_graph(), I noticed\nthat we initialize our oid list with \"object count / 4\", which seems to be a\nhuge over-count. I reduce this by a factor of eight.\n\nI built off of ab/commit-graph-progress, because my patch involves lines\nclose to those changes.\n\nV2 includes feedback from V1 along with Martin's additional patches.\n\nThanks, -Stolee\n\nDerrick Stolee (2):\n commit-graph: clean up leaked memory during write\n commit-graph: reduce initial oid allocation\n\nMartin Ågren (1):\n builtin/commit-graph.c: UNLEAK variables\n\n builtin/commit-graph.c | 11 ++++++-----\n commit-graph.c | 16 ++++++++++------\n 2 files changed, 16 insertions(+), 11 deletions(-)\n\n\nbase-commit: 6b89a34c89fc763292f06012318b852b74825619\nPublished-As: https://github.com/gitgitgadget/git/releases/tags/pr-42%2Fderrickstolee%2Fcommit-graph-leak-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-42/derrickstolee/commit-graph-leak-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/42\n\nRange-diff vs v1:\n\n 1: 6906c25415 ! 1: ba65680b3d commit-graph: clean up leaked memory during write\n @@ -7,17 +7,29 @@\n leaked in write_commit_graph_reachable(). Clean these up so our\n memory checking is cleaner.\n \n - Running 'valgrind --leak-check=full git commit-graph write\n - --reachable' demonstrates these leaks and how they are fixed after\n - this change.\n + Further, if we use a list of pack-files to find the commits, we\n + can leak the packed_git structs after scanning them for commits.\n \n + Running the following commands demonstrates the leak before and\n + the fix after:\n +\n + * valgrind --leak-check=full ./git commit-graph write --reachable\n + * valgrind --leak-check=full ./git commit-graph write --stdin-packs\n +\n + Signed-off-by: Martin Ågren \n Signed-off-by: Derrick Stolee \n \n diff --git a/commit-graph.c b/commit-graph.c\n --- a/commit-graph.c\n +++ b/commit-graph.c\n @@\n - \tstring_list_init(&list, 1);\n + void write_commit_graph_reachable(const char *obj_dir, int append,\n + \t\t\t\t int report_progress)\n + {\n +-\tstruct string_list list;\n ++\tstruct string_list list = STRING_LIST_INIT_DUP;\n + \n +-\tstring_list_init(&list, 1);\n \tfor_each_ref(add_ref_to_list, &list);\n \twrite_commit_graph(obj_dir, NULL, &list, append, report_progress);\n +\n @@ -25,6 +37,14 @@\n }\n \n void write_commit_graph(const char *obj_dir,\n +@@\n + \t\t\t\tdie(_(\"error opening index for %s\"), packname.buf);\n + \t\t\tfor_each_object_in_pack(p, add_packed_commits, &oids, 0);\n + \t\t\tclose_pack(p);\n ++\t\t\tfree(p);\n + \t\t}\n + \t\tstop_progress(&oids.progress);\n + \t\tstrbuf_release(&packname);\n @@\n \tcompute_generation_numbers(&commits, report_progress);\n \n @@ -45,5 +65,8 @@\n +\tfree(graph_name);\n +\tfree(commits.list);\n \tfree(oids.list);\n - \toids.alloc = 0;\n - \toids.nr = 0;\n +-\toids.alloc = 0;\n +-\toids.nr = 0;\n + }\n + \n + #define VERIFY_COMMIT_GRAPH_ERROR_HASH 2\n -: ---------- > 2: 13032d8475 builtin/commit-graph.c: UNLEAK variables\n 2: e29a0eaf03 = 3: 1002fd34fc commit-graph: reduce initial oid allocation\n\n-- \ngitgitgadget\n"},{"id":"359554","messageId":"ba65680b3d0e50ad8be415e10c1e6b63c58d8b13.1538586732.git.gitgitgadget@gmail.com","threadId":"49467","inReplyTo":"pull.42.v2.git.gitgitgadget@gmail.com","subject":"[PATCH v2 1/3] commit-graph: clean up leaked memory during write","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2018-10-03T17:12:15Z","receivedAt":"2018-10-03T17:12:19Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nThe write_commit_graph() method in commit-graph.c leaks some lits\nand strings during execution. In addition, a list of strings is\nleaked in write_commit_graph_reachable(). Clean these up so our\nmemory checking is cleaner.\n\nFurther, if we use a list of pack-files to find the commits, we\ncan leak the packed_git structs after scanning them for commits.\n\nRunning the following commands demonstrates the leak before and\nthe fix after:\n\n* valgrind --leak-check=full ./git commit-graph write --reachable\n* valgrind --leak-check=full ./git commit-graph write --stdin-packs\n\nSigned-off-by: Martin Ågren \nSigned-off-by: Derrick Stolee \n---\n commit-graph.c | 14 +++++++++-----\n 1 file changed, 9 insertions(+), 5 deletions(-)\n\ndiff --git a/commit-graph.c b/commit-graph.c\nindex 2a24eb8b5a..ceca6026b0 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -693,11 +693,12 @@ static int add_ref_to_list(const char *refname,\n void write_commit_graph_reachable(const char *obj_dir, int append,\n \t\t\t\t int report_progress)\n {\n-\tstruct string_list list;\n+\tstruct string_list list = STRING_LIST_INIT_DUP;\n \n-\tstring_list_init(&list, 1);\n \tfor_each_ref(add_ref_to_list, &list);\n \twrite_commit_graph(obj_dir, NULL, &list, append, report_progress);\n+\n+\tstring_list_clear(&list, 0);\n }\n \n void write_commit_graph(const char *obj_dir,\n@@ -764,6 +765,7 @@ void write_commit_graph(const char *obj_dir,\n \t\t\t\tdie(_(\"error opening index for %s\"), packname.buf);\n \t\t\tfor_each_object_in_pack(p, add_packed_commits, &oids, 0);\n \t\t\tclose_pack(p);\n+\t\t\tfree(p);\n \t\t}\n \t\tstop_progress(&oids.progress);\n \t\tstrbuf_release(&packname);\n@@ -846,9 +848,11 @@ void write_commit_graph(const char *obj_dir,\n \tcompute_generation_numbers(&commits, report_progress);\n \n \tgraph_name = get_commit_graph_filename(obj_dir);\n-\tif (safe_create_leading_directories(graph_name))\n+\tif (safe_create_leading_directories(graph_name)) {\n+\t\tUNLEAK(graph_name);\n \t\tdie_errno(_(\"unable to create leading directories of %s\"),\n \t\t\t graph_name);\n+\t}\n \n \thold_lock_file_for_update(&lk, graph_name, LOCK_DIE_ON_ERROR);\n \tf = hashfd(lk.tempfile->fd, lk.tempfile->filename.buf);\n@@ -893,9 +897,9 @@ void write_commit_graph(const char *obj_dir,\n \tfinalize_hashfile(f, NULL, CSUM_HASH_IN_STREAM | CSUM_FSYNC);\n \tcommit_lock_file(&lk);\n \n+\tfree(graph_name);\n+\tfree(commits.list);\n \tfree(oids.list);\n-\toids.alloc = 0;\n-\toids.nr = 0;\n }\n \n #define VERIFY_COMMIT_GRAPH_ERROR_HASH 2\n-- \ngitgitgadget\n\n"},{"id":"359555","messageId":"13032d847533cfecbd9a912312cab4f256e46b30.1538586732.git.gitgitgadget@gmail.com","threadId":"49467","inReplyTo":"pull.42.v2.git.gitgitgadget@gmail.com","subject":"[PATCH v2 2/3] builtin/commit-graph.c: UNLEAK variables","fromName":"Martin Ågren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2018-10-03T17:12:17Z","receivedAt":"2018-10-03T17:12:21Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"From: =?UTF-8?q?Martin=20=C3=85gren?= \n\n`graph_verify()`, `graph_read()` and `graph_write()` do the hard work of\n`cmd_commit_graph()`. As soon as these return, so does\n`cmd_commit_graph()`.\n\n`strbuf_getline()` may allocate memory in the strbuf, yet return EOF.\nWe need to release the strbuf or UNLEAK it. Go for the latter since we\nare close to returning from `graph_write()`.\n\n`graph_write()` also fails to free the strings in the string list. They\nhave been added to the list with `strdup_strings` set to 0. We could\nflip `strdup_strings` before clearing the list, which is our usual hack\nin situations like this. But since we are about to exit, let's just\nUNLEAK the whole string list instead.\n\nUNLEAK `graph` in `graph_verify`. While at it, and for consistency,\nUNLEAK in `graph_read()` as well, and remove an unnecessary UNLEAK just\nbefore dying.\n\nSigned-off-by: Martin Ågren \nSigned-off-by: Derrick Stolee \n---\n builtin/commit-graph.c | 11 ++++++-----\n 1 file changed, 6 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/commit-graph.c b/builtin/commit-graph.c\nindex bc0fa9ba52..66f12eb009 100644\n--- a/builtin/commit-graph.c\n+++ b/builtin/commit-graph.c\n@@ -64,6 +64,7 @@ static int graph_verify(int argc, const char **argv)\n \tif (!graph)\n \t\treturn 0;\n \n+\tUNLEAK(graph);\n \treturn verify_commit_graph(the_repository, graph);\n }\n \n@@ -89,10 +90,8 @@ static int graph_read(int argc, const char **argv)\n \tgraph_name = get_commit_graph_filename(opts.obj_dir);\n \tgraph = load_commit_graph_one(graph_name);\n \n-\tif (!graph) {\n-\t\tUNLEAK(graph_name);\n+\tif (!graph)\n \t\tdie(\"graph file %s does not exist\", graph_name);\n-\t}\n \n \tFREE_AND_NULL(graph_name);\n \n@@ -115,7 +114,7 @@ static int graph_read(int argc, const char **argv)\n \t\tprintf(\" large_edges\");\n \tprintf(\"\\n\");\n \n-\tfree_commit_graph(graph);\n+\tUNLEAK(graph);\n \n \treturn 0;\n }\n@@ -166,6 +165,8 @@ static int graph_write(int argc, const char **argv)\n \t\t\tpack_indexes = &lines;\n \t\tif (opts.stdin_commits)\n \t\t\tcommit_hex = &lines;\n+\n+\t\tUNLEAK(buf);\n \t}\n \n \twrite_commit_graph(opts.obj_dir,\n@@ -174,7 +175,7 @@ static int graph_write(int argc, const char **argv)\n \t\t\t opts.append,\n \t\t\t 1);\n \n-\tstring_list_clear(&lines, 0);\n+\tUNLEAK(lines);\n \treturn 0;\n }\n \n-- \ngitgitgadget\n\n"},{"id":"359556","messageId":"1002fd34fc57e929265d07ad6e6010116b27b7a9.1538586732.git.gitgitgadget@gmail.com","threadId":"49467","inReplyTo":"pull.42.v2.git.gitgitgadget@gmail.com","subject":"[PATCH v2 3/3] commit-graph: reduce initial oid allocation","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2018-10-03T17:12:19Z","receivedAt":"2018-10-03T17:12:21Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nWhile writing a commit-graph file, we store the full list of\ncommits in a flat list. We use this list for sorting and ensuring\nwe are closed under reachability.\n\nThe initial allocation assumed that (at most) one in four objects\nis a commit. This is a dramatic over-count for many repos,\nespecially large ones. Since we grow the repo dynamically, reduce\nthis count by a factor of eight. We still set it to a minimum of\n1024 before allocating.\n\nSigned-off-by: Derrick Stolee \n---\n commit-graph.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/commit-graph.c b/commit-graph.c\nindex ceca6026b0..e773703e1d 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -720,7 +720,7 @@ void write_commit_graph(const char *obj_dir,\n \tstruct progress *progress = NULL;\n \n \toids.nr = 0;\n-\toids.alloc = approximate_object_count() / 4;\n+\toids.alloc = approximate_object_count() / 32;\n \toids.progress = NULL;\n \toids.progress_done = 0;\n \n-- \ngitgitgadget\n"}]}