On 2022/09/05 11:22:47 +0200, Stefan Sperling <stsp@stsp.name> wrote:
> 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);

yes and no.  The current callers assume that they don't need to free
the id returned and that it's safe to store it for some time.  Adding
a got_object_id_dup there would only move the leak from the node to
the id.

Diff below is an improved version.  It fixes the leak and fixes some
of the consumer and passes a full regress run \o/ (this time i
remembered to `make regress' before sending the diff!)

The idea now is that when calling got_commit_graph_iter_next the
returned pointer is safe until the next call to it.  Most callers were
already doing that, so this is not an invasive change.

For a future improvement (post-release) would be fine to make the
callers provide the storage for the got_object_id that iter_next
returns?

(the bit for gotwebd will be committed separatedly: gotwebd reuses the
same variable to store an allocated object id *and* the iter_next
result.)

diff /home/op/w/got
commit - 9ea55f08a2fdb3e7018231c9fe4014c758a0b69a
path + /home/op/w/got
blob - 1877932f7650a337705d19c325c27b9f594e54bf
file + got/got.c
--- got/got.c
+++ got/got.c
@@ -9708,6 +9708,7 @@ collect_commits(struct got_object_id_queue *commits,
 	struct got_object_id *parent_id = NULL;
 	struct got_object_qid *qid;
         struct got_object_id *commit_id = initial_commit_id;
+	struct got_object_id *tmp = NULL;
 
 	err = got_commit_graph_open(&graph, "/", 1);
 	if (err)
@@ -9738,10 +9739,19 @@ collect_commits(struct got_object_id_queue *commits,
 			if (err)
 				goto done;
 			STAILQ_INSERT_HEAD(commits, qid, entry);
-			commit_id = parent_id;
+
+			free(tmp);
+			tmp = got_object_id_dup(parent_id);
+			if (tmp == NULL) {
+				err = got_error_from_errno(
+				    "got_object_id_dup");
+				goto done;
+			}
+			commit_id = tmp;
 		}
 	}
 done:
+	free(tmp);
 	got_commit_graph_close(graph);
 	return err;
 }
blob - 76816f00e1861046d23bfcd7dc01706f222e7f5c
file + gotwebd/got_operations.c
--- gotwebd/got_operations.c
+++ gotwebd/got_operations.c
@@ -434,6 +434,8 @@ got_get_repo_commits(struct request *c, int limit)
 		goto done;
 
 	for (;;) {
+		struct got_object_id *next_id;
+
 		if (limit_chk == ((limit * qs->page) - (limit - 1)) &&
 		    commit_found == 0 && repo_commit &&
 		    repo_commit->commit_id != NULL) {
@@ -444,17 +446,15 @@ got_get_repo_commits(struct request *c, int limit)
 			}
 		}
 
-		error = got_commit_graph_iter_next(&id, graph, repo, NULL,
+		error = got_commit_graph_iter_next(&next_id, graph, repo, NULL,
 		    NULL);
 		if (error) {
 			if (error->code == GOT_ERR_ITER_COMPLETED)
 				error = NULL;
 			goto done;
 		}
-		if (id == NULL)
-			goto done;
 
-		error = got_object_open_as_commit(&commit, repo, id);
+		error = got_object_open_as_commit(&commit, repo, next_id);
 		if (error)
 			goto done;
 
@@ -470,7 +470,7 @@ got_get_repo_commits(struct request *c, int limit)
 		TAILQ_INSERT_TAIL(&t->repo_commits, repo_commit, entry);
 
 		error = got_get_repo_commit(c, repo_commit, commit,
-		    &refs, id);
+		    &refs, next_id);
 		if (error)
 			goto done;
 
@@ -482,15 +482,10 @@ got_get_repo_commits(struct request *c, int limit)
 				commit_found = 1;
 			else {
 				limit_chk++;
-				free(id);
-				id = NULL;
 				continue;
 			}
 		}
 
-		free(id);
-		id = NULL;
-
 		if (limit == 1 && chk_multi == 0 &&
 		    srv->max_commits_display != 1)
 			commit_found = 1;
blob - c11280e036c43527546ff7dea1fdf989ad7cf971
file + lib/blame.c
--- lib/blame.c
+++ lib/blame.c
@@ -595,7 +595,12 @@ blame_open(struct got_blame **blamep, const char *path
 			goto done;
 		}
 		if (next_id) {
-			id = next_id;
+			free(id);
+			id = got_object_id_dup(next_id);
+			if (id == NULL) {
+				err = got_error_from_errno("got_object_id_dup");
+				goto done;
+			}
 			err = blame_commit(blame, id, path, repo, cb, arg);
 			if (err) {
 				if (err->code == GOT_ERR_ITER_COMPLETED)
@@ -628,6 +633,7 @@ done:
 	if (graph)
 		got_commit_graph_close(graph);
 	free(obj_id);
+	free(id);
 	if (blob)
 		got_object_blob_close(blob);
 	if (start_commit)
blob - 793795838b82267e63c1ce81996fa76ad63f3860
file + lib/commit_graph.c
--- lib/commit_graph.c
+++ lib/commit_graph.c
@@ -90,6 +90,12 @@ struct got_commit_graph {
 	 * commit timestmap.
 	 */
 	struct got_commit_graph_iter_list iter_list;
+
+	/*
+	 * Temporary storage for the id returned by
+	 * got_commit_graph_iter_next.
+	 */
+	struct got_object_id id;
 };
 
 static const struct got_error *
@@ -614,8 +620,11 @@ got_commit_graph_iter_next(struct got_object_id **id,
 			return err;
 	}
 
-	*id = &node->id;
+	memcpy(&graph->id, &node->id, sizeof(graph->id));
+	*id = &graph->id;
+
 	TAILQ_REMOVE(&graph->iter_list, node, entry);
+	free(node);
 	return NULL;
 }