From: Stefan Sperling <stsp@stsp.name>
Subject: Re: memleak in commit_graph
To: Omar Polo <op@omarpolo.com>
Cc: gameoftrees@openbsd.org
Date: Mon, 5 Sep 2022 11:22:47 +0200

On Sun, Sep 04, 2022 at 09:34:48PM +0200, Omar Polo wrote:
> > @@ -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.

Indeed. We could use got_object_id_dup() to avoid this issue, something
like this should work (untested):

  	*id = got_object_id_dup(&node->id);
	if (*id == NULL)
		return got_error_from_errno("got_object_id_dup");
  	TAILQ_REMOVE(&graph->iter_list, node, entry);
	free(node);

Alternatively, we could return the node instead of node->id, allowing
the caller to free the node. But that would break the current API.

Regarding the other hunk in the patch you sent, it looks correct to me.
But I would prefer to skip add_node() altogether to avoid the need to
free its output node. See the patch below.

As a later change, add_node() could be inlined into add_branch_tip()
to simplify the code further.

diff /home/stsp/src/got
commit - 04666d1a54c25c8be7e39bc628b4a80f3376c127
path + /home/stsp/src/got
blob - ae6de06b406405893b822d52382ee6165539e3b3
file + lib/commit_graph.c
--- lib/commit_graph.c
+++ lib/commit_graph.c
@@ -213,8 +213,6 @@ packed_first_parent_traversal(int *ncommits_traversed,
 
 	/* Add all traversed commits to the graph... */
 	STAILQ_FOREACH(qid, &traversed_commits, entry) {
-		struct got_commit_graph_node *node;
-
 		if (got_object_idset_contains(graph->open_branches, &qid->id))
 			continue;
 		if (got_object_idset_contains(graph->node_ids, &qid->id))
@@ -229,7 +227,7 @@ packed_first_parent_traversal(int *ncommits_traversed,
 			break;
 		}
 
-		err = add_node(&node, graph, &qid->id, repo);
+		err = got_object_idset_add(graph->node_ids, &qid->id, NULL);
 		if (err)
 			break;
 	}