On Tue, Jun 15, 2021 at 03:05:55PM -0600, Tracey Emery wrote:
> Hello,
> 
> While working on a section of gotwebd, I was testing and found that the
> process's fd table kept growing and growing, also verified by the
> quantity of opened fds in fstat.
> 
> I tracked this down to repo->gitdir_fd being opened in got_repo_open,
> but never being closed in got_repo_close. The following is a small diff
> that fixes the problem.
> 
> ok?
> 
> -- 
> 
> Tracey Emery
> 
> diff 282f42e5d1095015379b49280429e558b3bbc4fe /home/tracey/src/got
> blob - a97375aa5712bae416359100f0059fa17f6cfc46
> file + lib/repository.c
> --- lib/repository.c
> +++ lib/repository.c
> @@ -752,6 +752,9 @@ got_repo_close(struct got_repository *repo)
>  			err = got_error_from_errno("close");
>  	}
>  
> +	if (repo->gitdir_fd != -1)
> +		close(repo->gitdir_fd);
> +

This does not check for errors from close(2). Of course such errors are
unlikely in this particular case but could happen due to bugs (e.g. if
the repository fd was closed twice somehow).

In any case, ignoring errors from close(2) or fclose(3) has hidden bugs
in the past. I'd prefer us being consistent about error checking.

The patch below adds the missing error check as follows:

  -	if (repo->gitdir_fd != -1)
  -		close(repo->gitdir_fd);
  +	if (repo->gitdir_fd != -1 && close(repo->gitdir_fd) == -1 &&
  +	    err == NULL)
  +		err = got_error_from_errno("close");

and also propagates errors from got_repo_close() up the call stack
where possible. Some code paths already did this, but most did not.

We should probably do the same for got_worktree_close(), too.
But that's best left for another patch.

diff 991ff1aa4f423a1faea1bae0e85a913a88038309 /home/stsp/src/got
blob - 192b847bcb16c4c5fe215175d5a8109d0712f76b
file + got/got.c
--- got/got.c
+++ got/got.c
@@ -1800,8 +1800,11 @@ done:
 	}
 	if (fetchfd != -1 && close(fetchfd) == -1 && error == NULL)
 		error = got_error_from_errno("close");
-	if (repo)
-		got_repo_close(repo);
+	if (repo) {
+		const struct got_error *close_err = got_repo_close(repo);
+		if (error == NULL)
+			error = close_err;
+	}
 	TAILQ_FOREACH(pe, &refs, entry) {
 		free((void *)pe->path);
 		free(pe->data);
@@ -2515,8 +2518,11 @@ done:
 	}
 	if (fetchfd != -1 && close(fetchfd) == -1 && error == NULL)
 		error = got_error_from_errno("close");
