From: Omar Polo <op@omarpolo.com>
Subject: Re: memleak in commit_graph
To: Omar Polo <op@omarpolo.com>
Cc: Stefan Sperling <stsp@stsp.name>, gameoftrees@openbsd.org
Date: Sun, 04 Sep 2022 21:34:48 +0200

On 2022/09/04 20:19:18 +0200, Omar Polo <op@omarpolo.com> wrote:
> On 2022/09/04 16:55:39 +0200, Stefan Sperling <stsp@stsp.name> wrote:
> > On Sun, Sep 04, 2022 at 04:36:15PM +0200, Omar Polo wrote:
> > > I'm chasing a memleak in gotwebd' blame page and landed here.  A dummy
> > > commit graph loop seems to leak a bit so chances are that what I'm
> > > seeing in gotwebd are due to the commit graph loop (via blame.c)
> > 
> > > regress is happy.  OK/further ideas for leaks in here?
> > 
> > These changes look right and regress is happy indeed. ok stsp@
> 
> and here's the diff to plug the missing leaks in here.
> 
> My previous diff free'd the commit that we didn't push into the
> iter_list, but we need to eventually free also the item pushed into
> the list!  (see second hunk.)
> 
> Additionally, we were leaking a bit of memory from add_node (it
> returns a newly allocated struct) in packed_first_parent_traversal.
> 
> Both leaks spotted by valgrind.
> 
> With this valgrind is happy (for memleaks, it complains about some
> possible off-by-one, will get thru those warnings later) and my repro
> doesn't leak memory anymore.  I'm about to test this with gotwebd too.

i was a bit too over-confident.

> diff /home/op/w/got
> commit - 04666d1a54c25c8be7e39bc628b4a80f3376c127
> path + /home/op/w/got
> blob - ae6de06b406405893b822d52382ee6165539e3b3
> file + lib/commit_graph.c
> --- lib/commit_graph.c
> +++ lib/commit_graph.c
> @@ -232,6 +232,7 @@ packed_first_parent_traversal(int *ncommits_traversed,
>  		err = add_node(&node, graph, &qid->id, repo);
>  		if (err)
>  			break;
> +		free(node);
>  	}
>  
>  	got_object_id_queue_free(&traversed_commits);
> @@ -618,6 +619,7 @@ got_commit_graph_iter_next(struct got_object_id **id,
>  
>  	*id = &node->id;
>  	TAILQ_REMOVE(&graph->iter_list, node, entry);
> +	free(node);

this releases memory that's about to be used by the callers.  Need to
think a bit more how to fix this.

>  	return NULL;
>  }
>