{"thread":{"id":"53392","subject":"[PATCH] commit-graph: avoid memory leaks","startedAt":"2020-05-04T19:13:33Z","lastAt":"2020-05-04T22:15:04Z","messageCount":4,"participants":["Jonathan Tan","Derrick Stolee","Junio C Hamano","Taylor Blau"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"396970","messageId":"20200504191324.201663-1-jonathantanmy@google.com","threadId":"53392","inReplyTo":null,"subject":"[PATCH] commit-graph: avoid memory leaks","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2020-05-04T19:13:24Z","receivedAt":"2020-05-04T19:13:33Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"A fuzzer running on the entry point provided by fuzz-commit-graph.c\nrevealed a memory leak when parse_commit_graph() creates a struct\nbloom_filter_settings and then returns early due to error. Fix that\nerror by always freeing that struct first (if it exists) before\nreturning early due to error.\n\nWhile making that change, I also noticed another possible memory leak -\nwhen the BLOOMDATA chunk is provided but not BLOOMINDEXES. Also fix that\nerror.\n\nSigned-off-by: Jonathan Tan \n---\nHere's a memory leak fix revealed by a fuzzer running at $DAYJOB, and\nanother one that I noticed while making that fix.\n---\n commit-graph.c | 22 +++++++++++-----------\n 1 file changed, 11 insertions(+), 11 deletions(-)\n\ndiff --git a/commit-graph.c b/commit-graph.c\nindex 6dc777e2f3..1694f0a691 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -281,8 +281,7 @@ struct commit_graph *parse_commit_graph(void *graph_map, size_t graph_size)\n \t\tif (data + graph_size - chunk_lookup <\n \t\t GRAPH_CHUNKLOOKUP_WIDTH) {\n \t\t\terror(_(\"commit-graph chunk lookup table entry missing; file may be incomplete\"));\n-\t\t\tfree(graph);\n-\t\t\treturn NULL;\n+\t\t\tgoto free_and_return;\n \t\t}\n \n \t\tchunk_id = get_be32(chunk_lookup + 0);\n@@ -293,8 +292,7 @@ struct commit_graph *parse_commit_graph(void *graph_map, size_t graph_size)\n \t\tif (chunk_offset > graph_size - the_hash_algo->rawsz) {\n \t\t\terror(_(\"commit-graph improper chunk offset %08x%08x\"), (uint32_t)(chunk_offset >> 32),\n \t\t\t (uint32_t)chunk_offset);\n-\t\t\tfree(graph);\n-\t\t\treturn NULL;\n+\t\t\tgoto free_and_return;\n \t\t}\n \n \t\tswitch (chunk_id) {\n@@ -361,8 +359,7 @@ struct commit_graph *parse_commit_graph(void *graph_map, size_t graph_size)\n \n \t\tif (chunk_repeated) {\n \t\t\terror(_(\"commit-graph chunk id %08x appears multiple times\"), chunk_id);\n-\t\t\tfree(graph);\n-\t\t\treturn NULL;\n+\t\t\tgoto free_and_return;\n \t\t}\n \n \t\tif (last_chunk_id == GRAPH_CHUNKID_OIDLOOKUP)\n@@ -381,17 +378,20 @@ struct commit_graph *parse_commit_graph(void *graph_map, size_t graph_size)\n \t\t/* We need both the bloom chunks to exist together. Else ignore the data */\n \t\tgraph->chunk_bloom_indexes = NULL;\n \t\tgraph->chunk_bloom_data = NULL;\n-\t\tgraph->bloom_filter_settings = NULL;\n+\t\tFREE_AND_NULL(graph->bloom_filter_settings);\n \t}\n \n \thashcpy(graph->oid.hash, graph->data + graph->data_len - graph->hash_len);\n \n-\tif (verify_commit_graph_lite(graph)) {\n-\t\tfree(graph);\n-\t\treturn NULL;\n-\t}\n+\tif (verify_commit_graph_lite(graph))\n+\t\tgoto free_and_return;\n \n \treturn graph;\n+\n+free_and_return:\n+\tfree(graph->bloom_filter_settings);\n+\tfree(graph);\n+\treturn NULL;\n }\n \n static struct commit_graph *load_commit_graph_one(const char *graph_file,\n-- \n2.26.2.526.g744177e7f7-goog\n\n"},{"id":"396980","messageId":"16be6de4-3453-2fc2-2881-7068bf674517@gmail.com","threadId":"53392","inReplyTo":"20200504191324.201663-1-jonathantanmy@google.com","subject":"Re: [PATCH] commit-graph: avoid memory leaks","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2020-05-04T20:20:41Z","receivedAt":"2020-05-04T20:20:48Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 5/4/2020 3:13 PM, Jonathan Tan wrote:\n> A fuzzer running on the entry point provided by fuzz-commit-graph.c\n> revealed a memory leak when parse_commit_graph() creates a struct\n> bloom_filter_settings and then returns early due to error. Fix that\n> error by always freeing that struct first (if it exists) before\n> returning early due to error.\n\nThis fuzzer is an excellent tool that I hope can continue to be\nextended to other features. Here is a case where it found an error in\na feature long after the integration was introduced.\n\n> While making that change, I also noticed another possible memory leak -\n> when the BLOOMDATA chunk is provided but not BLOOMINDEXES. Also fix that\n> error.\n\nThanks for discovering these issues, and for fixing them!\n\nReviewed-by: Derrick Stolee \n\nThanks,\n-Stolee\n"},{"id":"396988","messageId":"xmqq8si7flw0.fsf@gitster.c.googlers.com","threadId":"53392","inReplyTo":"20200504191324.201663-1-jonathantanmy@google.com","subject":"Re: [PATCH] commit-graph: avoid memory leaks","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-05-04T21:09:03Z","receivedAt":"2020-05-04T21:09:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan writes:\n\n> A fuzzer running on the entry point provided by fuzz-commit-graph.c\n> revealed a memory leak when parse_commit_graph() creates a struct\n> bloom_filter_settings and then returns early due to error. Fix that\n> error by always freeing that struct first (if it exists) before\n> returning early due to error.\n>\n> While making that change, I also noticed another possible memory leak -\n> when the BLOOMDATA chunk is provided but not BLOOMINDEXES. Also fix that\n> error.\n>\n> Signed-off-by: Jonathan Tan \n> ---\n> Here's a memory leak fix revealed by a fuzzer running at $DAYJOB, and\n> another one that I noticed while making that fix.\n> ---\n\nThanks. The patch looks good.\n\nWill queue.\n"},{"id":"397002","messageId":"20200504221500.GG45250@syl.local","threadId":"53392","inReplyTo":"16be6de4-3453-2fc2-2881-7068bf674517@gmail.com","subject":"Re: [PATCH] commit-graph: avoid memory leaks","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-05-04T22:15:00Z","receivedAt":"2020-05-04T22:15:04Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, May 04, 2020 at 04:20:41PM -0400, Derrick Stolee wrote:\n> On 5/4/2020 3:13 PM, Jonathan Tan wrote:\n> > A fuzzer running on the entry point provided by fuzz-commit-graph.c\n> > revealed a memory leak when parse_commit_graph() creates a struct\n> > bloom_filter_settings and then returns early due to error. Fix that\n> > error by always freeing that struct first (if it exists) before\n> > returning early due to error.\n>\n> This fuzzer is an excellent tool that I hope can continue to be\n> extended to other features. Here is a case where it found an error in\n> a feature long after the integration was introduced.\n>\n> > While making that change, I also noticed another possible memory leak -\n> > when the BLOOMDATA chunk is provided but not BLOOMINDEXES. Also fix that\n> > error.\n>\n> Thanks for discovering these issues, and for fixing them!\n>\n> Reviewed-by: Derrick Stolee \n\nSince it already appears to be queued, I'm not sure that my review is\nadding much. But, this fix looks very good to me, and I am grateful for\nyou running a fuzzer against this code, and finding/fixing bugs in it.\n\n Reviewed-by: Taylor Blau \n\n> Thanks,\n> -Stolee\n\nThanks,\nTaylor\n"}]}