On Fri, Nov 17, 2023 at 09:09:48AM +0100, Stefan Sperling wrote:
> On Fri, Nov 17, 2023 at 12:46:20AM +0100, Stefan Sperling wrote:
> > I suspect the sharing of sock->pack_fds across cached open repositories
> > during requests is the source of the "unexpecte EOF" errors that
> > sometimes occur on my gotwebd instance.
> > 
> > This attempts to remove the cache again. The hard part is ensuring that
> > the repository is closed once the request is done. Did I get this right?
> 
> There previous diff still file descriptor leaks, causing fd exhaustion
> resulting in errors like "bad exit code from unprivileged process".
> Presumbaly because a forked child ran out of available fd slots early on.
> One sockets worker had 1024 open files, which is the hard limit.
> 
> New diff, with all the early returns in process_request changed to
> close_repo gotos. Just in case I am also letting gotweb_load_got_path()
> close open repositories on error.

On IRC, Omar pointed out a better place to close the per-request repository.

-----------------------------------------------
 remove the gotwebd repository cache
 
 It only had 4 slots so was never quite useful, and sharing of sock->pack_fds
 across cached repositories seems problematic.
 
diff 7607b8e0588a18b371f96092b43969a53ac94b09 591ef18b4a9375dfbdbbae762f54381aca152c7e
commit - 7607b8e0588a18b371f96092b43969a53ac94b09
commit + 591ef18b4a9375dfbdbbae762f54381aca152c7e
blob - b35fa0bcfb79ea056b7d4831d3597333b7341e2a
blob + b1108c45d826da11c2d77f4d5c684f9c629ea7c8
--- gotwebd/config.c
+++ gotwebd/config.c
@@ -85,11 +85,6 @@ config_getserver(struct gotwebd *env, struct imsg *ims
 	IMSG_SIZE_CHECK(imsg, srv);
 
 	memcpy(srv, p, sizeof(*srv));
-	srv->cached_repos = calloc(GOTWEBD_REPO_CACHESIZE,
-	    sizeof(*srv->cached_repos));
-	if (srv->cached_repos == NULL)
-		fatal("%s: calloc", __func__);
-	srv->ncached_repos = 0;
 
 	/* log server info */
 	log_debug("%s: server=%s fcgi_socket=%s unix_socket=%s", __func__,
blob - 47f0357248e0fce31de5f002bfe19d13da3d5137
blob + 4a28201f51028f53e89c9ab96cd01da12d0c1927
--- gotwebd/gotweb.c
+++ gotwebd/gotweb.c
@@ -745,6 +745,8 @@ gotweb_free_transport(struct transport *t)
 			free(t->repos[i]);
 		free(t->repos);
 	}
+	if (t->repo)
+		got_repo_close(t->repo);
 	free(t);
 }
 
@@ -860,6 +862,10 @@ gotweb_render_index(struct template *tp)
 				    sd_dent[d_i]->d_name, error->msg);
 			gotweb_free_repo_dir(repo_dir);
 			repo_dir = NULL;
+			if (t->repo) {
+				got_repo_close(t->repo);
+				t->repo = NULL;
+			}
 			d_skipped++;
 			continue;
 		}
@@ -869,6 +875,11 @@ gotweb_render_index(struct template *tp)
 
 		r = gotweb_render_repo_fragment(c->tp, repo_dir);
 		gotweb_free_repo_dir(repo_dir);
+		repo_dir = NULL;
+		if (t->repo) {
+			got_repo_close(t->repo);
+			t->repo = NULL;
+		}
 		if (r == -1)
 			return -1;
 
@@ -1104,75 +1115,13 @@ gotweb_render_absolute_url(struct request *c, struct g
 	return gotweb_render_url(c, url);
 }
 
-static struct got_repository *
-find_cached_repo(struct server *srv, const char *path)
-{
-	int i;
-
-	for (i = 0; i < srv->ncached_repos; i++) {
-		if (strcmp(srv->cached_repos[i].path, path) == 0)
-			return srv->cached_repos[i].repo;
-	}
-
-	return NULL;
-}
-
 static const struct got_error *