-	if (repo)
-		got_repo_close(repo);
+	if (repo) {
+		const struct got_error *close_err = got_repo_close(repo);
+		if (error == NULL)
+			error = close_err;
+	}
 	if (worktree)
 		got_worktree_close(worktree);
 	TAILQ_FOREACH(pe, &refs, entry) {
@@ -4063,10 +4069,9 @@ done:
 	if (worktree)
 		got_worktree_close(worktree);
 	if (repo) {
-		const struct got_error *repo_error;
-		repo_error = got_repo_close(repo);
+		const struct got_error *close_err = got_repo_close(repo);
 		if (error == NULL)
-			error = repo_error;
+			error = close_err;
 	}
 	if (refs_idmap)
 		got_reflist_object_id_map_free(refs_idmap);
@@ -4488,10 +4493,9 @@ done:
 	if (worktree)
 		got_worktree_close(worktree);
 	if (repo) {
-		const struct got_error *repo_error;
-		repo_error = got_repo_close(repo);
+		const struct got_error *close_err = got_repo_close(repo);
 		if (error == NULL)
-			error = repo_error;
+			error = close_err;
 	}
 	got_ref_list_free(&refs);
 	return error;
@@ -4818,10 +4822,9 @@ done:
 	if (worktree)
 		got_worktree_close(worktree);
 	if (repo) {
-		const struct got_error *repo_error;
-		repo_error = got_repo_close(repo);
+		const struct got_error *close_err = got_repo_close(repo);
 		if (error == NULL)
-			error = repo_error;
+			error = close_err;
 	}
 	if (bca.lines) {
 		for (i = 0; i < bca.nlines; i++) {
@@ -5106,10 +5109,9 @@ done:
 	if (worktree)
 		got_worktree_close(worktree);
 	if (repo) {
-		const struct got_error *repo_error;
-		repo_error = got_repo_close(repo);
+		const struct got_error *close_err = got_repo_close(repo);
 		if (error == NULL)
-			error = repo_error;
+			error = close_err;
 	}
 	return error;
 }
@@ -5495,8 +5497,11 @@ cmd_ref(int argc, char *argv[])
 	}
 done:
 	free(refname);
-	if (repo)
-		got_repo_close(repo);
+	if (repo) {
+		const struct got_error *close_err = got_repo_close(repo);
+		if (error == NULL)
+			error = close_err;
+	}
 	if (worktree)
 		got_worktree_close(worktree);
 	free(cwd);
@@ -5877,8 +5882,11 @@ cmd_branch(int argc, char *argv[])
 done:
 	if (ref)
 		got_ref_close(ref);
-	if (repo)
-		got_repo_close(repo);
+	if (repo) {
+		const struct got_error *close_err = got_repo_close(repo);
+		if (error == NULL)
+			error = close_err;
+	}
 	if (worktree)
 		got_worktree_close(worktree);
 	free(cwd);
@@ -6387,8 +6395,11 @@ cmd_tag(int argc, char *argv[])
 		    commit_id_str ? commit_id_str : commit_id_arg, tagmsg);
 	}
 done:
-	if (repo)
-		got_repo_close(repo);
+	if (repo) {
+		const struct got_error *close_err = got_repo_close(repo);
+		if (error == NULL)
+			error = close_err;
+	}
 	if (worktree)
 		got_worktree_close(worktree);
 	free(cwd);
@@ -6519,8 +6530,11 @@ cmd_add(int argc, char *argv[])
 	error = got_worktree_schedule_add(worktree, &paths, add_progress,
 	    NULL, repo, no_ignores);
 done:
-	if (repo)
-		got_repo_close(repo);
+	if (repo) {
+		const struct got_error *close_err = got_repo_close(repo);
+		if (error == NULL)
+			error = close_err;
+	}
 	if (worktree)
 		got_worktree_close(worktree);
 	TAILQ_FOREACH(pe, &paths, entry)
@@ -6668,8 +6682,11 @@ cmd_remove(int argc, char *argv[])
 	    delete_local_mods, status_codes, print_remove_status, NULL,
 	    repo, keep_on_disk);
 done:
-	if (repo)
-		got_repo_close(repo);
+	if (repo) {
+		const struct got_error *close_err = got_repo_close(repo);
+		if (error == NULL)
+			error = close_err;
+	}
 	if (worktree)
 		got_worktree_close(worktree);
 	TAILQ_FOREACH(pe, &paths, entry)
@@ -6929,8 +6946,11 @@ done:
 	if (patch_script_file && fclose(patch_script_file) == EOF &&
 	    error == NULL)
 		error = got_error_from_errno2("fclose", patch_script_path);
-	if (repo)
-		got_repo_close(repo);
+	if (repo) {
+		const struct got_error *close_err = got_repo_close(repo);
+		if (error == NULL)
+			error = close_err;
+	}
 	if (worktree)
 		got_worktree_close(worktree);
 	free(path);
@@ -7234,8 +7254,11 @@ done:
 	    error == NULL)
 		error = got_error_from_errno2("unlink", cl_arg.logmsg_path);
 	free(cl_arg.logmsg_path);
-	if (repo)
-		got_repo_close(repo);
+	if (repo) {
+		const struct got_error *close_err = got_repo_close(repo);
+		if (error == NULL)
+			error = close_err;
+	}
 	if (worktree)
 		got_worktree_close(worktree);
 	free(cwd);
