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

On 2022/11/05 17:04:25 +0100, Stefan Sperling <stsp@stsp.name> wrote:
> On Sat, Nov 05, 2022 at 03:27:46PM +0100, Omar Polo wrote:
> > Honestly, I don't really like the idea of got_path_dir_is_empty.  What
> 
> Why? Because it opens and closes the directory again even
> though gotwebd already has an open handle to the directory?
> Or is there another reason?

in part that, and in part because i don't like the idea of opening a
directory to tell if it's empty, to then re-open it again if it's not.

haven't checked how got_path_dir_is_empty is called usually, but these
two call were doing that.

> > @@ -2443,6 +2439,17 @@ done:
> >  		goto err;
> >  	}
> >  
> > +	while ((dent = readdir(dt)) != NULL) {
> > +		if (strcmp(dent->d_name, ".") == 0 ||
> > +		    strcmp(dent->d_name, "..") == 0)
> > +			continue;
> > +		empty = 0;
> > +		break;
> > +	}
> 
> Instead of hand-rolling this loop, we could have a function which implements
> this loop in path.c, and call it from here and from got_path_dir_is_empty().

ah yes, but turns out it's not needed :)

here's a slightly more ambitious diff that:

 - drops the double loop; paginate in one go
 - improves the check for max_repos: look at the number of item
   processed; don't assume we'll skip only '.' and '..'
 - avoids lstat by looking at dt_type
 - drop got_path_dir_is_empty; gotweb_load_got_path already returns
   GOT_ERR_NOT_GIT_REPO in that case
 - fixes another memleak somehow: before `continue' we need to call
   gotweb_free_repo_dir

repos_total now it's an estimate but since it's used only to know if
to render the "next" button I don't think it's an issue.

diff /home/op/w/got
commit - 86979eca9f6d38458e13c64618b71e8667ce5b94
path + /home/op/w/got
blob - 7cb4b4aa6c9a239aeb25328ef01b302f2131753a
file + gotwebd/gotweb.c
--- gotwebd/gotweb.c
+++ gotwebd/gotweb.c
@@ -993,9 +993,8 @@ 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;
+	unsigned int d_skipped = 0;
 	int r;
 
 	d = opendir(srv->repos_path);
@@ -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)
@@ -1051,12 +1031,15 @@ gotweb_render_index(struct request *c)
 		goto done;
 
 	for (d_i = 0; d_i < d_cnt; d_i++) {
-		if (srv->max_repos > 0 && (d_i - 2) == srv->max_repos)
-			break; /* account for parent and self */
+		if (srv->max_repos > 0 && t->prev_disp == srv->max_repos)
+			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 ||
+		    sd_dent[d_i]->d_type != DT_DIR) {
+			d_skipped++;
 			continue;
+		}
 
 		if (qs->index_page > 0 && (qs->index_page *
 		    srv->max_repos_display) > t->prev_disp) {
@@ -1071,21 +1054,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 +1177,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)