From: Stefan Sperling <stsp@stsp.name>
Subject: Re: gotwebd: error path leak & semplification for render_index
To: Omar Polo <op@omarpolo.com>
Cc: gameoftrees@openbsd.org
Date: Tue, 22 Nov 2022 19:27:03 +0100

On Tue, Nov 22, 2022 at 12:04:44PM +0100, Omar Polo wrote:
> On 2022/11/09 18:36:27 +0100, Omar Polo <op@omarpolo.com> wrote:
> > On 2022/11/09 15:48:47 +0100, Stefan Sperling <stsp@stsp.name> wrote:
> > > On Wed, Nov 09, 2022 at 03:27:52PM +0100, Omar Polo wrote:
> > > > -		    strcmp(sd_dent[d_i]->d_name, "..") == 0)
> > > > +		    strcmp(sd_dent[d_i]->d_name, "..") == 0 ||
> > > > +		    sd_dent[d_i]->d_type != DT_DIR) {
> > > 
> > > Unfortunately, we cannot rely on d_type directly. It is unreliable
> > > because it depends on the filesystem implementation.
> > > See got_path_dirent_type() in lib/path.c for a workaround.
> > 
> > Indeed, i forgot about the NFS without readdir plus issue.  Here's an
> > updated diff that uses got_path_dirent_type.
> 
> ping

ok by me.

> -----------------------------------------------
> commit ee2f63eba45d621e2387fe7297d27dec9290a4e8
> from: Omar Polo <op@omarpolo.com>
> date: Tue Nov 22 11:00:49 2022 UTC
>  
>  gotwebd: simplify gotweb_render_index
>  
>   - drops the double loop; paginate in one go
>  
>   - avoid lstat + got_path_dir_is_empty for each entry: use dt_type if
>     provided by the underlying filesystem
>  
>   - fixes a memleak: before `continue' we need to call
>     gotweb_free_repo_dir
>  
> diff 0ceb1b9e49ae865b84f237977d7edbcaa9697161 ee2f63eba45d621e2387fe7297d27dec9290a4e8
> commit - 0ceb1b9e49ae865b84f237977d7edbcaa9697161
> commit + ee2f63eba45d621e2387fe7297d27dec9290a4e8
> blob - fdc41cd4d2cb16ff84e4c42c6a8c826e6f239dc0
> blob + 09d8f166fab40a704104d07aaa4c8e0564295c66
> --- gotwebd/gotweb.c
> +++ gotwebd/gotweb.c
> @@ -993,10 +993,9 @@ gotweb_render_index(struct request *c)
>  	struct repo_dir *repo_dir = NULL;
>  	DIR *d;
>  	struct dirent **sd_dent = NULL;
> -	char *c_path = NULL;
> -	struct stat st;
>  	unsigned int d_cnt, d_i, d_disp = 0;
> -	int r;
> +	unsigned int d_skipped = 0;
> +	int r, type;
>  
>  	d = opendir(srv->repos_path);
>  	if (d == NULL) {
> @@ -1011,25 +1010,6 @@ gotweb_render_index(struct request *c)
>  		goto done;
>  	}
>  
> -	/* get total count of repos */
> -	for (d_i = 0; d_i < d_cnt; d_i++) {
> -		if (strcmp(sd_dent[d_i]->d_name, ".") == 0 ||
> -		    strcmp(sd_dent[d_i]->d_name, "..") == 0)
> -			continue;
> -
> -		if (asprintf(&c_path, "%s/%s", srv->repos_path,
> -		    sd_dent[d_i]->d_name) == -1) {
> -			error = got_error_from_errno("asprintf");
> -			goto done;
> -		}
> -
> -		if (lstat(c_path, &st) == 0 && S_ISDIR(st.st_mode) &&
> -		    !got_path_dir_is_empty(c_path))
> -			t->repos_total++;
> -		free(c_path);
> -		c_path = NULL;
> -	}
> -
>  	r = fcgi_printf(c, "<div id='index_header'>\n"
>  	    "<div id='index_header_project'>Project</div>\n");
>  	if (r == -1)
> @@ -1055,9 +1035,20 @@ gotweb_render_index(struct request *c)
>  			break;
>  
>  		if (strcmp(sd_dent[d_i]->d_name, ".") == 0 ||
> -		    strcmp(sd_dent[d_i]->d_name, "..") == 0)
> +		    strcmp(sd_dent[d_i]->d_name, "..") == 0) {
> +			d_skipped++;
>  			continue;
> +		}
>  
> +		error = got_path_dirent_type(&type, srv->repos_path,
> +		    sd_dent[d_i]);
> +		if (error)
> +			goto done;
> +		if (type != DT_DIR) {
> +			d_skipped++;
> +			continue;
> +		}
> +
>  		if (qs->index_page > 0 && (qs->index_page *
>  		    srv->max_repos_display) > t->prev_disp) {
>  			t->prev_disp++;
> @@ -1071,21 +1062,14 @@ gotweb_render_index(struct request *c)
>  		error = gotweb_load_got_path(c, repo_dir);
>  		if (error && error->code == GOT_ERR_NOT_GIT_REPO) {
>  			error = NULL;
> -			continue;
> -		}
> -		else if (error && error->code != GOT_ERR_LONELY_PACKIDX)
> -			goto done;
> -
> -		if (lstat(repo_dir->path, &st) == 0 &&
> -		    S_ISDIR(st.st_mode) &&
> -		    !got_path_dir_is_empty(repo_dir->path))
> -			goto render;
> -		else {
>  			gotweb_free_repo_dir(repo_dir);
>  			repo_dir = NULL;
> +			d_skipped++;
>  			continue;
>  		}
> -render:
> +		if (error && error->code != GOT_ERR_LONELY_PACKIDX)
> +			goto done;
> +
>  		d_disp++;
>  		t->prev_disp++;
>  
> @@ -1201,6 +1185,8 @@ render:
>  		if (d_disp == srv->max_repos_display)
>  			break;
>  	}
> +	t->repos_total = d_cnt - d_skipped;
> +
>  	if (srv->max_repos_display == 0)
>  		goto done;
>  	if (srv->max_repos > 0 && srv->max_repos < srv->max_repos_display)
> 
>