From: Tracey Emery <tracey@traceyemery.net>
Subject: Re: gotwebd: fix null-deref and ENOMEM question
To: gameoftrees@openbsd.org, Omar Polo <op@omarpolo.com>
Date: Wed, 24 Aug 2022 22:08:44 -0600

On August 24, 2022 10:33:42 AM MDT, Omar Polo <op@omarpolo.com> wrote:
>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?

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);
>
>


-- 
Tracey Emery
Sent from my phone.