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 18:36:27 +0100

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.

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,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)
@@ -1051,13 +1031,24 @@ 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) {
+			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)