From: Stefan Sperling <stsp@stsp.name>
Subject: Re: plug memleaks in got-fetch-pack and got-send-pack
To: Christian Weisgerber <naddy@mips.inka.de>
Cc: gameoftrees@openbsd.org
Date: Thu, 7 Oct 2021 16:06:01 +0200

On Thu, Oct 07, 2021 at 03:49:26PM +0200, Christian Weisgerber wrote:
> Stefan Sperling:
> 
> > The id_str and refname results of got_gitproto_parse_refline()
> > were allocated with strdup(3) and must be freed during every
> > iteration of the loop which parses reflines, not just when the
> > current function exits.
> -snip-
> 
> That looks correct, but a bit cumbersome.
> How about free()ing them at the head of the loop, maybe right before
> the got_gitproto_parse_refline() call?

Good point. Looking at this I realized that got_gitproto_parse_refline()
should be initializing its output arguments to make things easier for
callers. Except the capabilities string, which is only parsed once,
and which should be freed as well if it occurs more than once.

diff 4afb33a50adb03566efa20a315bf6fb7ad0b74df fa204bc395ee4d4d6c14b5565906f20f323e58ae
blob - e84621dc25c55193b9782419f2eae52217f447c0
blob + dec248671df1f10f622bd433e33a46fc2d10f627
--- lib/gitproto.c
+++ lib/gitproto.c
@@ -84,6 +84,10 @@ got_gitproto_parse_refline(char **id_str, char **refna
 	const struct got_error *err = NULL;
 	char *tokens[3];
 
+	*id_str = NULL;
+	*refname = NULL;
+	/* don't reset *server_capabilities */
+
 	err = tokenize_refline(tokens, line, len, nitems(tokens));
 	if (err)
 		return err;
@@ -93,11 +97,14 @@ got_gitproto_parse_refline(char **id_str, char **refna
 	if (tokens[1])
 		*refname = tokens[1];
 	if (tokens[2]) {
-		char *p;
-		*server_capabilities = tokens[2];
-		p = strrchr(*server_capabilities, '\n');
-		if (p)
-			*p = '\0';
+		if (*server_capabilities == NULL) {
+			char *p;
+			*server_capabilities = tokens[2];
+			p = strrchr(*server_capabilities, '\n');
+			if (p)
+				*p = '\0';
+		} else
+			free(tokens[2]);
 	}
 
 	return NULL;
blob - 171700f1fa26666e69adceb379ea1e594e5513d7
blob + b85f024e4b7e5925391dbd4ff5c8959ddb3234bc
--- libexec/got-fetch-pack/got-fetch-pack.c
+++ libexec/got-fetch-pack/got-fetch-pack.c
@@ -346,6 +346,8 @@ fetch_pack(int fd, int packfd, uint8_t *pack_sha1,
 			err = fetch_error(&buf[4], n - 4);
 			goto done;
 		}
+		free(id_str);
+		free(refname);
 		err = got_gitproto_parse_refline(&id_str, &refname,
 		    &server_capabilities, buf, n);
 		if (err)
blob - eb8f383cfa96ab51e90ae06602d44b10e2336430
blob + bec42fbe983ba0d87334fee8b88459e849fbc337
--- libexec/got-send-pack/got-send-pack.c
+++ libexec/got-send-pack/got-send-pack.c
@@ -353,6 +353,8 @@ send_pack(int fd, struct got_pathlist_head *refs,
 			err = send_error(&buf[4], n - 4);
 			goto done;
 		}
+		free(id_str);
+		free(refname);
 		err = got_gitproto_parse_refline(&id_str, &refname,
 		    &server_capabilities, buf, n);
 		if (err)