{"thread":{"id":"53621","subject":"[PATCH] fuzz-commit-graph: properly free graph struct","startedAt":"2020-06-05T22:55:18Z","lastAt":"2020-06-05T23:30:28Z","messageCount":3,"participants":["Josh Steadmon","Jonathan Nieder"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"399219","messageId":"35063a0ab4edbaa5bd5b0138e6a6a5b36a8664c5.1591397562.git.steadmon@google.com","threadId":"53621","inReplyTo":null,"subject":"[PATCH] fuzz-commit-graph: properly free graph struct","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2020-06-05T22:55:14Z","receivedAt":"2020-06-05T22:55:18Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"Use the provided free_commit_graph() to properly free the commit graph\nin fuzz-commit-graph. Otherwise, the fuzzer itself leaks memory when the\nstruct contains pointers to allocated memory.\n\nSigned-off-by: Josh Steadmon \n---\n fuzz-commit-graph.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/fuzz-commit-graph.c b/fuzz-commit-graph.c\nindex 9fd1c04edd..430817214d 100644\n--- a/fuzz-commit-graph.c\n+++ b/fuzz-commit-graph.c\n@@ -12,7 +12,7 @@ int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size)\n \tinitialize_the_repository();\n \tg = parse_commit_graph((void *)data, size);\n \trepo_clear(the_repository);\n-\tfree(g);\n+\tfree_commit_graph(g);\n \n \treturn 0;\n }\n-- \n2.27.0.278.ge193c7cf3a9-goog\n\n"},{"id":"399220","messageId":"20200605230233.GA167014@google.com","threadId":"53621","inReplyTo":"35063a0ab4edbaa5bd5b0138e6a6a5b36a8664c5.1591397562.git.steadmon@google.com","subject":"Re: [PATCH] fuzz-commit-graph: properly free graph struct","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2020-06-05T23:02:33Z","receivedAt":"2020-06-05T23:02:38Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Josh Steadmon wrote:\n\n> Use the provided free_commit_graph() to properly free the commit graph\n> in fuzz-commit-graph. Otherwise, the fuzzer itself leaks memory when the\n> struct contains pointers to allocated memory.\n>\n> Signed-off-by: Josh Steadmon \n> ---\n> fuzz-commit-graph.c | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n\nHow can I reproduce this?\n\n> diff --git a/fuzz-commit-graph.c b/fuzz-commit-graph.c\n> index 9fd1c04edd..430817214d 100644\n> --- a/fuzz-commit-graph.c\n> +++ b/fuzz-commit-graph.c\n> @@ -12,7 +12,7 @@ int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size)\n> \tinitialize_the_repository();\n> \tg = parse_commit_graph((void *)data, size);\n> \trepo_clear(the_repository);\n> -\tfree(g);\n> +\tfree_commit_graph(g);\n\nIn any event, the patch itself is sensible, so\nReviewed-by: Jonathan Nieder \n\nThanks.\n"},{"id":"399223","messageId":"20200605233019.GC65111@google.com","threadId":"53621","inReplyTo":"20200605230233.GA167014@google.com","subject":"Re: [PATCH] fuzz-commit-graph: properly free graph struct","fromName":"Josh Steadmon","fromEmail":"steadmon@google.com","sentAt":"2020-06-05T23:30:19Z","receivedAt":"2020-06-05T23:30:28Z","isPatch":true,"sender":{"key":"steadmon@google.com","avatar":"https://avatars.githubusercontent.com/u/2654920?v=4"},"body":"On 2020.06.05 16:02, Jonathan Nieder wrote:\n> Josh Steadmon wrote:\n> \n> > Use the provided free_commit_graph() to properly free the commit graph\n> > in fuzz-commit-graph. Otherwise, the fuzzer itself leaks memory when the\n> > struct contains pointers to allocated memory.\n> >\n> > Signed-off-by: Josh Steadmon \n> > ---\n> > fuzz-commit-graph.c | 2 +-\n> > 1 file changed, 1 insertion(+), 1 deletion(-)\n> \n> How can I reproduce this?\n\nBuilding the fuzzer is dependent on the exact versions of clang &\nlibFuzzer that you have installed; on my machine, with clang 9.0.1-10\nand libFuzzer 9, I build as follows:\n\n$ make CC=clang \\\n CXX=clang++ \\\n CFLAGS=\"-fsanitize=fuzzer-no-link,address\" \\\n LIB_FUZZING_ENGINE=/usr/lib/llvm-9/lib/libFuzzer.a \\\n fuzz-all\n\nThen you can run fuzz-commit-graph on the attached test case:\n\n$ ./fuzz-commit-graph /tmp/testcase-fuzzer-leak\n\nWhen built from master, I get a leak error:\n\n\n$ ./fuzz-commit-graph /tmp/testcase-fuzzer-leak\nINFO: Seed: 2332984289\nINFO: Loaded 1 modules (70798 inline 8-bit counters): 70798 [0x15c8b82, 0x15da010), \nINFO: Loaded 1 PC tables (70798 PCs): 70798 [0x12a7db8,0x13bc698), \n./fuzz-commit-graph: Running 1 inputs 1 time(s) each.\nRunning: /tmp/testcase-5725798091980800\n\n=================================================================\n==192153==ERROR: LeakSanitizer: detected memory leaks\n\nDirect leak of 12 byte(s) in 1 object(s) allocated from:\n #0 0x49be6d in malloc (/usr/local/google/home/steadmon/src/git/fuzz-commit-graph+0x49be6d)\n #1 0xc2ff15 in do_xmalloc (/usr/local/google/home/steadmon/src/git/fuzz-commit-graph+0xc2ff15)\n #2 0xc2fe66 in xmalloc (/usr/local/google/home/steadmon/src/git/fuzz-commit-graph+0xc2fe66)\n #3 0x5cfc4e in parse_commit_graph (/usr/local/google/home/steadmon/src/git/fuzz-commit-graph+0x5cfc4e)\n #4 0x1085b39 in LLVMFuzzerTestOneInput (/usr/local/google/home/steadmon/src/git/fuzz-commit-graph+0x1085b39)\n #5 0x109b41c in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) (/usr/local/google/home/steadmon/src/git/fuzz-commit-graph+0x109b41c)\n\nSUMMARY: AddressSanitizer: 12 byte(s) leaked in 1 allocation(s).\n\nINFO: a leak has been found in the initial corpus.\n\nINFO: to ignore leaks on libFuzzer side use -detect_leaks=0.\n\n\nWhen run with this patch, the fuzzer does not report a leak.\n\n\n> > diff --git a/fuzz-commit-graph.c b/fuzz-commit-graph.c\n> > index 9fd1c04edd..430817214d 100644\n> > --- a/fuzz-commit-graph.c\n> > +++ b/fuzz-commit-graph.c\n> > @@ -12,7 +12,7 @@ int LLVMFuzzerTestOneInput(const uint8_t *data, size_t size)\n> > \tinitialize_the_repository();\n> > \tg = parse_commit_graph((void *)data, size);\n> > \trepo_clear(the_repository);\n> > -\tfree(g);\n> > +\tfree_commit_graph(g);\n> \n> In any event, the patch itself is sensible, so\n> Reviewed-by: Jonathan Nieder \n> \n> Thanks.\n"}]}