@@ -7365,8 +7388,11 @@ done:
 		got_ref_close(head_ref);
 	if (worktree)
 		got_worktree_close(worktree);
-	if (repo)
-		got_repo_close(repo);
+	if (repo) {
+		const struct got_error *close_err = got_repo_close(repo);
+		if (error == NULL)
+			error = close_err;
+	}
 	return error;
 }
 
@@ -7485,8 +7511,11 @@ done:
 		got_ref_close(head_ref);
 	if (worktree)
 		got_worktree_close(worktree);
-	if (repo)
-		got_repo_close(repo);
+	if (repo) {
+		const struct got_error *close_err = got_repo_close(repo);
+		if (error == NULL)
+			error = close_err;
+	}
 	return error;
 }
 
@@ -8322,8 +8351,11 @@ done:
 		got_ref_close(tmp_branch);
 	if (worktree)
 		got_worktree_close(worktree);
-	if (repo)
-		got_repo_close(repo);
+	if (repo) {
+		const struct got_error *close_err = got_repo_close(repo);
+		if (error == NULL)
+			error = close_err;
+	}
 	return error;
 }
 
@@ -9620,8 +9652,11 @@ done:
 		got_ref_close(tmp_branch);
 	if (worktree)
 		got_worktree_close(worktree);
-	if (repo)
-		got_repo_close(repo);
+	if (repo) {
+		const struct got_error *close_err = got_repo_close(repo);
+		if (error == NULL)
+			error = close_err;
+	}
 	return error;
 }
 
@@ -9753,8 +9788,11 @@ cmd_integrate(int argc, char *argv[])
 	printf("Integrated %s into %s\n", refname, base_refname);
 	print_update_progress_stats(&upa);
 done:
-	if (repo)
-		got_repo_close(repo);
+	if (repo) {
+		const struct got_error *close_err = got_repo_close(repo);
+		if (error == NULL)
+			error = close_err;
+	}
 	if (worktree)
 		got_worktree_close(worktree);
 	free(cwd);
@@ -9900,8 +9938,11 @@ done:
 	if (patch_script_file && fclose(patch_script_file) == EOF &&
 	    error == NULL)
 		error = got_error_from_errno2("fclose", patch_script_path);
-	if (repo)
-		got_repo_close(repo);
+	if (repo) {
+		const struct got_error *close_err = got_repo_close(repo);
+		if (error == NULL)
+			error = close_err;
+	}
 	if (worktree)
 		got_worktree_close(worktree);
 	TAILQ_FOREACH(pe, &paths, entry)
@@ -10010,8 +10051,11 @@ done:
 	if (patch_script_file && fclose(patch_script_file) == EOF &&
 	    error == NULL)
 		error = got_error_from_errno2("fclose", patch_script_path);
-	if (repo)
-		got_repo_close(repo);
+	if (repo) {
+		const struct got_error *close_err = got_repo_close(repo);
+		if (error == NULL)
+			error = close_err;
+	}
 	if (worktree)
 		got_worktree_close(worktree);
 	TAILQ_FOREACH(pe, &paths, entry)
@@ -10322,10 +10366,9 @@ done:
 	if (worktree)
 		got_worktree_close(worktree);
 	if (repo) {
-		const struct got_error *repo_error;
-		repo_error = got_repo_close(repo);
+		const struct got_error *close_err = got_repo_close(repo);
 		if (error == NULL)
-			error = repo_error;
+			error = close_err;
 	}
 	got_ref_list_free(&refs);
 	return error;
blob - 4cc5428af954c47772c98d07aff210d72929cc7c
file + gotweb/gotweb.c
--- gotweb/gotweb.c
+++ gotweb/gotweb.c
@@ -2833,8 +2833,11 @@ gw_get_repo_age(char **repo_age, struct gw_trans *gw_t
 	}
 done:
 	got_ref_list_free(&refs);
