threads / patch / 51044

patchcommit-graph: fix memory leak

Subject: [PATCH] commit-graph: fix memory leak

## tl;dr

5 messages between May 6, 2019 and May 7, 2019. Diffs are folded; open one to read it.

replies: 4people: 5as markdown or json

Josh Steadmon· May 6, 2019, 21:36 UTC · lore

Free the commit graph when verify_commit_graph_lite() reports an error. Credit to OSS-Fuzz for finding this leak.

Signed-off-by: Josh Steadmon <steadmon@google.com>
---
 commit-graph.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
Show changes to commit-graph.c +3 −1
diff --git a/commit-graph.c b/commit-graph.c
index 66865acbd7..4bce70d35c 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -267,8 +267,10 @@ struct commit_graph *parse_commit_graph(void *graph_map, int fd,
 		last_chunk_offset = chunk_offset;
 	}
 
-	if (verify_commit_graph_lite(graph))
+	if (verify_commit_graph_lite(graph)) {
+		free(graph);
 		return NULL;
+	}
 
 	return graph;
 }

-- 
2.21.0.1020.gf2820cf01a-goog
Emily Shaffer· May 6, 2019, 21:58 UTC · re: Josh Steadmon · lore

Re: [PATCH] commit-graph: fix memory leak

Hi,

This change looks good to me, and like good evidence for the benefits of automated tooling :)

Thanks!
 - Emily
On Mon, May 06, 2019 at 02:36:58PM -0700, Josh Steadmon wrote:
Show 27 quoted lines
> Free the commit graph when verify_commit_graph_lite() reports an error.
> Credit to OSS-Fuzz for finding this leak.
> 
> Signed-off-by: Josh Steadmon <steadmon@google.com>
> ---
>  commit-graph.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/commit-graph.c b/commit-graph.c
> index 66865acbd7..4bce70d35c 100644
> --- a/commit-graph.c
> +++ b/commit-graph.c
> @@ -267,8 +267,10 @@ struct commit_graph *parse_commit_graph(void *graph_map, int fd,
>  		last_chunk_offset = chunk_offset;
>  	}
>  
> -	if (verify_commit_graph_lite(graph))
> +	if (verify_commit_graph_lite(graph)) {
> +		free(graph);
>  		return NULL;
> +	}
>  
>  	return graph;
>  }
> -- 
> 2.21.0.1020.gf2820cf01a-goog
>
Derrick Stolee· May 7, 2019, 01:58 UTC · re: Emily Shaffer · lore

Re: [PATCH] commit-graph: fix memory leak

On 5/6/2019 5:58 PM, Emily Shaffer wrote:
> Hi,
> 
> This change looks good to me, and like good evidence for the benefits of
> automated tooling :)
Same here! Keep up the great work here.
-Stolee
Ævar Arnfjörð Bjarmason· May 7, 2019, 09:49 UTC · re: Josh Steadmon · lore

Re: [PATCH] commit-graph: fix memory leak

On Mon, May 06 2019, Josh Steadmon wrote:
Show 24 quoted lines
> Free the commit graph when verify_commit_graph_lite() reports an error.
> Credit to OSS-Fuzz for finding this leak.
>
> Signed-off-by: Josh Steadmon <steadmon@google.com>
> ---
>  commit-graph.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/commit-graph.c b/commit-graph.c
> index 66865acbd7..4bce70d35c 100644
> --- a/commit-graph.c
> +++ b/commit-graph.c
> @@ -267,8 +267,10 @@ struct commit_graph *parse_commit_graph(void *graph_map, int fd,
>  		last_chunk_offset = chunk_offset;
>  	}
>
> -	if (verify_commit_graph_lite(graph))
> +	if (verify_commit_graph_lite(graph)) {
> +		free(graph);
>  		return NULL;
> +	}
>
>  	return graph;
>  }

This is obviously correct, FWIW the leak was there before the verify_commit_graph_lite() refactoring I did, but I read the rest of the surrounding code (but haven't run valgrind etc.) and it seems to be the only one.

I wonder in general if there's a more sustainable solution to these one-at-a-time memory leak fixes we're doing to these libraries. E.g. marking some tests in the test suite as passing cleanly with valgrind's leak checker, and adding a test mode to run those tests.

Jeff King· May 7, 2019, 22:26 UTC · re: Ævar Arnfjörð Bjarmason · lore

Re: [PATCH] commit-graph: fix memory leak

On Tue, May 07, 2019 at 11:49:41AM +0200, Ævar Arnfjörð Bjarmason wrote:
> I wonder in general if there's a more sustainable solution to these
> one-at-a-time memory leak fixes we're doing to these
> libraries. E.g. marking some tests in the test suite as passing cleanly
> with valgrind's leak checker, and adding a test mode to run those tests.

I'd recommend going with the LeakSanitizer, since the resulting tests run a lot faster. We made some progress a while ago, and some tests do pass, but there's a lot of manual inspection (and either fixing leaks, or annotating with UNLEAK as appropriate) still to do.

Running "make SANITIZE=leak test" shows our current state.

If we just want to stop the bleeding, so to speak, I suspect that rather than marking individual tests as "clean", we'd do better to collect all of the results, sort and remove duplicates, and then just compare the result before and after certain branches. That would tell us the new leaks being added.

Something like:
  export LSAN_OPTIONS=exitcode=0:log_path=/tmp/lsan
  make SANITIZE=leak test

should dump a bunch of files in /tmp. (Note that when we tried this in late 2017, log_path did not seem to work in pure-LSan mode, but I think this was a bug; it works fine for me now).

Collating the results is a little tricky, because the top of the stack when the leak was allocated is usually uninteresting (it's almost always xmalloc).

There's some discussion and some scripts in:
  https://public-inbox.org/git/20170923163817.7ltmkav2ytk7n43k@sigill.intra.peff.net/
and
  https://public-inbox.org/git/20170925160835.aoomjaqrn2o2aosi@sigill.intra.peff.net/

I think just pumping the results of the second one through "sort -u" would get you a starting point that you could use for before/after diffs.

-Peff

← back to recent threads