From: Omar Polo <op@omarpolo.com>
Subject: Re: gotwebd: simplify gotweb_load_got_path
To: Stefan Sperling <stsp@stsp.name>
Cc: gameoftrees@openbsd.org
Date: Thu, 01 Sep 2022 10:40:39 +0200

On 2022/09/01 09:44:14 +0200, Stefan Sperling <stsp@stsp.name> wrote:
> On Wed, Aug 31, 2022 at 06:48:34PM +0200, Omar Polo wrote:
> > landed here why looking for possible leaks.  Still found none, but I
> > thought we can avoid an extra strdup and simplify the logic by
> > dropping `opened'.  We either have opened a dir (and so dt is non
> > NULL) or we haven't.
> > 
> > ok?
> 
> I think the check for GOT_DIR can be dropped. If someone runs gotwebd
> on a work-tree then gotwebd will fail soon enough in some way. There is
> no reason to treat this case with a special check.

agreed, here's a rebased and updated diff that also gc GOTWEB_GOT_DIR
(now unused.)

diff refs/heads/main refs/heads/gwd-load-path
commit - b5c757f5f816a8061f4879da9e68a39141148e40
commit + 7407d6b52b8783876ff8ce04e314aafe1c55736a
blob - 7520e71fd4327bf9eb5df008f47f25845a44564e
blob + 06b0e06e2d7ebf7f9abe95e13bf22731f0389295
--- gotwebd/gotweb.c
+++ gotwebd/gotweb.c
@@ -1979,7 +1979,6 @@ gotweb_load_got_path(struct request *c, struct repo_di
 	struct got_repository *repo = NULL;
 	DIR *dt;
 	char *dir_test;
-	int opened = 0;
 
 	if (asprintf(&dir_test, "%s/%s/%s", srv->repos_path, repo_dir->name,
 	    GOTWEB_GIT_DIR) == -1)
@@ -1989,52 +1988,24 @@ gotweb_load_got_path(struct request *c, struct repo_di
 	if (dt == NULL) {
 		free(dir_test);
 	} else {
-		repo_dir->path = strdup(dir_test);
-		if (repo_dir->path == NULL) {
-			opened = 1;
-			error = got_error_from_errno("strdup");
-			goto err;
-		}
-		opened = 1;
-		goto done;
-	}
-
-	if (asprintf(&dir_test, "%s/%s/%s", srv->repos_path, repo_dir->name,
-	    GOTWEB_GOT_DIR) == -1) {
+		repo_dir->path = dir_test;
 		dir_test = NULL;
-		error = got_error_from_errno("asprintf");
-		goto err;
+		goto done;
 	}
 
-	dt = opendir(dir_test);
-	if (dt == NULL)
-		free(dir_test);
-	else {
-		opened = 1;
-		error = got_error(GOT_ERR_NOT_GIT_REPO);
-		goto err;
-	}
-
 	if (asprintf(&dir_test, "%s/%s", srv->repos_path,
-	    repo_dir->name) == -1) {
-		error = got_error_from_errno("asprintf");
-		dir_test = NULL;
-		goto err;
-	}
+	    repo_dir->name) == -1)
+		return got_error_from_errno("asprintf");
 
-	repo_dir->path = strdup(dir_test);
-	if (repo_dir->path == NULL) {
-		opened = 1;
-		error = got_error_from_errno("strdup");
-		goto err;
-	}
-
 	dt = opendir(dir_test);
 	if (dt == NULL) {
 		error = got_error_path(repo_dir->name, GOT_ERR_NOT_GIT_REPO);
 		goto err;
-	} else
-		opened = 1;
+	} else {
+		repo_dir->path = dir_test;
+		dir_test = NULL;
+	}
+
 done:
 	repo = find_cached_repo(srv, repo_dir->path);
 	if (repo == NULL) {
@@ -2057,9 +2028,8 @@ done:
 	error = gotweb_get_clone_url(&repo_dir->url, srv, repo_dir->path);
 err:
 	free(dir_test);
-	if (opened)
-		if (dt != NULL && closedir(dt) == EOF && error == NULL)
-			error = got_error_from_errno("closedir");
+	if (dt != NULL && closedir(dt) == EOF && error == NULL)
+		error = got_error_from_errno("closedir");
 	return error;
 }
 
blob - 74ee67ae6734f6af0af3ab58f3276ceeccc8863a
blob + bf762e12dd70fa07bb83c88d023e1ebb29d35f27
--- gotwebd/gotwebd.h
+++ gotwebd/gotwebd.h
@@ -53,7 +53,6 @@
 #define MAX_SCRIPT_NAME		 255
 #define MAX_SERVER_NAME		 255
 
-#define GOTWEB_GOT_DIR		 ".got"
 #define GOTWEB_GIT_DIR		 ".git"
 
 #define D_HTTPD_CHROOT		 "/var/www"