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