From: Omar Polo <op@omarpolo.com>
Subject: gotwebd: plug leak regarding repo_commit (plus bonus)
To: gameoftrees@openbsd.org
Date: Wed, 31 Aug 2022 23:24:36 +0200

The `repo_tag' structure are not properly free'd.  The tag_name and
tag_commit field are left leaking.  How tag_commit is populated in
got_get_repo_tags needs some tweaking too.

While here I'm bundling as a bonus ;-) some other minor tweaks spotted
while hunting for memleaks.  There are a couple of `return error' that
leaks memory on error (unlikely), a tautological s[strlen(s)] == '\0'
dropped, a few duplicate checks remove (the t->next_id == NULL) and a
lone if (error) when it's always NULL too.  In gotweb_free_transport
I'm taking the chance to drop an unneeded NULL check of t.

ok?

diff /home/op/w/got
commit - b765b7a889c2e1d41b6b0d21118c549fdc2b969e
path + /home/op/w/got
blob - 85594b289ee57c52802a8fa7745f70d2a9bb3e77
file + gotwebd/got_operations.c
--- gotwebd/got_operations.c
+++ gotwebd/got_operations.c
@@ -227,8 +227,7 @@ got_get_repo_commit(struct request *c, struct repo_com
 			name += 6;
 		if (strncmp(name, "remotes/", 8) == 0) {
 			name += 8;
-			s = strstr(name, "/" GOT_REF_HEAD);
-			if (s != NULL && s[strlen(s)] == '\0')
+			if (strstr(name, "/" GOT_REF_HEAD) != NULL)
 				continue;
 		}
 		error = got_ref_resolve(&ref_id, t->repo, re->ref);
@@ -351,12 +350,14 @@ got_get_repo_commits(struct request *c, int limit)
 			return got_error_from_errno("asprintf");
 
 	if (asprintf(&repo_path, "%s/%s", srv->repos_path,
-	    repo_dir->name) == -1)
-		return got_error_from_errno("asprintf");
+	    repo_dir->name) == -1) {
+		error = got_error_from_errno("asprintf");
+		goto done;
+	}
 
 	error = got_init_repo_commit(&repo_commit);
 	if (error)
-		return error;
+		goto done;
 
 	/*
 	 * XXX: jumping directly to a commit id via
@@ -523,10 +524,6 @@ got_get_repo_commits(struct request *c, int limit)
 					got_object_commit_close(commit);
 					commit = NULL;
 				}
-				if (t->next_id == NULL) {
-					error = got_error_from_errno("strdup");
-					goto done;
-				}
 				TAILQ_REMOVE(&t->repo_commits, new_repo_commit,
 				    entry);
 				gotweb_free_repo_commit(new_repo_commit);
@@ -576,6 +573,7 @@ got_get_repo_tags(struct request *c, int limit)
 	struct got_tag_object *tag = NULL;
 	struct repo_tag *rt = NULL, *trt = NULL;
 	char *in_repo_path = NULL, *repo_path = NULL, *id_str = NULL;
+	char *tag_commit = NULL, *tag_commit0 = NULL;
 	char *commit_msg = NULL, *commit_msg0 = NULL;
 	int chk_next = 0, chk_multi = 1, commit_found = 0, c_cnt = 0;
 
@@ -585,9 +583,6 @@ got_get_repo_tags(struct request *c, int limit)
 	    repo_dir->name) == -1)
 		return got_error_from_errno("asprintf");
 
-	if (error)
-		return error;
-
 	if (qs->commit == NULL && qs->action == TAGS) {
 		error = got_ref_open(&ref, repo, qs->headref, 0);
 		if (error)
@@ -724,33 +719,36 @@ got_get_repo_tags(struct request *c, int limit)
 				got_object_commit_close(commit);
 				commit = NULL;
 			}
-			if (t->next_id == NULL) {
-				error = got_error_from_errno("strdup");
-				goto err;
-			}
 			TAILQ_REMOVE(&t->repo_tags, new_repo_tag, entry);
 			gotweb_free_repo_tag(new_repo_tag);
 			goto done;
 		}
 
 		if (commit) {
-			error = got_object_commit_get_logmsg(&new_repo_tag->
-			    tag_commit, commit);
+			error = got_object_commit_get_logmsg(&tag_commit0,
+			    commit);
 			if (error)
-				goto done;
+				goto err;
 			got_object_commit_close(commit);
 			commit = NULL;
 		} else {
-			new_repo_tag->tag_commit =
-			    strdup(got_object_tag_get_message(tag));
-			if (new_repo_tag->tag_commit == NULL) {
+			tag_commit0 = strdup(got_object_tag_get_message(tag));
+			if (tag_commit0 == NULL) {
 				error = got_error_from_errno("strdup");
-				goto done;
+				goto err;
 			}
 		}
 
-		while (*new_repo_tag->tag_commit == '\n')
-			new_repo_tag->tag_commit++;
+		tag_commit = tag_commit0;
+		while (*tag_commit == '\n')
+			tag_commit++;
+		new_repo_tag->tag_commit = strdup(tag_commit);
+		if (new_repo_tag->tag_commit == NULL) {
+			error = got_error_from_errno("strdup");
+			free(tag_commit0);
+			goto err;
+		}
+		free(tag_commit0);
 
 		if (qs->action != SUMMARY && qs->action != TAGS) {
 			commit_msg = commit_msg0;
blob - aa8a0d92f2c47069f782a2be91a69ccd329c0c12
file + gotwebd/gotweb.c
--- gotwebd/gotweb.c
+++ gotwebd/gotweb.c
@@ -540,8 +540,10 @@ void
 gotweb_free_repo_tag(struct repo_tag *rt)
 {
 	if (rt != NULL) {
-		free(rt->commit_msg);
 		free(rt->commit_id);
+		free(rt->tag_name);
+		free(rt->tag_commit);
+		free(rt->commit_msg);
 		free(rt->tagger);
 	}
 	free(rt);
@@ -608,10 +610,8 @@ gotweb_free_transport(struct transport *t)
 	}
 	gotweb_free_repo_dir(t->repo_dir);
 	gotweb_free_querystring(t->qs);
-	if (t != NULL) {
-		free(t->next_id);
-		free(t->prev_id);
-	}
+	free(t->next_id);
+	free(t->prev_id);
 	free(t);
 }
 
@@ -2026,9 +2026,8 @@ gotweb_get_repo_description(char **description, struct
 
 	f = fopen(d_file, "r");
 	if (f == NULL) {
-		if (errno == ENOENT || errno == EACCES)
-			return NULL;
-		error = got_error_from_errno2("fopen", d_file);
+		if (errno != ENOENT && errno != EACCES)
+			error = got_error_from_errno2("fopen", d_file);
 		goto done;
 	}
 
@@ -2045,7 +2044,7 @@ gotweb_get_repo_description(char **description, struct
 	if (len == 0) {
 		*description = strdup("");
 		if (*description == NULL)
-			return got_error_from_errno("strdup");
+			error = got_error_from_errno("strdup");
 		goto done;
 	}