From: Omar Polo <op@omarpolo.com>
Subject: Re: gotwebd: fix null-deref and ENOMEM question
To: gameoftrees@openbsd.org
Date: Thu, 01 Sep 2022 23:20:13 +0200

On 2022/09/01 22:24:18 +0200, Omar Polo <op@omarpolo.com> wrote:
> On 2022/09/01 20:35:58 +0200, Omar Polo <op@omarpolo.com> wrote:
> > On 2022/09/01 17:44:17 +0200, Omar Polo <op@omarpolo.com> wrote:
> > > On 2022/08/25 17:41:46 +0200, Omar Polo <op@omarpolo.com> wrote:
> > > > I'm still seeing gotwebd increasing its memory usage over time so I'm
> > > > still searching for leaks.
> > > > 
> > > > [...]
> > > > 
> > > > this still doesn't explain why gotwebd reached 600M of RES in hours,
> > > > so there's probably more.
> > > 
> > > one big leak was in gotweb_render_index where we fail to free the data
> > > allocated by scandir(3)
> > > 
> > > with this is, gotwebd memory consumption didn't raised after a few
> > > thousands requests (I'm running ftp in a loop against it)! \o/
> > 
> > I phrased this poorly; i'm going page by page and the listing page is
> > now memleaks free (well, for the most part at least :D)
> > 
> > diff below tries to fix most of the leaks in the repo summary page.
> > definitely needs more eyeballs because i'm not sure anymore what I'm
> > doing.

this is another set of leaks for the summary page.

along with the previous, it kills 99.9% (TM) of leaks in the summary page.

jokes aside, there are still a few leaks hanging around in here, but
instead of measuring them in tens of megabytes i'm down to a few
hundred kilobytes over 4K requests.

diff /home/op/w/got
commit - 05f412091c5b0750d674d943124116be5c2429ad
path + /home/op/w/got
blob - 43c180c05f3a98f0f8f9ab85355e557ee6cadf99
file + gotwebd/got_operations.c
--- gotwebd/got_operations.c
+++ gotwebd/got_operations.c
@@ -670,15 +670,11 @@ got_get_repo_tags(struct request *c, int limit)
 		error = got_object_open_as_tag(&tag, repo, id);
 		if (error) {
 			if (error->code != GOT_ERR_OBJ_TYPE) {
-				free(id);
-				id = NULL;
 				goto done;
 			}
 			/* "lightweight" tag */
 			error = got_object_open_as_commit(&commit, repo, id);
 			if (error) {
-				free(id);
-				id = NULL;
 				goto done;
 			}
 			new_repo_tag->tagger =
@@ -692,11 +688,7 @@ got_get_repo_tags(struct request *c, int limit)
 			error = got_object_id_str(&id_str, id);
 			if (error)
 				goto err;
-			free(id);
-			id = NULL;
 		} else {
-			free(id);
-			id = NULL;
 			new_repo_tag->tagger =
 			    strdup(got_object_tag_get_tagger(tag));
 			if (new_repo_tag->tagger == NULL) {
@@ -777,10 +769,8 @@ got_get_repo_tags(struct request *c, int limit)
 			new_repo_tag->commit_msg = strdup(commit_msg);
 			if (new_repo_tag->commit_msg == NULL) {
 				error = got_error_from_errno("strdup");
-				free(commit_msg0);
 				goto err;
 			}
-			free(commit_msg0);
 		}
 
 		if (limit && --limit == 0) {
@@ -788,8 +778,17 @@ got_get_repo_tags(struct request *c, int limit)
 				break;
 			chk_next = 1;
 		}
+
 		free(id);
 		id = NULL;
+
+		free(id_str);
+		id_str = NULL;
+
+		if (tag) {
+			got_object_tag_close(tag);
+			tag = NULL;
+		}
 	}
 
 done:
@@ -823,11 +822,17 @@ done:
 		}
 	}
 err:
+	if (tag)
+		got_object_tag_close(tag);
 	if (commit)
 		got_object_commit_close(commit);
+	if (commit_msg0)
+		free(commit_msg0);
 	got_ref_list_free(&refs);
 	free(repo_path);
+	free(in_repo_path);
 	free(id);
+	free(id_str);
 	return error;
 }
 
blob - c25f0953fa4e796d24ebe3806a950049ea1bd120
file + gotwebd/gotweb.c
--- gotwebd/gotweb.c
+++ gotwebd/gotweb.c
@@ -1481,6 +1481,8 @@ gotweb_render_branches(struct request *c)
 	}
 	fcgi_printf(c, "</div>\n"); /* #branches_content */
 done:
+	free(age);
+	got_ref_list_free(&refs);
 	return error;
 }