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

From: Taylor Blau <me@ttaylorr.com>
Date: Sep 27, 2022, 00:18 UTC
Message-ID: <YzJBZ9QJWIv0hpXb@nand.local>
In-Reply-To: <pull.1348.git.git.1664236383785.gitgitgadget@gmail.com>

[+cc Peff as the author of tmp-objdir]

On Mon, Sep 26, 2022 at 11:53:03PM +0000, John Cai via GitGitGadget wrote:

> One place we call tmp_objdir_create() is in git-receive-pack, where
> we create a temporary quarantine directory "incoming". Incoming
> objects will be written to this directory before they get moved to
> the object directory.

Right, calling opendir() will allocate memory, so we'll get stuck in a deadlock if the signal arrives while libc's allocator lock is held. So we can't safely call opendir() there.

It does make me a little uneasy leaving the quarantine directory around via this path. So I wonder if we should be optimistically opening up the DIR handle? Calling unlink() in a signal is perfectly fine, so I'd think as long as we have an open DIR handle we could call readdir_r(), but I don't think we've discussed it before.

Show 7 quoted lines

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

Comparing to the zero value is discouraged. Consider:

    if (!(flag & REMOVE_DIR_SIGNAL))

instead.

Show 7 quoted lines

> @@ -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
> +

Perhaps REMOVE_DIR_IN_SIGNAL would be slightly more descriptive.

Show 8 quoted lines

> @@ -49,7 +50,11 @@ static int tmp_objdir_destroy_1(struct tmp_objdir *t, int on_signal)
>  	 * have pre-grown t->path sufficiently so that this
>  	 * doesn't happen in practice.
>  	 */
> -	err = remove_dir_recursively(&t->path, 0);
> +
> +	if (on_signal)
> +		flags = flags | REMOVE_DIR_SIGNAL;

I'm nitpicking, but you could just write "flags |= REMOVE_DIR_SIGNAL", or even:

    err = remove_dir_recursively(&t->path,
                                 on_signal ? REMOVE_DIR_SIGNAL : 0);

Thanks, Taylor