-	if (gw_trans->repo == NULL)
-		got_repo_close(repo);
+	if (gw_trans->repo == NULL) {
+		const struct got_error *close_err = got_repo_close(repo);
+		if (error == NULL)
+			error = close_err;
+	}
 	return error;
 }
 
@@ -2927,7 +2930,7 @@ done:
 static const struct got_error *
 gw_get_repo_owner(char **owner, struct gw_trans *gw_trans, char *dir)
 {
-	const struct got_error *error = NULL;
+	const struct got_error *error = NULL, *close_err;
 	struct got_repository *repo;
 	const char *gitconfig_owner;
 
@@ -2945,7 +2948,9 @@ gw_get_repo_owner(char **owner, struct gw_trans *gw_tr
 		if (*owner == NULL)
 			error = got_error_from_errno("strdup");
 	}
-	got_repo_close(repo);
+	close_err = got_repo_close(repo);
+	if (error == NULL)
+		error = close_err;
 	return error;
 }
 
@@ -4736,6 +4741,12 @@ main(int argc, char *argv[])
 	else
 		error = gw_display_index(gw_trans);
 done:
+	if (gw_trans->repo) {
+		const struct got_error *close_err;
+		close_err = got_repo_close(gw_trans->repo);
+		if (error == NULL)
+			error = close_err;
+	}
 	if (error) {
 		gw_trans->error = error;
 		gw_trans->action = GW_ERR;
@@ -4768,9 +4779,6 @@ cleanup:
 	free(gw_trans->prev_id);
 	free(gw_trans->prev_prev_id);
 	free(gw_trans->repo_path);
-	if (gw_trans->repo)
-		got_repo_close(gw_trans->repo);
-
 	TAILQ_FOREACH_SAFE(dir, &gw_trans->gw_dirs, entry, tdir) {
 		free(dir->name);
 		free(dir->description);
blob - 16616cca614bcd144f1e84477ab65ea3f34b7b0e
file + lib/repository.c
--- lib/repository.c
+++ lib/repository.c
@@ -752,8 +752,9 @@ got_repo_close(struct got_repository *repo)
 			err = got_error_from_errno("close");
 	}
 
-	if (repo->gitdir_fd != -1)
-		close(repo->gitdir_fd);
+	if (repo->gitdir_fd != -1 && close(repo->gitdir_fd) == -1 &&
+	    err == NULL)
+		err = got_error_from_errno("close");
 
 	if (repo->gotconfig)
 		got_gotconfig_free(repo->gotconfig);
blob - 13cc9fbaca9c08dbb86084c978e908bee7eebe17
file + lib/worktree.c
--- lib/worktree.c
+++ lib/worktree.c
@@ -436,8 +436,11 @@ open_worktree(struct got_worktree **worktree, const ch
 		goto done;
 	}
 done:
-	if (repo)
-		got_repo_close(repo);
+	if (repo) {
+		const struct got_error *close_err = got_repo_close(repo);
+		if (err == NULL)
+			err = close_err;
+	}
 	free(path_got);
 	free(path_lock);
 	free(base_commit_id_str);
blob - c426e9d11b25926d3d473cea50910a6e8ecd3240
file + tog/tog.c
--- tog/tog.c
+++ tog/tog.c
@@ -2111,7 +2111,7 @@ stop_log_thread(struct tog_log_view_state *s)
 	}
 
 	if (s->thread_args.repo) {
-		got_repo_close(s->thread_args.repo);
+		err = got_repo_close(s->thread_args.repo);
 		s->thread_args.repo = NULL;
 	}
 
@@ -2806,8 +2806,11 @@ done:
 	free(label);
 	if (ref)
 		got_ref_close(ref);
-	if (repo)
-		got_repo_close(repo);
+	if (repo) {
+		const struct got_error *close_err = got_repo_close(repo);
+		if (error == NULL)
+			error = close_err;
+	}
 	if (worktree)
 		got_worktree_close(worktree);
 	tog_free_refs();
