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

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

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