From: Stefan Sperling <stsp@stsp.name>
Subject: Re: gotwebd: fix null-deref and ENOMEM question
To: Omar Polo <op@omarpolo.com>
Cc: Tracey Emery <tracey@traceyemery.net>, gameoftrees@openbsd.org
Date: Thu, 1 Sep 2022 19:32:50 +0200

On Thu, Sep 01, 2022 at 05:44:17PM +0200, Omar Polo wrote:
> On 2022/08/25 17:41:46 +0200, Omar Polo <op@omarpolo.com> wrote:
> > I'm still seeing gotwebd increasing its memory usage over time so I'm
> > still searching for leaks.
> > 
> > [...]
> > 
> > this still doesn't explain why gotwebd reached 600M of RES in hours,
> > so there's probably more.
> 
> one big leak was in gotweb_render_index where we fail to free the data
> allocated by scandir(3)
> 
> with this is, gotwebd memory consumption didn't raised after a few
> thousands requests (I'm running ftp in a loop against it)! \o/
> 
> (I suspect there are still some leaks in the error paths inside the
> main loop -- the one with the `render' label in it -- will come back
> to this later if i don't forget and nobody beats me to it.)

ok stsp

> diff /home/op/w/got
> commit - f8faf9f103c9a4869c82a3fe55658f0a065fb1c0
> path + /home/op/w/got
> blob - d7b904108bc299cb7ad3753fbb33a3660024d371
> file + gotwebd/gotweb.c
> --- gotwebd/gotweb.c
> +++ gotwebd/gotweb.c
> @@ -940,7 +940,7 @@ gotweb_render_index(struct request *c)
>  	struct querystring *qs = t->qs;
>  	struct repo_dir *repo_dir = NULL;
>  	DIR *d;
> -	struct dirent **sd_dent;
> +	struct dirent **sd_dent = NULL;
>  	const char *index_page_str;
>  	char *c_path = NULL;
>  	struct stat st;
> @@ -957,6 +957,7 @@ gotweb_render_index(struct request *c)
>  
>  	d_cnt = scandir(srv->repos_path, &sd_dent, NULL, alphasort);
>  	if (d_cnt == -1) {
> +		sd_dent = NULL;
>  		error = got_error_from_errno2("scandir", srv->repos_path);
>  		goto done;
>  	}
> @@ -1119,6 +1120,11 @@ render:
>  	if (error)
>  		goto done;
>  done:
> +	if (sd_dent) {
> +		for (d_i = 0; d_i < d_cnt; d_i++)
> +			free(sd_dent[d_i]);
> +		free(sd_dent);
> +	}
>  	if (d != NULL && closedir(d) == EOF && error == NULL)
>  		error = got_error_from_errno("closedir");
>  	return error;
> 
>