"GOT", but the "O" is a cute, smiling pufferfish. Index | Thread | Search

From:
Omar Polo <op@omarpolo.com>
Subject:
gotwebd: plug memleaks in got_get_repo_commits
To:
gameoftrees@openbsd.org
Date:
Fri, 02 Sep 2022 12:50:28 +0200

Download raw body.

Thread
second attemp at fixing this.  For my own mental sanity and for who
will read this, I've split it into three separate commits to ease
reviewing.  The first two should be evident, the third one is a bit
more convoluted: it avoids calling two times got_get_repo_commit
(singular!) and leaking at every iteration the contents of
`repo_commit'.  Instead, drop the new_repo_commit var and only use
repo_commit.  To ease tracking the lifetime of it, immediately push it
into the tailq like got_get_repo_tags does.

ok?

-----------------------------------------------
commit 548422fdbb511de35fb644b9e8e5c6effbba76da
from: Omar Polo <op@omarpolo.com>
date: Fri Sep  2 10:42:30 2022 UTC
 
 gotwebd: always free ref in got_get_repo_commits
 
 some code-paths may leak it.
 
diff bc95141ca7ce90e4b19a251b36c87601c150bb3f 548422fdbb511de35fb644b9e8e5c6effbba76da
commit - bc95141ca7ce90e4b19a251b36c87601c150bb3f
commit + 548422fdbb511de35fb644b9e8e5c6effbba76da
blob - 4beb308778696b4c4299304065d186d28b072d3e
blob + dcc24dae3dae23f69341a447788cdf594fc9a613
--- gotwebd/got_operations.c
+++ gotwebd/got_operations.c
@@ -331,7 +331,7 @@ got_get_repo_commits(struct request *c, int limit)
 	struct got_commit_graph *graph = NULL;
 	struct got_commit_object *commit = NULL;
 	struct got_reflist_head refs;
-	struct got_reference *ref;
+	struct got_reference *ref = NULL;
 	struct repo_commit *repo_commit = NULL;
 	struct server *srv = c->srv;
 	struct transport *t = c->t;
@@ -373,7 +373,6 @@ got_get_repo_commits(struct request *c, int limit)
 			goto done;
 
 		error = got_ref_resolve(&id, repo, ref);
-		got_ref_close(ref);
 		if (error)
 			goto done;
 	} else if (qs->commit != NULL) {
@@ -383,7 +382,6 @@ got_get_repo_commits(struct request *c, int limit)
 			if (error)
 				goto done;
 			error = got_object_get_type(&obj_type, repo, id);
-			got_ref_close(ref);
 			if (error)
 				goto done;
 			if (obj_type == GOT_OBJ_TYPE_TAG) {
@@ -545,6 +543,8 @@ got_get_repo_commits(struct request *c, int limit)
 	}
 done:
 	gotweb_free_repo_commit(repo_commit);
+	if (ref)
+		got_ref_close(ref);
 	if (commit)
 		got_object_commit_close(commit);
 	if (graph)

-----------------------------------------------
commit b63a0a6de295765dcd906255f2d2d2bff35fc4f4
from: Omar Polo <op@omarpolo.com>
date: Fri Sep  2 10:42:30 2022 UTC
 
 gotwebd: free in_repo_path in got_get_repo_commits
 
diff 548422fdbb511de35fb644b9e8e5c6effbba76da b63a0a6de295765dcd906255f2d2d2bff35fc4f4
commit - 548422fdbb511de35fb644b9e8e5c6effbba76da
commit + b63a0a6de295765dcd906255f2d2d2bff35fc4f4
blob - dcc24dae3dae23f69341a447788cdf594fc9a613
blob + a2964284b0746b56e407422c9347f61ca66b475d
--- gotwebd/got_operations.c
+++ gotwebd/got_operations.c
@@ -550,6 +550,7 @@ done:
 	if (graph)
 		got_commit_graph_close(graph);
 	got_ref_list_free(&refs);
+	free(in_repo_path);
 	free(file_path);
 	free(repo_path);
 	free(id);

-----------------------------------------------
commit 1089328056b8e8e6d9fb0a80575c437721ceb542 (main)
from: Omar Polo <op@omarpolo.com>
date: Fri Sep  2 10:42:39 2022 UTC
 
 gotwebd: plugs leaks in got_get_repo_commits
 
 call got_get_repo_commit only once and avoid leaking the field of
 repo_commit at each loop iteration.
 
diff b63a0a6de295765dcd906255f2d2d2bff35fc4f4 1089328056b8e8e6d9fb0a80575c437721ceb542
commit - b63a0a6de295765dcd906255f2d2d2bff35fc4f4
commit + 1089328056b8e8e6d9fb0a80575c437721ceb542
blob - a2964284b0746b56e407422c9347f61ca66b475d
blob + 5ea76c0eefd10ebe0ddf57279275a15c27edd525
--- gotwebd/got_operations.c
+++ gotwebd/got_operations.c
@@ -355,10 +355,6 @@ got_get_repo_commits(struct request *c, int limit)
 		goto done;
 	}
 
-	error = got_init_repo_commit(&repo_commit);
-	if (error)
-		goto done;
-
 	/*
 	 * XXX: jumping directly to a commit id via
 	 * got_repo_match_object_id_prefix significantly improves performance,
@@ -439,7 +435,8 @@ got_get_repo_commits(struct request *c, int limit)
 
 	for (;;) {
 		if (limit_chk == ((limit * qs->page) - (limit - 1)) &&
-		    commit_found == 0 && repo_commit->commit_id != NULL) {
+		    commit_found == 0 && repo_commit &&
+		    repo_commit->commit_id != NULL) {
 			t->prev_id = strdup(repo_commit->commit_id);
 			if (t->prev_id == NULL) {
 				error = got_error_from_errno("strdup");
@@ -466,6 +463,12 @@ got_get_repo_commits(struct request *c, int limit)
 		if (error)
 			goto done;
 
+		error = got_init_repo_commit(&repo_commit);
+		if (error)
+			goto done;
+
+		TAILQ_INSERT_TAIL(&t->repo_commits, repo_commit, entry);
+
 		error = got_get_repo_commit(c, repo_commit, commit,
 		    &refs, id);
 		if (error)
@@ -485,18 +488,6 @@ got_get_repo_commits(struct request *c, int limit)
 			}
 		}
 
-		struct repo_commit *new_repo_commit = NULL;
-		error = got_init_repo_commit(&new_repo_commit);
-		if (error)
-			goto done;
-
-		TAILQ_INSERT_TAIL(&t->repo_commits, new_repo_commit, entry);
-
-		error = got_get_repo_commit(c, new_repo_commit, commit,
-		    &refs, id);
-		if (error)
-			goto done;
-
 		free(id);
 		id = NULL;
 
@@ -513,7 +504,7 @@ got_get_repo_commits(struct request *c, int limit)
 			 */
 			if (chk_next && (qs->action == BRIEFS ||
 			    qs->action == COMMITS || qs->action == SUMMARY)) {
-				t->next_id = strdup(new_repo_commit->commit_id);
+				t->next_id = strdup(repo_commit->commit_id);
 				if (t->next_id == NULL) {
 					error = got_error_from_errno("strdup");
 					goto done;
@@ -522,9 +513,9 @@ got_get_repo_commits(struct request *c, int limit)
 					got_object_commit_close(commit);
 					commit = NULL;
 				}
-				TAILQ_REMOVE(&t->repo_commits, new_repo_commit,
+				TAILQ_REMOVE(&t->repo_commits, repo_commit,
 				    entry);
-				gotweb_free_repo_commit(new_repo_commit);
+				gotweb_free_repo_commit(repo_commit);
 				goto done;
 			}
 		}
@@ -542,7 +533,6 @@ got_get_repo_commits(struct request *c, int limit)
 		}
 	}
 done:
-	gotweb_free_repo_commit(repo_commit);
 	if (ref)
 		got_ref_close(ref);
 	if (commit)