From: Omar Polo <op@omarpolo.com>
Subject: Re: gotwebd: plug leak regarding repo_commit (plus bonus)
To: Stefan Sperling <stsp@stsp.name>
Cc: gameoftrees@openbsd.org
Date: Thu, 01 Sep 2022 12:56:22 +0200

On 2022/09/01 11:50:54 +0200, Stefan Sperling <stsp@stsp.name> wrote:
> On Wed, Aug 31, 2022 at 11:24:36PM +0200, Omar Polo wrote:
> > 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?
> 
> ok stsp; everything in here looks like an improvement :)

I missed that tag_name is advanced in a few places, so now it can die
in gotweb_free_repo_tag:

gotwebd(30578) in free(): modified chunk-pointer 0xb1f47deaa45

I'll populate my local /var/www/got/public with more interesting
repositories to avoid issues like this in the future, sorry.

is this ok-ish or need to introduce an ad-hoc temp variable?

diff /home/op/w/got
commit - 625e5896fc9ecf87ccfc92ad2a65cd3be58f73c0
path + /home/op/w/got
blob - 0feafb3e945e44bdf1ef9bc88c821630cfd8f703
file + gotwebd/gotweb.c
--- gotwebd/gotweb.c
+++ gotwebd/gotweb.c
@@ -1693,9 +1693,10 @@ gotweb_render_tag(struct request *c)
 	if (error)
 		goto done;
 
-	if (strncmp(rt->tag_name, "refs/", 5) == 0)
-		rt->tag_name += 5;
-	error = gotweb_escape_html(&tagname, rt->tag_name);
+	tagname = rt->tag_name;
+	if (strncmp(tagname, "refs/", 5) == 0)
+		tagname += 5;
+	error = gotweb_escape_html(&tagname, tagname);
 	if (error)
 		goto done;
 
@@ -1780,9 +1781,10 @@ gotweb_render_tags(struct request *c)
 		if (error)
 			goto done;
 
-		if (strncmp(rt->tag_name, "refs/tags/", 10) == 0)
-			rt->tag_name += 10;
-		error = gotweb_escape_html(&tagname, rt->tag_name);
+		tagname = rt->tag_name;
+		if (strncmp(tagname, "refs/tags/", 10) == 0)
+			tagname += 10;
+		error = gotweb_escape_html(&tagname, tagname);
 		if (error)
 			goto done;