From: Tracey Emery <tracey@traceyemery.net>
Subject: Re: fds not getting properly closed
To: gameoftrees@openbsd.org
Date: Wed, 16 Jun 2021 08:10:56 -0600

On Wed, Jun 16, 2021 at 10:28:37AM +0200, Stefan Sperling wrote:
> 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.
> 

Of course, you're right. I was lazy with my patch. This all looks ok to
me.

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

-- 

Tracey Emery