From: Omar Polo <op@omarpolo.com>
Subject: Re: gotwebd: leaks in blame page
To: Omar Polo <op@omarpolo.com>
Cc: Stefan Sperling <stsp@stsp.name>, gameoftrees@openbsd.org
Date: Sat, 03 Sep 2022 13:21:30 +0200

On 2022/09/03 12:51:12 +0200, Omar Polo <op@omarpolo.com> wrote:
> On 2022/09/03 09:31:45 +0200, Stefan Sperling <stsp@stsp.name> wrote:
> > On Fri, Sep 02, 2022 at 05:20:09PM +0200, Omar Polo wrote:
> > > however, the blame page still leaks like crazy.  There's some serious
> > > leak between got_gotweb_blame_cb and got_output_file_blame that I'm
> > > just not seeing.
> > > 
> > > In a controlled environment (localhost) after 1K requests to a blame
> > > page (always the same to rule out the cache) gotwebd' RES is 24M.  If
> > > I comment the call to got_blame then it's "just" ~5M.  So, it leaks
> > > both outside got_output_file_blame but the more serious leak is
> > > inside.
> > 
> > Is this easy to trigger via a reproducer hacked into got/got.c?
> 
> it seems so!
> 
> it's an hack of a diff but then `got blame Makefile' starts leaking
> memory quite fast.

the big leak is inside got_blame, but i managed to find something
outside too.

diff below fixes a leak in got_repo_pack_fds_open where we forget to
release the memory for the temp array.

I see two ways to fix it.  The first one is the smallest one and less
invasive:

diff /home/op/w/got
commit - 06edcc519b27fc4831d2583a8dd10c7d2ab61e10
path + /home/op/w/got
blob - 9d9dfd0586d7bfc07846c61a82e2f1778968d95c
file + lib/repository.c
--- lib/repository.c
+++ lib/repository.c
@@ -277,10 +277,12 @@ got_repo_pack_fds_open(int **pack_fds)
 		if (pack_fds_tmp[i] == -1) {
 			err = got_error_from_errno("got_opentempfd");
 			got_repo_pack_fds_close(pack_fds_tmp);
+			free(pack_fds);
 			return err;
 		}
 	}
 	memcpy(*pack_fds, pack_fds_tmp, GOT_PACK_NUM_TEMPFILES * sizeof(int));
+	free(pack_fds_tmp);
 	return err;
 }
 

The second one is to just get rid of the temp array altogether.  a bit
more invasive but i like it a bit more:

diff /home/op/w/got
commit - 06edcc519b27fc4831d2583a8dd10c7d2ab61e10
path + /home/op/w/got
blob - 9d9dfd0586d7bfc07846c61a82e2f1778968d95c
file + lib/repository.c
--- lib/repository.c
+++ lib/repository.c
@@ -252,16 +252,11 @@ const struct got_error *
 got_repo_pack_fds_open(int **pack_fds)
 {
 	const struct got_error *err = NULL;
-	int i, *pack_fds_tmp;
+	int i;
 
-	pack_fds_tmp = calloc(GOT_PACK_NUM_TEMPFILES, sizeof(int));
-	if (pack_fds_tmp == NULL)
-		return got_error_from_errno("calloc");
 	*pack_fds = calloc(GOT_PACK_NUM_TEMPFILES, sizeof(**pack_fds));
-	if (*pack_fds == NULL) {
-		free(pack_fds_tmp);
+	if (*pack_fds == NULL)
 		return got_error_from_errno("calloc");
-	}
 
 	/*
 	 * got_repo_pack_fds_close will try to close all of the
@@ -270,18 +265,19 @@ got_repo_pack_fds_open(int **pack_fds)
 	 * we do not initialize to -1 here.
 	 */
 	for (i = 0; i < GOT_PACK_NUM_TEMPFILES; i++)
-		pack_fds_tmp[i] = -1;
+		(*pack_fds)[i] = -1;
 
 	for (i = 0; i < GOT_PACK_NUM_TEMPFILES; i++) {
-		pack_fds_tmp[i] = got_opentempfd();
-		if (pack_fds_tmp[i] == -1) {
+		(*pack_fds)[i] = got_opentempfd();
+		if ((*pack_fds)[i] == -1) {
 			err = got_error_from_errno("got_opentempfd");
-			got_repo_pack_fds_close(pack_fds_tmp);
+			got_repo_pack_fds_close(*pack_fds);
+			*pack_fds = NULL;
 			return err;
 		}
 	}
-	memcpy(*pack_fds, pack_fds_tmp, GOT_PACK_NUM_TEMPFILES * sizeof(int));
-	return err;
+
+	return NULL;
 }
 
 const struct got_error *