@@ -3861,8 +3864,11 @@ done:
 	free(label2);
 	free(repo_path);
 	free(cwd);
-	if (repo)
-		got_repo_close(repo);
+	if (repo) {
+		const struct got_error *close_err = got_repo_close(repo);
+		if (error == NULL)
+			error = close_err;
+	}
 	if (worktree)
 		got_worktree_close(worktree);
 	tog_free_refs();
@@ -4092,7 +4098,7 @@ done:
 static void *
 blame_thread(void *arg)
 {
-	const struct got_error *err;
+	const struct got_error *err, *close_err;
 	struct tog_blame_thread_args *ta = arg;
 	struct tog_blame_cb_args *a = ta->cb_args;
 	int errcode;
@@ -4111,7 +4117,9 @@ blame_thread(void *arg)
 		return (void *)got_error_set_errno(errcode,
 		    "pthread_mutex_lock");
 
-	got_repo_close(ta->repo);
+	close_err = got_repo_close(ta->repo);
+	if (err == NULL)
+		err = close_err;
 	ta->repo = NULL;
 	*ta->complete = 1;
 
@@ -4162,7 +4170,10 @@ stop_blame(struct tog_blame *blame)
 		blame->thread = NULL;
 	}
 	if (blame->thread_args.repo) {
-		got_repo_close(blame->thread_args.repo);
+		const struct got_error *close_err;
+		close_err = got_repo_close(blame->thread_args.repo);
+		if (err == NULL)
+			err = close_err;
 		blame->thread_args.repo = NULL;
 	}
 	if (blame->f) {
@@ -4782,8 +4793,11 @@ done:
 	free(commit_id);
 	if (worktree)
 		got_worktree_close(worktree);
-	if (repo)
-		got_repo_close(repo);
+	if (repo) {
+		const struct got_error *close_err = got_repo_close(repo);
+		if (error == NULL)
+			error = close_err;
+	}
 	tog_free_refs();
 	return error;
 }
@@ -5613,8 +5627,11 @@ done:
 		got_object_commit_close(commit);
 	if (tree)
 		got_object_tree_close(tree);
-	if (repo)
-		got_repo_close(repo);
+	if (repo) {
+		const struct got_error *close_err = got_repo_close(repo);
+		if (error == NULL)
+			error = close_err;
+	}
 	tog_free_refs();
 	return error;
 }
@@ -6289,8 +6306,11 @@ cmd_ref(int argc, char *argv[])
 done:
 	free(repo_path);
 	free(cwd);
-	if (repo)
-		got_repo_close(repo);
+	if (repo) {
+		const struct got_error *close_err = got_repo_close(repo);
+		if (close_err)
+			error = close_err;
+	}
 	tog_free_refs();
 	return error;
 }
@@ -6354,7 +6374,7 @@ make_argv(int argc, ...)
 static const struct got_error *
 tog_log_with_path(int argc, char *argv[])
 {
-	const struct got_error *error = NULL;
+	const struct got_error *error = NULL, *close_err;
 	struct tog_cmd *cmd = NULL;
 	struct got_repository *repo = NULL;
 	struct got_worktree *worktree = NULL;
@@ -6412,7 +6432,9 @@ tog_log_with_path(int argc, char *argv[])
 		/* not reached */
 	}
 
-	got_repo_close(repo);
+	close_err = got_repo_close(repo);
+	if (error == NULL)
+		error = close_err;
 	repo = NULL;
 
 	error = got_object_id_str(&commit_id_str, commit_id);
@@ -6424,8 +6446,11 @@ tog_log_with_path(int argc, char *argv[])
 	cmd_argv = make_argv(argc, cmd->name, "-c", commit_id_str, argv[0]);
 	error = cmd->cmd_main(argc, cmd_argv);
 done:
-	if (repo)
-		got_repo_close(repo);
+	if (repo) {
+		close_err = got_repo_close(repo);
+		if (error == NULL)
+			error = close_err;
+	}
 	if (worktree)
 		got_worktree_close(worktree);
 	free(id);