From: Omar Polo <op@omarpolo.com>
Subject: Re: gotwebd: fix null-deref and ENOMEM question
To: Omar Polo <op@omarpolo.com>
Cc: gameoftrees@openbsd.org
Date: Wed, 24 Aug 2022 18:33:42 +0200

On 2022/08/23 10:39:26 +0200, Omar Polo <op@omarpolo.com> wrote:
> i had another gotwebd crash.  This one is pretty simple to fix, in
> gotweb_process_request if gotweb_init_querystring fails c->t-qs is
> NULL and at the end of the function we might crash.
> 
> diff /home/op/w/got
> commit - 4d648b92ac1ac5f952a42f29052c56e8d32547a3
> path + /home/op/w/got
> blob - 072126ddb17614b1c833d48856c433c128ab891c
> file + gotwebd/gotweb.c
> --- gotwebd/gotweb.c
> +++ gotwebd/gotweb.c
> @@ -292,7 +292,7 @@ err:
>  	if (html && fcgi_printf(c, "</div>\n") == -1)
>  		return;
>  done:
> -	if (c->t->repo != NULL && qs->action != INDEX)
> +	if (c->t->repo != NULL && qs && qs->action != INDEX)
>  		got_repo_close(c->t->repo);
>  	if (html && srv != NULL)
>  		gotweb_render_footer(c);
> 
> easy to fix.

this wasn't enough.  if, like i discovered today,
gotweb_init_transport fails it still segfaults.

> but now the question, i had these logs:
> 
> ------8<--------
> gotweb_process_request: malloc: Cannot allocate memory
> [...]
> ------>8--------
> 
> that's a lot of ENOMEM!

I've also spotted the memory leak.  When adding the escaping of
various strings i forgot to free one.

updated and combined diff, ok?

diff /home/op/w/got
commit - 4d648b92ac1ac5f952a42f29052c56e8d32547a3
path + /home/op/w/got
blob - 18dca123b78589b4efb8482bc4caa54e1aae7176
file + gotwebd/got_operations.c
--- gotwebd/got_operations.c
+++ gotwebd/got_operations.c
@@ -830,7 +830,7 @@ got_output_repo_tree(struct request *c)
 	struct got_tree_object *tree = NULL;
 	struct repo_dir *repo_dir = t->repo_dir;
 	const char *name, *index_page_str, *folder;
-	char *id_str = NULL, *escaped_name;
+	char *id_str = NULL, *escaped_name = NULL;
 	char *path = NULL, *in_repo_path = NULL, *modestr = NULL;
 	int nentries, i, r;
 
@@ -967,8 +967,11 @@ got_output_repo_tree(struct request *c)
 		id_str = NULL;
 		free(modestr);
 		modestr = NULL;
+		free(escaped_name);
+		escaped_name = NULL;
 	}
 done:
+	free(escaped_name);
 	free(id_str);
 	free(modestr);
 	free(path);
blob - 072126ddb17614b1c833d48856c433c128ab891c
file + gotwebd/gotweb.c
--- gotwebd/gotweb.c
+++ gotwebd/gotweb.c
@@ -124,7 +124,7 @@ gotweb_process_request(struct request *c)
 	error = gotweb_init_transport(&c->t);
 	if (error) {
 		log_warnx("%s: %s", __func__, error->msg);
-		goto err;
+		return;
 	}
 	/* don't process any further if client disconnected */
 	if (c->sock->client_status == CLIENT_DISCONNECT)
@@ -292,7 +292,7 @@ err:
 	if (html && fcgi_printf(c, "</div>\n") == -1)
 		return;
 done:
-	if (c->t->repo != NULL && qs->action != INDEX)
+	if (c->t->repo != NULL && qs && qs->action != INDEX)
 		got_repo_close(c->t->repo);
 	if (html && srv != NULL)
 		gotweb_render_footer(c);