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: Sat, 05 Nov 2022 15:27:46 +0100

On 2022/11/05 14:30:50 +0100, Stefan Sperling <stsp@stsp.name> wrote:
> On Sat, Nov 05, 2022 at 12:42:27PM +0100, Omar Polo wrote:
> > I'm still trying to migrate gotwebd to that templating system I posted
> > some time ago and so I'm re-reading all the code that produces the
> > html...
> > 
> > There's a memory leak in the error path if asprintf fails, and while
> > here I'd also like to simplify the check and remove the `goto render'.
> 
> Looks good to me, just one hint below:
> 
> > -		if (lstat(repo_dir->path, &st) == 0 &&
> > -		    S_ISDIR(st.st_mode) &&
> > -		    !got_path_dir_is_empty(repo_dir->path))
> > -			goto render;
> > -		else {
> > +		if (lstat(repo_dir->path, &st) == -1 ||
> > +		    !S_ISDIR(st.st_mode) ||
> > +		    got_path_dir_is_empty(repo_dir->path)) {
> 
> It might be a good idea to set err = got_error_from_errno() if lstat() fails
> for some reason other than ENOENT, and do the ISDIR + empty checks separately.

Honestly, I don't really like the idea of got_path_dir_is_empty.  What
about moving that check in gotweb_load_got_path after we've opened the
directory?  Note that this already happens *before* this hunk, so
we're re-opening a directory needlessly.

The only difference is that before it didn't follow symlinks (due to
lseek) while now do since we're just using opendir.  I can change the
opendir in gotweb_load_got_path to open(O_NOFOLLOW) + fdopendir in a
follow-up commit should it be a concern.

There's still one call to got_path_dir_is_empty in a previous loop in
the same function, and that's the only call in gotwebd.  I'd like to
remove that too (and the whole loop actually; there's no reason we
can't paginate in a single loop.)

While here i'm also fixing a memory leak: before `continue' we need to
call gotweb_free_repo_dir.

diff /home/op/w/gotd
commit - db2d7e9ec84a0ca1f09571551401eea90df80d2a
path + /home/op/w/gotd
blob - 9032f8f27d7be1c8cf05dfbf1d3c6909cf32821f
file + gotwebd/gotweb.c
--- gotwebd/gotweb.c
+++ gotwebd/gotweb.c
@@ -1071,18 +1071,12 @@ 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;
-		}
-		if (error && error->code != GOT_ERR_LONELY_PACKIDX)
-			goto done;
-
-		if (lstat(repo_dir->path, &st) == -1 ||
-		    !S_ISDIR(st.st_mode) ||
-		    got_path_dir_is_empty(repo_dir->path)) {
 			gotweb_free_repo_dir(repo_dir);
 			repo_dir = NULL;
 			continue;
 		}
+		if (error && error->code != GOT_ERR_LONELY_PACKIDX)
+			goto done;
 
 		d_disp++;
 		t->prev_disp++;
@@ -2408,6 +2402,8 @@ gotweb_load_got_path(struct request *c, struct repo_di
 	struct transport *t = c->t;
 	struct got_repository *repo = NULL;
 	DIR *dt;
+	struct dirent *dent;
+	int empty = 1;
 	char *dir_test;
 
 	if (asprintf(&dir_test, "%s/%s/%s", srv->repos_path, repo_dir->name,
@@ -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;
+	}
+	if (empty) {
+		error = got_error_path(repo_dir->name, GOT_ERR_NOT_GIT_REPO);
+		goto err;
+	}
 
 	repo = find_cached_repo(srv, repo_dir->path);
 	if (repo == NULL) {