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>, gameoftrees@openbsd.org
Date: Thu, 7 Oct 2021 16:18:55 +0200

On Thu, Oct 07, 2021 at 04:06:01PM +0200, Stefan Sperling wrote:
> 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.

And one more time with proper error checking in got_gitproto_parse_refline().

diff 4afb33a50adb03566efa20a315bf6fb7ad0b74df fdbd238288c139ce7db8b54e93ff0c7afc1aab20
blob - e84621dc25c55193b9782419f2eae52217f447c0
blob + 65b72c94cb38ba341e45ff3da37382ec86713f5e
--- lib/gitproto.c
+++ lib/gitproto.c
@@ -82,25 +82,47 @@ got_gitproto_parse_refline(char **id_str, char **refna
     char **server_capabilities, char *line, int len)
 {
 	const struct got_error *err = NULL;
-	char *tokens[3];
+	char *tokens[3] = { NULL, NULL, NULL };
 
+	*id_str = NULL;
+	*refname = NULL;
+	/* don't reset *server_capabilities */
+
 	err = tokenize_refline(tokens, line, len, nitems(tokens));
 	if (err)
 		return err;
 
 	if (tokens[0])
 		*id_str = tokens[0];
+	else {
+		*id_str = NULL;
+		err = got_error(GOT_ERR_BAD_PACKET);
+		goto done;
+	}
 	if (tokens[1])
 		*refname = tokens[1];
+	else {
+		*id_str = NULL;
+		err = got_error(GOT_ERR_BAD_PACKET);
+		goto done;
+	}
 	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;
+done:
+	if (err) {
+		int i;
+		for (i = 0; i < nitems(tokens); i++)
+			free(tokens[i]);
+	}
+	return err;
 }
 
 static const struct got_error *
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)