Re: [PATCH] commit-graph: fix memory leak
- From
Ævar Arnfjörð Bjarmason <avarab@gmail.com>
- Date
- May 7, 2019, 09:49 UTC
- Message-ID
- <87zhnyh9vu.fsf@evledraar.gmail.com>
- In-Reply-To
- <f4ab2a50873b2fd91926d7401f784479504d1b10.1557178485.git.steadmon@google.com>
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.