-cache_repo(struct got_repository **new, struct server *srv,
-    struct repo_dir *repo_dir, struct socket *sock)
-{
-	const struct got_error *error = NULL;
-	struct got_repository *repo;
-	struct cached_repo *cr;
-	int evicted = 0;
-
-	if (srv->ncached_repos >= GOTWEBD_REPO_CACHESIZE) {
-		cr = &srv->cached_repos[srv->ncached_repos - 1];
-		error = got_repo_close(cr->repo);
-		memset(cr, 0, sizeof(*cr));
-		srv->ncached_repos--;
-		if (error)
-			return error;
-		memmove(&srv->cached_repos[1], &srv->cached_repos[0],
-		    srv->ncached_repos * sizeof(srv->cached_repos[0]));
-		cr = &srv->cached_repos[0];
-		evicted = 1;
-	} else {
-		cr = &srv->cached_repos[srv->ncached_repos];
-	}
-
-	error = got_repo_open(&repo, repo_dir->path, NULL, sock->pack_fds);
-	if (error) {
-		if (evicted) {
-			memmove(&srv->cached_repos[0], &srv->cached_repos[1],
-			    srv->ncached_repos * sizeof(srv->cached_repos[0]));
-		}
-		return error;
-	}
-
-	if (strlcpy(cr->path, repo_dir->path, sizeof(cr->path))
-	    >= sizeof(cr->path)) {
-		if (evicted) {
-			memmove(&srv->cached_repos[0], &srv->cached_repos[1],
-			    srv->ncached_repos * sizeof(srv->cached_repos[0]));
-		}
-		return got_error(GOT_ERR_NO_SPACE);
-	}
-
-	cr->repo = repo;
-	srv->ncached_repos++;
-	*new = repo;
-	return NULL;
-}
-
-static const struct got_error *
 gotweb_load_got_path(struct request *c, struct repo_dir *repo_dir)
 {
 	const struct got_error *error = NULL;
 	struct socket *sock = c->sock;
 	struct server *srv = c->srv;
 	struct transport *t = c->t;
-	struct got_repository *repo = NULL;
 	DIR *dt;
 	char *dir_test;
 
@@ -1186,12 +1135,14 @@ gotweb_load_got_path(struct request *c, struct repo_di
 	} else {
 		repo_dir->path = dir_test;
 		dir_test = NULL;
-		goto done;
+		goto open_repo;
 	}
 
 	if (asprintf(&dir_test, "%s/%s", srv->repos_path,
-	    repo_dir->name) == -1)
-		return got_error_from_errno("asprintf");
+	    repo_dir->name) == -1) {
+		error = got_error_from_errno("asprintf");
+		goto err;
+	}
 
 	dt = opendir(dir_test);
 	if (dt == NULL) {
@@ -1202,20 +1153,16 @@ gotweb_load_got_path(struct request *c, struct repo_di
 		dir_test = NULL;
 	}
 
-done:
+open_repo:
 	if (srv->respect_exportok &&
 	    faccessat(dirfd(dt), "git-daemon-export-ok", F_OK, 0) == -1) {
 		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) {
-		error = cache_repo(&repo, srv, repo_dir, sock);
-		if (error)
-			goto err;
-	}
-	t->repo = repo;
+	error = got_repo_open(&t->repo, repo_dir->path, NULL, sock->pack_fds);
+	if (error)
+		goto err;
 	error = gotweb_get_repo_description(&repo_dir->description, srv,
 	    repo_dir->path, dirfd(dt));
 	if (error)
@@ -1234,6 +1181,10 @@ err:
 	free(dir_test);
 	if (dt != NULL && closedir(dt) == EOF && error == NULL)
 		error = got_error_from_errno("closedir");
+	if (error && t->repo) {
+		got_repo_close(t->repo);
+		t->repo = NULL;
+	}
 	return error;
 }
 
blob - a76ef44049655b82575dc5a398b4739eeb44efbc
blob + 71b34a0258f2a201a28b0745032caf69c7fcd5e6
--- gotwebd/gotwebd.h
+++ gotwebd/gotwebd.h
@@ -47,7 +47,6 @@
 #define GOTWEBD_MAXNAME		 64
 #define GOTWEBD_MAXPORT		 6
 #define GOTWEBD_NUMPROC		 3
-#define GOTWEBD_REPO_CACHESIZE	 4
 #define GOTWEBD_SOCK_FILENO	 3
 
 #define PROC_MAX_INSTANCES	 32
@@ -290,18 +289,10 @@ struct address {
 };
 TAILQ_HEAD(addresslist, address);
 
-struct cached_repo {
-	char path[PATH_MAX];
-	struct got_repository *repo;
-};
-
 struct server {
 	TAILQ_ENTRY(server)	 entry;
 	struct addresslist	al;
 
-	struct cached_repo	*cached_repos;
-	int		 ncached_repos;
-
 	char		 name[GOTWEBD_MAXTEXT];
 
 	char		 repos_path[PATH_MAX];
blob - a16821af966e854627051ef7d1ddb100a024c3d1
blob + 4ac43e3ddee0f601ea33b3a80f934242683cdc4c
--- gotwebd/sockets.c
+++ gotwebd/sockets.c
@@ -399,7 +399,6 @@ sockets_shutdown(void)
 {
 	struct server *srv, *tsrv;
 	struct socket *sock, *tsock;
-	int i;
 
 	sockets_purge(gotwebd_env);
 
@@ -411,11 +410,8 @@ sockets_shutdown(void)
 	}
 
 	/* clean servers */
-	TAILQ_FOREACH_SAFE(srv, &gotwebd_env->servers, entry, tsrv) {
-		for (i = 0; i < srv->ncached_repos; i++)
-			got_repo_close(srv->cached_repos[i].repo);
+	TAILQ_FOREACH_SAFE(srv, &gotwebd_env->servers, entry, tsrv)
 		free(srv);
-	}
 
 	free(gotwebd_env);
 }