From: Omar Polo <op@omarpolo.com>
Subject: memleak in commit_graph
To: gameoftrees@openbsd.org
Date: Sun, 04 Sep 2022 16:36:15 +0200

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)

With "dummy" loop I mean more or less:

	got_commit_graph_open(...);
	got_commit_graph_iter_start(...);
	for (;;) {
		err = got_commit_graph_iter_next(...);
		if (err)
			break;
	}
	got_commit_graph_close(graph);

ran in an infinite loop to spot leaks.

I managed to find something (the tailq leak fix in
got_commit_graph_close was actually suggested by Stefan on IRC) but
it's not the whole story, there are still leaks.  I'm sending what I
have for the moment.

in fetch_commits_from_open_branches we might leak some `new_node's
allocated in the arg struct.  In fact, if i'm reading the code
correctly, every time that we don't end up calling
add_node_to_iter_list we're leaking a node.

the change to add_branch_tip fixes a leak if add_node fails and
clarify one bit: if add_node succeeds new_node is not NULL so can drop
the ternary.

regress is happy.  OK/further ideas for leaks in here?


diff /home/op/w/got
commit - 442ede73eadb025cdc45bede186bf31aee869dad
path + /home/op/w/got
blob - 01dc8264285fe44769741f2870eda27d3d1e6b16
file + lib/commit_graph.c
--- lib/commit_graph.c
+++ lib/commit_graph.c
@@ -446,10 +446,12 @@ add_branch_tip(struct got_object_id *commit_id, void *
 		return err;
 
 	err = add_node(&new_node, a->graph, commit_id, a->repo);
-	if (err)
+	if (err) {
+		got_object_commit_close(commit);
 		return err;
+	}
 
-	a->tips[a->ntips].commit_id = new_node ? &new_node->id : NULL;
+	a->tips[a->ntips].commit_id = &new_node->id;
 	a->tips[a->ntips].commit = commit;
 	a->tips[a->ntips].new_node = new_node;
 	a->ntips++;
@@ -518,22 +520,33 @@ fetch_commits_from_open_branches(struct got_commit_gra
 				break;
 			continue;
 		}
-		if (changed)
+		if (changed) {
 			add_node_to_iter_list(graph, new_node,
 			    got_object_commit_get_committer_time(commit));
+			arg.tips[i].new_node = NULL;
+		}
 		err = advance_branch(graph, commit_id, commit, repo);
 		if (err)
 			break;
 	}
 done:
-	for (i = 0; i < arg.ntips; i++)
+	for (i = 0; i < arg.ntips; i++) {
 		got_object_commit_close(arg.tips[i].commit);
+		free(arg.tips[i].new_node);
+	}
 	return err;
 }
 
 void
 got_commit_graph_close(struct got_commit_graph *graph)
 {
+	struct got_commit_graph_node *node;
+
+	while ((node = TAILQ_FIRST(&graph->iter_list))) {
+		TAILQ_REMOVE(&graph->iter_list, node, entry);
+		free(node);
+	}
+
 	if (graph->open_branches)
 		got_object_idset_free(graph->open_branches);
 	if (graph->node_ids)