Re: [PATCH] tmp-objdir: do not opendir() when handling a signal
From: Taylor Blau <hidden>
Date: 2022-09-27 00:18:53
[+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.
quoted hunk ↗ jump to hunk
@@ -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.
quoted hunk ↗ jump to hunk
@@ -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.
quoted hunk ↗ jump to hunk
@@ -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