{"thread":{"id":"51044","subject":"[PATCH] commit-graph: fix memory leak","startedAt":"2019-05-06T21:37:02Z","lastAt":"2019-05-07T22:26:05Z","messageCount":5,"participants":["Josh Steadmon","Emily Shaffer","Derrick Stolee","Ævar Arnfjörð Bjarmason","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"374958","messageId":"f4ab2a50873b2fd91926d7401f784479504d1b10.1557178485.git.steadmon@google.com","threadId":"51044","inReplyTo":null,"subject":"[PATCH] commit-graph: fix memory leak","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2019-05-06T21:36:58Z","receivedAt":"2019-05-06T21:37:02Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"Free the commit graph when verify_commit_graph_lite() reports an error.\nCredit to OSS-Fuzz for finding this leak.\n\nSigned-off-by: Josh Steadmon \n---\n commit-graph.c | 4 +++-\n 1 file changed, 3 insertions(+), 1 deletion(-)\n\ndiff --git a/commit-graph.c b/commit-graph.c\nindex 66865acbd7..4bce70d35c 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -267,8 +267,10 @@ struct commit_graph *parse_commit_graph(void *graph_map, int fd,\n \t\tlast_chunk_offset = chunk_offset;\n \t}\n \n-\tif (verify_commit_graph_lite(graph))\n+\tif (verify_commit_graph_lite(graph)) {\n+\t\tfree(graph);\n \t\treturn NULL;\n+\t}\n \n \treturn graph;\n }\n-- \n2.21.0.1020.gf2820cf01a-goog\n\n"},{"id":"374959","messageId":"20190506215831.GB85211@google.com","threadId":"51044","inReplyTo":"f4ab2a50873b2fd91926d7401f784479504d1b10.1557178485.git.steadmon@google.com","subject":"Re: [PATCH] commit-graph: fix memory leak","fromName":"Emily Shaffer","fromEmail":"emilyshaffer@google.com","sentAt":"2019-05-06T21:58:31Z","receivedAt":"2019-05-06T21:58:38Z","isPatch":true,"sender":{"key":"nasamuffin@google.com","avatar":"https://avatars.githubusercontent.com/u/1606826?v=4"},"body":"Hi,\n\nThis change looks good to me, and like good evidence for the benefits of\nautomated tooling :)\n\nThanks!\n - Emily\n\nOn Mon, May 06, 2019 at 02:36:58PM -0700, Josh Steadmon wrote:\n> Free the commit graph when verify_commit_graph_lite() reports an error.\n> Credit to OSS-Fuzz for finding this leak.\n> \n> Signed-off-by: Josh Steadmon \n> ---\n> commit-graph.c | 4 +++-\n> 1 file changed, 3 insertions(+), 1 deletion(-)\n> \n> diff --git a/commit-graph.c b/commit-graph.c\n> index 66865acbd7..4bce70d35c 100644\n> --- a/commit-graph.c\n> +++ b/commit-graph.c\n> @@ -267,8 +267,10 @@ struct commit_graph *parse_commit_graph(void *graph_map, int fd,\n> \t\tlast_chunk_offset = chunk_offset;\n> \t}\n> \n> -\tif (verify_commit_graph_lite(graph))\n> +\tif (verify_commit_graph_lite(graph)) {\n> +\t\tfree(graph);\n> \t\treturn NULL;\n> +\t}\n> \n> \treturn graph;\n> }\n> -- \n> 2.21.0.1020.gf2820cf01a-goog\n> \n"},{"id":"374974","messageId":"56cd0368-1011-f06a-770c-f62b138e8627@gmail.com","threadId":"51044","inReplyTo":"20190506215831.GB85211@google.com","subject":"Re: [PATCH] commit-graph: fix memory leak","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2019-05-07T01:58:18Z","receivedAt":"2019-05-07T01:58:27Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 5/6/2019 5:58 PM, Emily Shaffer wrote:\n> Hi,\n> \n> This change looks good to me, and like good evidence for the benefits of\n> automated tooling :)\n\nSame here! Keep up the great work here.\n\n-Stolee\n"},{"id":"375004","messageId":"87zhnyh9vu.fsf@evledraar.gmail.com","threadId":"51044","inReplyTo":"f4ab2a50873b2fd91926d7401f784479504d1b10.1557178485.git.steadmon@google.com","subject":"Re: [PATCH] commit-graph: fix memory leak","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2019-05-07T09:49:41Z","receivedAt":"2019-05-07T09:49:46Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, May 06 2019, Josh Steadmon wrote:\n\n> Free the commit graph when verify_commit_graph_lite() reports an error.\n> Credit to OSS-Fuzz for finding this leak.\n>\n> Signed-off-by: Josh Steadmon \n> ---\n> commit-graph.c | 4 +++-\n> 1 file changed, 3 insertions(+), 1 deletion(-)\n>\n> diff --git a/commit-graph.c b/commit-graph.c\n> index 66865acbd7..4bce70d35c 100644\n> --- a/commit-graph.c\n> +++ b/commit-graph.c\n> @@ -267,8 +267,10 @@ struct commit_graph *parse_commit_graph(void *graph_map, int fd,\n> \t\tlast_chunk_offset = chunk_offset;\n> \t}\n>\n> -\tif (verify_commit_graph_lite(graph))\n> +\tif (verify_commit_graph_lite(graph)) {\n> +\t\tfree(graph);\n> \t\treturn NULL;\n> +\t}\n>\n> \treturn graph;\n> }\n\nThis is obviously correct, FWIW the leak was there before the\nverify_commit_graph_lite() refactoring I did, but I read the rest of the\nsurrounding code (but haven't run valgrind etc.) and it seems to be the\nonly one.\n\nI wonder in general if there's a more sustainable solution to these\none-at-a-time memory leak fixes we're doing to these\nlibraries. E.g. marking some tests in the test suite as passing cleanly\nwith valgrind's leak checker, and adding a test mode to run those tests.\n"},{"id":"375063","messageId":"20190507222601.GA976@sigill.intra.peff.net","threadId":"51044","inReplyTo":"87zhnyh9vu.fsf@evledraar.gmail.com","subject":"Re: [PATCH] commit-graph: fix memory leak","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-05-07T22:26:01Z","receivedAt":"2019-05-07T22:26:05Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, May 07, 2019 at 11:49:41AM +0200, Ævar Arnfjörð Bjarmason wrote:\n\n> I wonder in general if there's a more sustainable solution to these\n> one-at-a-time memory leak fixes we're doing to these\n> libraries. E.g. marking some tests in the test suite as passing cleanly\n> with valgrind's leak checker, and adding a test mode to run those tests.\n\nI'd recommend going with the LeakSanitizer, since the resulting tests\nrun a lot faster. We made some progress a while ago, and some tests do\npass, but there's a lot of manual inspection (and either fixing leaks,\nor annotating with UNLEAK as appropriate) still to do.\n\nRunning \"make SANITIZE=leak test\" shows our current state.\n\nIf we just want to stop the bleeding, so to speak, I suspect that rather\nthan marking individual tests as \"clean\", we'd do better to collect all\nof the results, sort and remove duplicates, and then just compare the\nresult before and after certain branches. That would tell us the new\nleaks being added.\n\nSomething like:\n\n export LSAN_OPTIONS=exitcode=0:log_path=/tmp/lsan\n make SANITIZE=leak test\n\nshould dump a bunch of files in /tmp. (Note that when we tried this in\nlate 2017, log_path did not seem to work in pure-LSan mode, but I think\nthis was a bug; it works fine for me now).\n\nCollating the results is a little tricky, because the top of the stack\nwhen the leak was allocated is usually uninteresting (it's almost always\nxmalloc).\n\nThere's some discussion and some scripts in:\n\n https://public-inbox.org/git/20170923163817.7ltmkav2ytk7n43k@sigill.intra.peff.net/\n\nand\n\n https://public-inbox.org/git/20170925160835.aoomjaqrn2o2aosi@sigill.intra.peff.net/\n\nI think just pumping the results of the second one through \"sort -u\"\nwould get you a starting point that you could use for before/after\ndiffs.\n\n-Peff\n"}]}