Re: [PATCH] tmp-objdir: do not opendir() when handling a signal

[Junio C Hamano <gitster@pobox.com>]

"John Cai via GitGitGadget" <gitgitgadget@gmail.com> writes:

>     tmp-objdir: do not closedir() when handling a signal
>     
>     We have recently observed a Git process hanging around for weeks. A
>     backtrace revealed that a git-receive-pack(1) process was deadlocked
>     when trying to remove the quarantine directory "incoming." It turns out
>     that the tmp_objdir API calls opendir(3) and closedir(3) to observe a
>     directory's contents in order to remove all the contents before removing
>     the directory itself. These functions are not async signal save as they
>     allocate and free memory.
>     
>     The fix is to avoid calling these functions when handling a signal in
>     order to avoid a deadlock. The implication of such a fix however, is
>     that temporary object directories may not get cleaned up properly when a
>     signal is being handled. The tradeoff this fix is making is to prevent
>     deadlocks at the cost of temporary object directory cleanup.
>     
>     This is similar to 58d4d7f1c5 (2022-01-07 fetch: fix deadlock when
>     cleaning up lockfiles in async signals)

Hmph, is it really similar?  That one, even though the lockfiles
won't be cleaned up inside signal handler, they will eventually be
cleaned, won't they?  As opposed to here, once we punt, we punt and
do not revisit when we re-raise and eventually exit, no?

Leaving temporary directories behind is MUCH MUCH better than
getting stuck in a deadlock, so it is much better than the status
quo, of course.

>  static int remove_dir_recurse(struct strbuf *path, int flag, int *kept_up)
>  {
> -	DIR *dir;
> +	DIR *dir = NULL;
>  	struct dirent *e;
>  	int ret = 0, original_len = path->len, len, kept_down = 0;
>  	int only_empty = (flag & REMOVE_DIR_EMPTY_ONLY);
> @@ -3261,7 +3261,10 @@ static int remove_dir_recurse(struct strbuf *path, int flag, int *kept_up)
>  	}
>  
>  	flag &= ~REMOVE_DIR_KEEP_TOPLEVEL;
> -	dir = opendir(path->buf);
> +
> +	if ((flag & REMOVE_DIR_SIGNAL) == 0)
> +		dir = opendir(path->buf);
> +
>  	if (!dir) {
>  		if (errno == ENOENT)
>  			return keep_toplevel ? -1 : 0;
> diff --git a/dir.h b/dir.h
> index 674747d93af..ba159f4abeb 100644
> --- a/dir.h
> +++ b/dir.h
> @@ -498,6 +498,9 @@ int get_sparse_checkout_patterns(struct pattern_list *pl);
>  /* Remove the_original_cwd too */
>  #define REMOVE_DIR_PURGE_ORIGINAL_CWD 0x08
>  
> +/* Indicates a signal is being handled */
> +#define REMOVE_DIR_SIGNAL 0x16
> +
>  /*
>   * Remove path and its contents, recursively. flags is a combination
>   * of the above REMOVE_DIR_* constants. Return 0 on success.
> diff --git a/tmp-objdir.c b/tmp-objdir.c

The fix looks quite straight-forward.

Thanks for spotting and working on this issue.