{"thread":{"id":"58517","subject":"[PATCH] tmp-objdir: do not opendir() when handling a signal","startedAt":"2022-09-26T23:53:10Z","lastAt":"2022-10-20T18:21:09Z","messageCount":18,"participants":["John Cai via GitGitGadget","Taylor Blau","Junio C Hamano","Phillip Wood","Jeff King","John Cai","Ævar Arnfjörð Bjarmason","Jan Pokorný"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"463686","messageId":"pull.1348.git.git.1664236383785.gitgitgadget@gmail.com","threadId":"58517","inReplyTo":null,"subject":"[PATCH] tmp-objdir: do not opendir() when handling a signal","fromName":"John Cai via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-09-26T23:53:03Z","receivedAt":"2022-09-26T23:53:10Z","isPatch":true,"sender":{"key":"johncai86@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2354211?v=4"},"body":"From: John Cai <johncai86@gmail.com>\n\nIn the tmp-objdir api, tmp_objdir_create will create a temporary\ndirectory but also register signal handlers responsible for removing\nthe directory's contents and the directory itself. However, the\nfunction responsible for recursively removing the contents and\ndirectory, remove_dir_recurse() calls opendir(3) and closedir(3).\nThis can be problematic because these functions allocate and free\nmemory, which are not async-signal-safe functions. This can lead to\ndeadlocks.\n\nOne place we call tmp_objdir_create() is in git-receive-pack, where\nwe create a temporary quarantine directory \"incoming\". Incoming\nobjects will be written to this directory before they get moved to\nthe object directory.\n\nWe have observed this code leading to a deadlock:\n\n\tThread 1 (Thread 0x7f621ba0b200 (LWP 326305)):\n\t#0  __lll_lock_wait_private (futex=futex@entry=0x7f621bbf8b80\n\t\t<main_arena>) at ./lowlevellock.c:35\n\t#1  0x00007f621baa635b in __GI___libc_malloc\n\t\t(bytes=bytes@entry=32816) at malloc.c:3064\n\t#2  0x00007f621bae9f49 in __alloc_dir (statp=0x7fff2ea7ed60,\n\t\tflags=0, close_fd=true, fd=5)\n\t\tat ../sysdeps/posix/opendir.c:118\n\t#3  opendir_tail (fd=5) at ../sysdeps/posix/opendir.c:69\n\t#4  __opendir (name=<optimized out>)\n\t\tat ../sysdeps/posix/opendir.c:92\n\t#5  0x0000557c19c77de1 in remove_dir_recurse ()\n\t#6  0x0000557c19d81a4f in remove_tmp_objdir_on_signal ()\n\t#7  <signal handler called>\n\t#8  _int_malloc (av=av@entry=0x7f621bbf8b80 <main_arena>,\n\t\tbytes=bytes@entry=7160) at malloc.c:4116\n\t#9  0x00007f621baa62c9 in __GI___libc_malloc (bytes=7160)\n\t\tat malloc.c:3066\n\t#10 0x00007f621bd1e987 in inflateInit2_ ()\n\t\tfrom /opt/gitlab/embedded/lib/libz.so.1\n\t#11 0x0000557c19dbe5f4 in git_inflate_init ()\n\t#12 0x0000557c19cee02a in unpack_compressed_entry ()\n\t#13 0x0000557c19cf08cb in unpack_entry ()\n\t#14 0x0000557c19cf0f32 in packed_object_info ()\n\t#15 0x0000557c19cd68cd in do_oid_object_info_extended ()\n\t#16 0x0000557c19cd6e2b in read_object_file_extended ()\n\t#17 0x0000557c19cdec2f in parse_object ()\n\t#18 0x0000557c19c34977 in lookup_commit_reference_gently ()\n\t#19 0x0000557c19d69309 in mark_uninteresting ()\n\t#20 0x0000557c19d2d180 in do_for_each_repo_ref_iterator ()\n\t#21 0x0000557c19d21678 in for_each_ref ()\n\t#22 0x0000557c19d6a94f in assign_shallow_commits_to_refs ()\n\t#23 0x0000557c19bc02b2 in cmd_receive_pack ()\n\t#24 0x0000557c19b29fdd in handle_builtin ()\n\t#25 0x0000557c19b2a526 in cmd_main ()\n\t#26 0x0000557c19b28ea2 in main ()\n\nTo prevent this, add a flag REMOVE_DIR_SIGNAL that allows\nremove_dir_recurse() to know that a signal is being handled and avoid\ncalling opendir(3). One implication of this change is that when\nsignal handling, the temporary directory may not get cleaned up\nproperly.\n\nSigned-off-by: John Cai <johncai86@gmail.com>\n---\n    tmp-objdir: do not closedir() when handling a signal\n    \n    We have recently observed a Git process hanging around for weeks. A\n    backtrace revealed that a git-receive-pack(1) process was deadlocked\n    when trying to remove the quarantine directory \"incoming.\" It turns out\n    that the tmp_objdir API calls opendir(3) and closedir(3) to observe a\n    directory's contents in order to remove all the contents before removing\n    the directory itself. These functions are not async signal save as they\n    allocate and free memory.\n    \n    The fix is to avoid calling these functions when handling a signal in\n    order to avoid a deadlock. The implication of such a fix however, is\n    that temporary object directories may not get cleaned up properly when a\n    signal is being handled. The tradeoff this fix is making is to prevent\n    deadlocks at the cost of temporary object directory cleanup.\n    \n    This is similar to 58d4d7f1c5 (2022-01-07 fetch: fix deadlock when\n    cleaning up lockfiles in async signals)\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1348%2Fjohn-cai%2Fjc%2Ffix-tmp-objdir-signal-handling-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1348/john-cai/jc/fix-tmp-objdir-signal-handling-v1\nPull-Request: https://github.com/git/git/pull/1348\n\n dir.c        | 7 +++++--\n dir.h        | 3 +++\n tmp-objdir.c | 7 ++++++-\n 3 files changed, 14 insertions(+), 3 deletions(-)\n\ndiff --git a/dir.c b/dir.c\nindex 75429508200..a3183cb043f 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -3244,7 +3244,7 @@ void strip_dir_trailing_slashes(char *dir)\n \n static int remove_dir_recurse(struct strbuf *path, int flag, int *kept_up)\n {\n-\tDIR *dir;\n+\tDIR *dir = NULL;\n \tstruct dirent *e;\n \tint ret = 0, original_len = path->len, len, kept_down = 0;\n \tint only_empty = (flag & REMOVE_DIR_EMPTY_ONLY);\n@@ -3261,7 +3261,10 @@ static int remove_dir_recurse(struct strbuf *path, int flag, int *kept_up)\n \t}\n \n \tflag &= ~REMOVE_DIR_KEEP_TOPLEVEL;\n-\tdir = opendir(path->buf);\n+\n+\tif ((flag & REMOVE_DIR_SIGNAL) == 0)\n+\t\tdir = opendir(path->buf);\n+\n \tif (!dir) {\n \t\tif (errno == ENOENT)\n \t\t\treturn keep_toplevel ? -1 : 0;\ndiff --git a/dir.h b/dir.h\nindex 674747d93af..ba159f4abeb 100644\n--- a/dir.h\n+++ b/dir.h\n@@ -498,6 +498,9 @@ int get_sparse_checkout_patterns(struct pattern_list *pl);\n /* Remove the_original_cwd too */\n #define REMOVE_DIR_PURGE_ORIGINAL_CWD 0x08\n \n+/* Indicates a signal is being handled */\n+#define REMOVE_DIR_SIGNAL 0x16\n+\n /*\n  * Remove path and its contents, recursively. flags is a combination\n  * of the above REMOVE_DIR_* constants. Return 0 on success.\ndiff --git a/tmp-objdir.c b/tmp-objdir.c\nindex adf6033549e..13943098738 100644\n--- a/tmp-objdir.c\n+++ b/tmp-objdir.c\n@@ -34,6 +34,7 @@ static void tmp_objdir_free(struct tmp_objdir *t)\n static int tmp_objdir_destroy_1(struct tmp_objdir *t, int on_signal)\n {\n \tint err;\n+\tint flags = 0;\n \n \tif (!t)\n \t\treturn 0;\n@@ -49,7 +50,11 @@ static int tmp_objdir_destroy_1(struct tmp_objdir *t, int on_signal)\n \t * have pre-grown t->path sufficiently so that this\n \t * doesn't happen in practice.\n \t */\n-\terr = remove_dir_recursively(&t->path, 0);\n+\n+\tif (on_signal)\n+\t\tflags = flags | REMOVE_DIR_SIGNAL;\n+\n+\terr = remove_dir_recursively(&t->path, flags);\n \n \t/*\n \t * When we are cleaning up due to a signal, we won't bother\n\nbase-commit: 4fd6c5e44459e6444c2cd93383660134c95aabd1\n-- \ngitgitgadget\n"},{"id":"463687","messageId":"YzJBZ9QJWIv0hpXb@nand.local","threadId":"58517","inReplyTo":"pull.1348.git.git.1664236383785.gitgitgadget@gmail.com","subject":"Re: [PATCH] tmp-objdir: do not opendir() when handling a signal","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-09-27T00:18:47Z","receivedAt":"2022-09-27T00:18:53Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"[+cc Peff as the author of tmp-objdir]\n\nOn Mon, Sep 26, 2022 at 11:53:03PM +0000, John Cai via GitGitGadget wrote:\n> One place we call tmp_objdir_create() is in git-receive-pack, where\n> we create a temporary quarantine directory \"incoming\". Incoming\n> objects will be written to this directory before they get moved to\n> the object directory.\n\nRight, calling opendir() will allocate memory, so we'll get stuck in a\ndeadlock if the signal arrives while libc's allocator lock is held. So\nwe can't safely call opendir() there.\n\nIt does make me a little uneasy leaving the quarantine directory around\nvia this path. So I wonder if we should be optimistically opening up the\nDIR handle? Calling unlink() in a signal is perfectly fine, so I'd think\nas long as we have an open DIR handle we could call readdir_r(), but I\ndon't think we've discussed it before.\n\n> @@ -3261,7 +3261,10 @@ static int remove_dir_recurse(struct strbuf *path, int flag, int *kept_up)\n>  \t}\n>\n>  \tflag &= ~REMOVE_DIR_KEEP_TOPLEVEL;\n> -\tdir = opendir(path->buf);\n> +\n> +\tif ((flag & REMOVE_DIR_SIGNAL) == 0)\n\nComparing to the zero value is discouraged. Consider:\n\n    if (!(flag & REMOVE_DIR_SIGNAL))\n\ninstead.\n\n\n> @@ -498,6 +498,9 @@ int get_sparse_checkout_patterns(struct pattern_list *pl);\n>  /* Remove the_original_cwd too */\n>  #define REMOVE_DIR_PURGE_ORIGINAL_CWD 0x08\n>\n> +/* Indicates a signal is being handled */\n> +#define REMOVE_DIR_SIGNAL 0x16\n> +\n\nPerhaps REMOVE_DIR_IN_SIGNAL would be slightly more descriptive.\n\n> @@ -49,7 +50,11 @@ static int tmp_objdir_destroy_1(struct tmp_objdir *t, int on_signal)\n>  \t * have pre-grown t->path sufficiently so that this\n>  \t * doesn't happen in practice.\n>  \t */\n> -\terr = remove_dir_recursively(&t->path, 0);\n> +\n> +\tif (on_signal)\n> +\t\tflags = flags | REMOVE_DIR_SIGNAL;\n\nI'm nitpicking, but you could just write \"flags |= REMOVE_DIR_SIGNAL\",\nor even:\n\n    err = remove_dir_recursively(&t->path,\n                                 on_signal ? REMOVE_DIR_SIGNAL : 0);\n\nThanks,\nTaylor\n"},{"id":"463690","messageId":"xmqqill9a93z.fsf@gitster.g","threadId":"58517","inReplyTo":"pull.1348.git.git.1664236383785.gitgitgadget@gmail.com","subject":"Re: [PATCH] tmp-objdir: do not opendir() when handling a signal","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-09-27T01:39:44Z","receivedAt":"2022-09-27T01:39:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"John Cai via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n>     tmp-objdir: do not closedir() when handling a signal\n>     \n>     We have recently observed a Git process hanging around for weeks. A\n>     backtrace revealed that a git-receive-pack(1) process was deadlocked\n>     when trying to remove the quarantine directory \"incoming.\" It turns out\n>     that the tmp_objdir API calls opendir(3) and closedir(3) to observe a\n>     directory's contents in order to remove all the contents before removing\n>     the directory itself. These functions are not async signal save as they\n>     allocate and free memory.\n>     \n>     The fix is to avoid calling these functions when handling a signal in\n>     order to avoid a deadlock. The implication of such a fix however, is\n>     that temporary object directories may not get cleaned up properly when a\n>     signal is being handled. The tradeoff this fix is making is to prevent\n>     deadlocks at the cost of temporary object directory cleanup.\n>     \n>     This is similar to 58d4d7f1c5 (2022-01-07 fetch: fix deadlock when\n>     cleaning up lockfiles in async signals)\n\nHmph, is it really similar?  That one, even though the lockfiles\nwon't be cleaned up inside signal handler, they will eventually be\ncleaned, won't they?  As opposed to here, once we punt, we punt and\ndo not revisit when we re-raise and eventually exit, no?\n\nLeaving temporary directories behind is MUCH MUCH better than\ngetting stuck in a deadlock, so it is much better than the status\nquo, of course.\n\n>  static int remove_dir_recurse(struct strbuf *path, int flag, int *kept_up)\n>  {\n> -\tDIR *dir;\n> +\tDIR *dir = NULL;\n>  \tstruct dirent *e;\n>  \tint ret = 0, original_len = path->len, len, kept_down = 0;\n>  \tint only_empty = (flag & REMOVE_DIR_EMPTY_ONLY);\n> @@ -3261,7 +3261,10 @@ static int remove_dir_recurse(struct strbuf *path, int flag, int *kept_up)\n>  \t}\n>  \n>  \tflag &= ~REMOVE_DIR_KEEP_TOPLEVEL;\n> -\tdir = opendir(path->buf);\n> +\n> +\tif ((flag & REMOVE_DIR_SIGNAL) == 0)\n> +\t\tdir = opendir(path->buf);\n> +\n>  \tif (!dir) {\n>  \t\tif (errno == ENOENT)\n>  \t\t\treturn keep_toplevel ? -1 : 0;\n> diff --git a/dir.h b/dir.h\n> index 674747d93af..ba159f4abeb 100644\n> --- a/dir.h\n> +++ b/dir.h\n> @@ -498,6 +498,9 @@ int get_sparse_checkout_patterns(struct pattern_list *pl);\n>  /* Remove the_original_cwd too */\n>  #define REMOVE_DIR_PURGE_ORIGINAL_CWD 0x08\n>  \n> +/* Indicates a signal is being handled */\n> +#define REMOVE_DIR_SIGNAL 0x16\n> +\n>  /*\n>   * Remove path and its contents, recursively. flags is a combination\n>   * of the above REMOVE_DIR_* constants. Return 0 on success.\n> diff --git a/tmp-objdir.c b/tmp-objdir.c\n\nThe fix looks quite straight-forward.\n\nThanks for spotting and working on this issue.\n\n"},{"id":"463704","messageId":"99c45b89-364f-4f77-7c1b-26231cc7455e@dunelm.org.uk","threadId":"58517","inReplyTo":"pull.1348.git.git.1664236383785.gitgitgadget@gmail.com","subject":"Re: [PATCH] tmp-objdir: do not opendir() when handling a signal","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2022-09-27T09:18:32Z","receivedAt":"2022-09-27T09:20:14Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi John\n\nOn 27/09/2022 00:53, John Cai via GitGitGadget wrote:\n> From: John Cai <johncai86@gmail.com>\n> \n> In the tmp-objdir api, tmp_objdir_create will create a temporary\n> directory but also register signal handlers responsible for removing\n> the directory's contents and the directory itself. However, the\n> function responsible for recursively removing the contents and\n> directory, remove_dir_recurse() calls opendir(3) and closedir(3).\n> This can be problematic because these functions allocate and free\n> memory, which are not async-signal-safe functions. This can lead to\n> deadlocks.\n\n> --- a/dir.h\n> +++ b/dir.h\n> @@ -498,6 +498,9 @@ int get_sparse_checkout_patterns(struct pattern_list *pl);\n>   /* Remove the_original_cwd too */\n>   #define REMOVE_DIR_PURGE_ORIGINAL_CWD 0x08\n>   \n> +/* Indicates a signal is being handled */\n> +#define REMOVE_DIR_SIGNAL 0x16\n\nThis is setting the bits for REMOVE_DIR_KEEP_NESTED_GIT and \nREMOVE_DIR_KEEP_TOPLEVEL is that intentional? (it looks like you've \ndoubled 8 to 16 to get the next free bit but used a hex constant, the \nearlier constants use decimal)\n\nBest Wishes\n\nPhillip\n"},{"id":"463707","messageId":"YzLiI1HZeBszsIJq@coredump.intra.peff.net","threadId":"58517","inReplyTo":"pull.1348.git.git.1664236383785.gitgitgadget@gmail.com","subject":"Re: [PATCH] tmp-objdir: do not opendir() when handling a signal","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-09-27T11:44:35Z","receivedAt":"2022-09-27T11:44:43Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Sep 26, 2022 at 11:53:03PM +0000, John Cai via GitGitGadget wrote:\n\n> One place we call tmp_objdir_create() is in git-receive-pack, where\n> we create a temporary quarantine directory \"incoming\". Incoming\n> objects will be written to this directory before they get moved to\n> the object directory.\n> \n> We have observed this code leading to a deadlock:\n> \n> \tThread 1 (Thread 0x7f621ba0b200 (LWP 326305)):\n> \t#0  __lll_lock_wait_private (futex=futex@entry=0x7f621bbf8b80\n> \t\t<main_arena>) at ./lowlevellock.c:35\n> \t#1  0x00007f621baa635b in __GI___libc_malloc\n> \t\t(bytes=bytes@entry=32816) at malloc.c:3064\n> \t#2  0x00007f621bae9f49 in __alloc_dir (statp=0x7fff2ea7ed60,\n> \t\tflags=0, close_fd=true, fd=5)\n> \t\tat ../sysdeps/posix/opendir.c:118\n> \t#3  opendir_tail (fd=5) at ../sysdeps/posix/opendir.c:69\n> \t#4  __opendir (name=<optimized out>)\n> \t\tat ../sysdeps/posix/opendir.c:92\n> \t#5  0x0000557c19c77de1 in remove_dir_recurse ()\n> \t#6  0x0000557c19d81a4f in remove_tmp_objdir_on_signal ()\n> \t#7  <signal handler called>\n\nYuck. Your analysis looks right, and certainly opendir() can't really\nwork without allocating memory for the pointer-to-DIR.\n\n> To prevent this, add a flag REMOVE_DIR_SIGNAL that allows\n> remove_dir_recurse() to know that a signal is being handled and avoid\n> calling opendir(3). One implication of this change is that when\n> signal handling, the temporary directory may not get cleaned up\n> properly.\n\nIt's not really \"may not\" here, is it? It will never get cleaned up on a\nsignal now. I don't think remove_dir_recursively() will try to rmdir()\nin this case. But even if it did, we'll always have a \"pack\"\nsubdirectory (minus the small race before we've created it).\n\nThat's unfortunate, but I don't think we really have a portable\nalternative. We can't keep an exact list of files to be deleted, because\nsome of them will be created by sub-processes. We could perhaps exec a\nhelper that does the deletion, but that seems like a race and\nportability nightmare. On Linux, we could probably use open() and\ngetdents64() to traverse, but obviously that won't work everywhere. It\n_might_ be worth having some kind of compat/ wrapper here, to let\nsupported systems do as good a job as they can. But it's not like there\naren't already cases where we might leave the tmp-objdir directory\naround (say, SIGKILL), so this is really just extending the existing\nproblem to more signals.\n\nI was going to suggest we should do a better job of cleaning up these\ndirectories via git-gc. But it looks like b3cecf49ea (tmp-objdir: new\nAPI for creating temporary writable databases, 2021-12-06) changed the\ndefault name such that a regular git-gc should do so. So I think we're\ncovered there.\n\nThe main case we care about is normal exit when index-pack or a hook\nsees an error, in which case we should still be cleaning up via the\natexit() handler.\n\nSo I think your patch is going in the right direction, but...\n\n>  static int remove_dir_recurse(struct strbuf *path, int flag, int *kept_up)\n>  {\n> -\tDIR *dir;\n> +\tDIR *dir = NULL;\n>  \tstruct dirent *e;\n>  \tint ret = 0, original_len = path->len, len, kept_down = 0;\n>  \tint only_empty = (flag & REMOVE_DIR_EMPTY_ONLY);\n> @@ -3261,7 +3261,10 @@ static int remove_dir_recurse(struct strbuf *path, int flag, int *kept_up)\n>  \t}\n>  \n>  \tflag &= ~REMOVE_DIR_KEEP_TOPLEVEL;\n> -\tdir = opendir(path->buf);\n> +\n> +\tif ((flag & REMOVE_DIR_SIGNAL) == 0)\n> +\t\tdir = opendir(path->buf);\n> +\n>  \tif (!dir) {\n>  \t\tif (errno == ENOENT)\n>  \t\t\treturn keep_toplevel ? -1 : 0;\n\nWe skip calling opendir() entirely, so \"dir\" will still be NULL. But we\nimmediately start looking at errno, which will have some undefined value\n(based on some previous syscall).\n\nIf we set \"errno\" to \"EACCES\" in this case, then we'd at least hit the\nrmdir() below:\n\n         if (!dir) {\n                  if (errno == ENOENT)\n                          return keep_toplevel ? -1 : 0;\n                  else if (errno == EACCES && !keep_toplevel)\n                          /*\n                           * An empty dir could be removable even if it\n                           * is unreadable:\n                           */\n                          return rmdir(path->buf);\n                  else\n                          return -1;\n          }\n\nbut we know it won't really do anything for our proposed caller, since\nit will have files inside the directory that need to be removed before\nrmdir() can work.\n\nMoreover, if you were to combine REMOVE_DIR_SIGNAL with\nREMOVE_DIR_KEEP_NESTED_GIT, I suspect that the call to\nresolve_gitlink_ref() would end up with similar deadlocks. Obviously\nnobody is proposing to do that, but it is a pitfall in the API.\n\nSo all of that makes me think we should not add a new flag here, but\ninstead just avoid calling the function entirely from\ntmp_objdir_destroy_1().\n\nBut then we can observe that tmp_objdir_destroy_1() is basically doing\nnothing if on_signal is set. So there is really no point in setting up\nthe signal handler at all. We should just set up the atexit() handler.\nI.e., something like:\n\ndiff --git a/tmp-objdir.c b/tmp-objdir.c\nindex a8be92bca1..10549e95db 100644\n--- a/tmp-objdir.c\n+++ b/tmp-objdir.c\n@@ -169,7 +169,6 @@ struct tmp_objdir *tmp_objdir_create(const char *prefix)\n \tthe_tmp_objdir = t;\n \tif (!installed_handlers) {\n \t\tatexit(remove_tmp_objdir);\n-\t\tsigchain_push_common(remove_tmp_objdir_on_signal);\n \t\tinstalled_handlers++;\n \t}\n \n\nwith the commit message explaining that we can't do the cleanup in a\nportable and signal-safe way, so we just punt on the whole concept.\n\nThere's also some minor cleanup we could do elsewhere to drop the\n\"on_signal\" argument (which can come as part of the same patch, or on\ntop).\n\n-Peff\n"},{"id":"463708","messageId":"YzLjKbLI13GN52R/@coredump.intra.peff.net","threadId":"58517","inReplyTo":"YzJBZ9QJWIv0hpXb@nand.local","subject":"Re: [PATCH] tmp-objdir: do not opendir() when handling a signal","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-09-27T11:48:57Z","receivedAt":"2022-09-27T11:49:52Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Sep 26, 2022 at 08:18:47PM -0400, Taylor Blau wrote:\n\n> It does make me a little uneasy leaving the quarantine directory around\n> via this path. So I wonder if we should be optimistically opening up the\n> DIR handle? Calling unlink() in a signal is perfectly fine, so I'd think\n> as long as we have an open DIR handle we could call readdir_r(), but I\n> don't think we've discussed it before.\n\nYou'd need to hold multiple such DIRs, since the removal is recursive.\nIt's easy-ish for the \"pack\" directory, but the sub-process index-pack\nmay have created 00-ff directories, too. You'd have to pre-create and\nopendir() all of them.\n\nI'm also not sure what timing guarantees we have. If I opendir() a\ndirectory, then wait a long time while somebody else creates entries, is\na readdir() guaranteed to see those new entries?\n\n-Peff\n"},{"id":"463712","messageId":"1B4D6262-532A-401E-B1A2-709BEECA4B67@gmail.com","threadId":"58517","inReplyTo":"YzLiI1HZeBszsIJq@coredump.intra.peff.net","subject":"Re: [PATCH] tmp-objdir: do not opendir() when handling a signal","fromName":"John Cai","fromEmail":"johncai86@gmail.com","sentAt":"2022-09-27T13:50:43Z","receivedAt":"2022-09-27T13:50:49Z","isPatch":true,"sender":{"key":"johncai86@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2354211?v=4"},"body":"Hey Peff,\n\nOn 27 Sep 2022, at 7:44, Jeff King wrote:\n\n> On Mon, Sep 26, 2022 at 11:53:03PM +0000, John Cai via GitGitGadget wrote:\n>\n>> One place we call tmp_objdir_create() is in git-receive-pack, where\n>> we create a temporary quarantine directory \"incoming\". Incoming\n>> objects will be written to this directory before they get moved to\n>> the object directory.\n>>\n>> We have observed this code leading to a deadlock:\n>>\n>> \tThread 1 (Thread 0x7f621ba0b200 (LWP 326305)):\n>> \t#0  __lll_lock_wait_private (futex=futex@entry=0x7f621bbf8b80\n>> \t\t<main_arena>) at ./lowlevellock.c:35\n>> \t#1  0x00007f621baa635b in __GI___libc_malloc\n>> \t\t(bytes=bytes@entry=32816) at malloc.c:3064\n>> \t#2  0x00007f621bae9f49 in __alloc_dir (statp=0x7fff2ea7ed60,\n>> \t\tflags=0, close_fd=true, fd=5)\n>> \t\tat ../sysdeps/posix/opendir.c:118\n>> \t#3  opendir_tail (fd=5) at ../sysdeps/posix/opendir.c:69\n>> \t#4  __opendir (name=<optimized out>)\n>> \t\tat ../sysdeps/posix/opendir.c:92\n>> \t#5  0x0000557c19c77de1 in remove_dir_recurse ()\n>> \t#6  0x0000557c19d81a4f in remove_tmp_objdir_on_signal ()\n>> \t#7  <signal handler called>\n>\n> Yuck. Your analysis looks right, and certainly opendir() can't really\n> work without allocating memory for the pointer-to-DIR.\n>\n>> To prevent this, add a flag REMOVE_DIR_SIGNAL that allows\n>> remove_dir_recurse() to know that a signal is being handled and avoid\n>> calling opendir(3). One implication of this change is that when\n>> signal handling, the temporary directory may not get cleaned up\n>> properly.\n>\n> It's not really \"may not\" here, is it? It will never get cleaned up on a\n> signal now. I don't think remove_dir_recursively() will try to rmdir()\n> in this case. But even if it did, we'll always have a \"pack\"\n> subdirectory (minus the small race before we've created it).\n>\n> That's unfortunate, but I don't think we really have a portable\n> alternative. We can't keep an exact list of files to be deleted, because\n> some of them will be created by sub-processes. We could perhaps exec a\n> helper that does the deletion, but that seems like a race and\n> portability nightmare. On Linux, we could probably use open() and\n> getdents64() to traverse, but obviously that won't work everywhere. It\n> _might_ be worth having some kind of compat/ wrapper here, to let\n> supported systems do as good a job as they can. But it's not like there\n> aren't already cases where we might leave the tmp-objdir directory\n> around (say, SIGKILL), so this is really just extending the existing\n> problem to more signals.\n>\n> I was going to suggest we should do a better job of cleaning up these\n> directories via git-gc. But it looks like b3cecf49ea (tmp-objdir: new\n> API for creating temporary writable databases, 2021-12-06) changed the\n> default name such that a regular git-gc should do so. So I think we're\n> covered there.\n\nI was wondering about this as well. Thanks for checking on this--that's\nreassuring.\n>\n> The main case we care about is normal exit when index-pack or a hook\n> sees an error, in which case we should still be cleaning up via the\n> atexit() handler.\n>\n> So I think your patch is going in the right direction, but...\n>\n>>  static int remove_dir_recurse(struct strbuf *path, int flag, int *kept_up)\n>>  {\n>> -\tDIR *dir;\n>> +\tDIR *dir = NULL;\n>>  \tstruct dirent *e;\n>>  \tint ret = 0, original_len = path->len, len, kept_down = 0;\n>>  \tint only_empty = (flag & REMOVE_DIR_EMPTY_ONLY);\n>> @@ -3261,7 +3261,10 @@ static int remove_dir_recurse(struct strbuf *path, int flag, int *kept_up)\n>>  \t}\n>>\n>>  \tflag &= ~REMOVE_DIR_KEEP_TOPLEVEL;\n>> -\tdir = opendir(path->buf);\n>> +\n>> +\tif ((flag & REMOVE_DIR_SIGNAL) == 0)\n>> +\t\tdir = opendir(path->buf);\n>> +\n>>  \tif (!dir) {\n>>  \t\tif (errno == ENOENT)\n>>  \t\t\treturn keep_toplevel ? -1 : 0;\n>\n> We skip calling opendir() entirely, so \"dir\" will still be NULL. But we\n> immediately start looking at errno, which will have some undefined value\n> (based on some previous syscall).\n>\n> If we set \"errno\" to \"EACCES\" in this case, then we'd at least hit the\n> rmdir() below:\n>\n>          if (!dir) {\n>                   if (errno == ENOENT)\n>                           return keep_toplevel ? -1 : 0;\n>                   else if (errno == EACCES && !keep_toplevel)\n>                           /*\n>                            * An empty dir could be removable even if it\n>                            * is unreadable:\n>                            */\n>                           return rmdir(path->buf);\n>                   else\n>                           return -1;\n>           }\n>\n> but we know it won't really do anything for our proposed caller, since\n> it will have files inside the directory that need to be removed before\n> rmdir() can work.\n\nyeah, I suppose the only case it would help is if the directory is already\nempty.\n\n>\n> Moreover, if you were to combine REMOVE_DIR_SIGNAL with\n> REMOVE_DIR_KEEP_NESTED_GIT, I suspect that the call to\n> resolve_gitlink_ref() would end up with similar deadlocks. Obviously\n> nobody is proposing to do that, but it is a pitfall in the API.\n>\n> So all of that makes me think we should not add a new flag here, but\n> instead just avoid calling the function entirely from\n> tmp_objdir_destroy_1().\n>\n> But then we can observe that tmp_objdir_destroy_1() is basically doing\n> nothing if on_signal is set. So there is really no point in setting up\n> the signal handler at all. We should just set up the atexit() handler.\n> I.e., something like:\n>\n> diff --git a/tmp-objdir.c b/tmp-objdir.c\n> index a8be92bca1..10549e95db 100644\n> --- a/tmp-objdir.c\n> +++ b/tmp-objdir.c\n> @@ -169,7 +169,6 @@ struct tmp_objdir *tmp_objdir_create(const char *prefix)\n>  \tthe_tmp_objdir = t;\n>  \tif (!installed_handlers) {\n>  \t\tatexit(remove_tmp_objdir);\n> -\t\tsigchain_push_common(remove_tmp_objdir_on_signal);\n>  \t\tinstalled_handlers++;\n>  \t}\n\nThis makes sense and is a clean solution. I guess the only case where we would\nbenefit in calling into remove_tmp_objdir_on_signal() is if the temp dir exists\nbut is empty. I'm not sure how often this would happen to make it worth it?\nProbably not...\n\n>\n>\n> with the commit message explaining that we can't do the cleanup in a\n> portable and signal-safe way, so we just punt on the whole concept.\n>\n> There's also some minor cleanup we could do elsewhere to drop the\n> \"on_signal\" argument (which can come as part of the same patch, or on\n> top).\n>\n> -Peff\n"},{"id":"463743","messageId":"xmqqedvw7od9.fsf@gitster.g","threadId":"58517","inReplyTo":"YzLiI1HZeBszsIJq@coredump.intra.peff.net","subject":"Re: [PATCH] tmp-objdir: do not opendir() when handling a signal","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-09-27T16:50:42Z","receivedAt":"2022-09-27T16:52:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> So all of that makes me think we should not add a new flag here, but\n> instead just avoid calling the function entirely from\n> tmp_objdir_destroy_1().\n\nThanks.  I missed that undefined access to errno that breaks the\nintention of the patch.\n\n> But then we can observe that tmp_objdir_destroy_1() is basically doing\n> nothing if on_signal is set. So there is really no point in setting up\n> the signal handler at all. We should just set up the atexit() handler.\n> I.e., something like:\n>\n> diff --git a/tmp-objdir.c b/tmp-objdir.c\n> index a8be92bca1..10549e95db 100644\n> --- a/tmp-objdir.c\n> +++ b/tmp-objdir.c\n> @@ -169,7 +169,6 @@ struct tmp_objdir *tmp_objdir_create(const char *prefix)\n>  \tthe_tmp_objdir = t;\n>  \tif (!installed_handlers) {\n>  \t\tatexit(remove_tmp_objdir);\n> -\t\tsigchain_push_common(remove_tmp_objdir_on_signal);\n>  \t\tinstalled_handlers++;\n>  \t}\n>  \n>\n> with the commit message explaining that we can't do the cleanup in a\n> portable and signal-safe way, so we just punt on the whole concept.\n>\n> There's also some minor cleanup we could do elsewhere to drop the\n> \"on_signal\" argument (which can come as part of the same patch, or on\n> top).\n\n;-)  I like the simplification.\n"},{"id":"463751","messageId":"YzNI8tEpREMLNUpg@coredump.intra.peff.net","threadId":"58517","inReplyTo":"1B4D6262-532A-401E-B1A2-709BEECA4B67@gmail.com","subject":"Re: [PATCH] tmp-objdir: do not opendir() when handling a signal","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-09-27T19:03:14Z","receivedAt":"2022-09-27T19:03:20Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Sep 27, 2022 at 09:50:43AM -0400, John Cai wrote:\n\n> > diff --git a/tmp-objdir.c b/tmp-objdir.c\n> > index a8be92bca1..10549e95db 100644\n> > --- a/tmp-objdir.c\n> > +++ b/tmp-objdir.c\n> > @@ -169,7 +169,6 @@ struct tmp_objdir *tmp_objdir_create(const char *prefix)\n> >  \tthe_tmp_objdir = t;\n> >  \tif (!installed_handlers) {\n> >  \t\tatexit(remove_tmp_objdir);\n> > -\t\tsigchain_push_common(remove_tmp_objdir_on_signal);\n> >  \t\tinstalled_handlers++;\n> >  \t}\n> \n> This makes sense and is a clean solution. I guess the only case where we would\n> benefit in calling into remove_tmp_objdir_on_signal() is if the temp dir exists\n> but is empty. I'm not sure how often this would happen to make it worth it?\n> Probably not...\n\nIt's more or less never, as the first thing we do after creating it is\ncall tmp_objdir_setup(), which creates the pack subdirectory. We _could_\ntry removing them both manually, like:\n\ndiff --git a/tmp-objdir.c b/tmp-objdir.c\nindex adf6033549..509eb58363 100644\n--- a/tmp-objdir.c\n+++ b/tmp-objdir.c\n@@ -49,7 +49,17 @@ static int tmp_objdir_destroy_1(struct tmp_objdir *t, int on_signal)\n \t * have pre-grown t->path sufficiently so that this\n \t * doesn't happen in practice.\n \t */\n-\terr = remove_dir_recursively(&t->path, 0);\n+\tif (!on_signal)\n+\t\terr = remove_dir_recursively(&t->path, 0);\n+\telse {\n+\t\tsize_t orig_len = t->path.len;\n+\n+\t\tstrbuf_addstr(&t->path, \"/pack\");\n+\t\trmdir(t->path.buf);\n+\t\tstrbuf_setlen(&t->path, orig_len);\n+\n+\t\trmdir(t->path.buf);\n+\t}\n \n \t/*\n \t * When we are cleaning up due to a signal, we won't bother\n\nbut even that would rarely do anything useful. As soon as the child\nindex-pack receives even a single byte, we'll have an actual pack\ntmpfile with an unknown name, which will cause rmdir() to fail (ditto\nfor unpack-objects, though it would probably write as soon as we've\nreceived a whole single object).\n\nIf we can't enumerate the list of objects via readdir() or similar, I\nthink there's really not much we can do.\n\n-Peff\n"},{"id":"463752","messageId":"pull.1348.v2.git.git.1664306341425.gitgitgadget@gmail.com","threadId":"58517","inReplyTo":"pull.1348.git.git.1664236383785.gitgitgadget@gmail.com","subject":"[PATCH v2] tmp-objdir: skip clean up when handling a signal","fromName":"John Cai via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-09-27T19:19:01Z","receivedAt":"2022-09-27T19:19:07Z","isPatch":true,"sender":{"key":"johncai86@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2354211?v=4"},"body":"From: John Cai <johncai86@gmail.com>\n\nIn the tmp-objdir api, tmp_objdir_create will create a temporary\ndirectory but also register signal handlers responsible for removing\nthe directory's contents and the directory itself. However, the\nfunction responsible for recursively removing the contents and\ndirectory, remove_dir_recurse() calls opendir(3) and closedir(3).\nThis can be problematic because these functions allocate and free\nmemory, which are not async-signal-safe functions. This can lead to\ndeadlocks.\n\nOne place we call tmp_objdir_create() is in git-receive-pack, where\nwe create a temporary quarantine directory \"incoming\". Incoming\nobjects will be written to this directory before they get moved to\nthe object directory.\n\nWe have observed this code leading to a deadlock:\n\n\tThread 1 (Thread 0x7f621ba0b200 (LWP 326305)):\n\t#0  __lll_lock_wait_private (futex=futex@entry=0x7f621bbf8b80\n\t\t<main_arena>) at ./lowlevellock.c:35\n\t#1  0x00007f621baa635b in __GI___libc_malloc\n\t\t(bytes=bytes@entry=32816) at malloc.c:3064\n\t#2  0x00007f621bae9f49 in __alloc_dir (statp=0x7fff2ea7ed60,\n\t\tflags=0, close_fd=true, fd=5)\n\t\tat ../sysdeps/posix/opendir.c:118\n\t#3  opendir_tail (fd=5) at ../sysdeps/posix/opendir.c:69\n\t#4  __opendir (name=<optimized out>)\n\t\tat ../sysdeps/posix/opendir.c:92\n\t#5  0x0000557c19c77de1 in remove_dir_recurse ()\n\tgit#6  0x0000557c19d81a4f in remove_tmp_objdir_on_signal ()\n\t#7  <signal handler called>\n\tgit#8  _int_malloc (av=av@entry=0x7f621bbf8b80 <main_arena>,\n\t\tbytes=bytes@entry=7160) at malloc.c:4116\n\tgit#9  0x00007f621baa62c9 in __GI___libc_malloc (bytes=7160)\n\t\tat malloc.c:3066\n\tgit#10 0x00007f621bd1e987 in inflateInit2_ ()\n\t\tfrom /opt/gitlab/embedded/lib/libz.so.1\n\tgit#11 0x0000557c19dbe5f4 in git_inflate_init ()\n\tgit#12 0x0000557c19cee02a in unpack_compressed_entry ()\n\tgit#13 0x0000557c19cf08cb in unpack_entry ()\n\tgit#14 0x0000557c19cf0f32 in packed_object_info ()\n\tgit#15 0x0000557c19cd68cd in do_oid_object_info_extended ()\n\tgit#16 0x0000557c19cd6e2b in read_object_file_extended ()\n\tgit#17 0x0000557c19cdec2f in parse_object ()\n\tgit#18 0x0000557c19c34977 in lookup_commit_reference_gently ()\n\tgit#19 0x0000557c19d69309 in mark_uninteresting ()\n\tgit#20 0x0000557c19d2d180 in do_for_each_repo_ref_iterator ()\n\tgit#21 0x0000557c19d21678 in for_each_ref ()\n\tgit#22 0x0000557c19d6a94f in assign_shallow_commits_to_refs ()\n\tgit#23 0x0000557c19bc02b2 in cmd_receive_pack ()\n\tgit#24 0x0000557c19b29fdd in handle_builtin ()\n\tgit#25 0x0000557c19b2a526 in cmd_main ()\n\tgit#26 0x0000557c19b28ea2 in main ()\n\nSince we can't do the cleanup in a portable and signal-safe way, skip\nthe cleanup when we're handling a signal.\n\nThis means that when signal handling, the temporary directory may not\nget cleaned up properly. This is mitigated by b3cecf49ea (tmp-objdir: new\nAPI for creating temporary writable databases, 2021-12-06) which changed\nthe default name and allows gc to clean up these temporary directories.\n\nSigned-off-by: John Cai <johncai86@gmail.com>\n---\n    tmp-objdir: do not closedir() when handling a signal\n    \n    We have recently observed a Git process hanging around for weeks. A\n    backtrace revealed that a git-receive-pack(1) process was deadlocked\n    when trying to remove the quarantine directory \"incoming.\" It turns out\n    that the tmp_objdir API calls opendir(3) and closedir(3) to observe a\n    directory's contents in order to remove all the contents before removing\n    the directory itself. These functions are not async signal save as they\n    allocate and free memory.\n    \n    The fix is to avoid calling these functions when handling a signal in\n    order to avoid a deadlock. The implication of such a fix however, is\n    that temporary object directories may not get cleaned up properly when a\n    signal is being handled. The tradeoff this fix is making is to prevent\n    deadlocks at the cost of temporary object directory cleanup.\n    \n    This is similar to 58d4d7f1c5 (2022-01-07 fetch: fix deadlock when\n    cleaning up lockfiles in async signals) The difference being in this\n    case, the cleanup of the files will not happen when handling a signal.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1348%2Fjohn-cai%2Fjc%2Ffix-tmp-objdir-signal-handling-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1348/john-cai/jc/fix-tmp-objdir-signal-handling-v2\nPull-Request: https://github.com/git/git/pull/1348\n\nRange-diff vs v1:\n\n 1:  78ee805c5aa < -:  ----------- tmp-objdir: do not opendir() when handling a signal\n -:  ----------- > 1:  ae58df8c938 tmp-objdir: skip clean up when handling a signal\n\n\n tmp-objdir.c | 22 ++++------------------\n 1 file changed, 4 insertions(+), 18 deletions(-)\n\ndiff --git a/tmp-objdir.c b/tmp-objdir.c\nindex adf6033549e..5d5f15f6d76 100644\n--- a/tmp-objdir.c\n+++ b/tmp-objdir.c\n@@ -31,7 +31,7 @@ static void tmp_objdir_free(struct tmp_objdir *t)\n \tfree(t);\n }\n \n-static int tmp_objdir_destroy_1(struct tmp_objdir *t, int on_signal)\n+static int tmp_objdir_destroy_1(struct tmp_objdir *t)\n {\n \tint err;\n \n@@ -41,7 +41,7 @@ static int tmp_objdir_destroy_1(struct tmp_objdir *t, int on_signal)\n \tif (t == the_tmp_objdir)\n \t\tthe_tmp_objdir = NULL;\n \n-\tif (!on_signal && t->prev_odb)\n+\tif (t->prev_odb)\n \t\trestore_primary_odb(t->prev_odb, t->path.buf);\n \n \t/*\n@@ -51,20 +51,14 @@ static int tmp_objdir_destroy_1(struct tmp_objdir *t, int on_signal)\n \t */\n \terr = remove_dir_recursively(&t->path, 0);\n \n-\t/*\n-\t * When we are cleaning up due to a signal, we won't bother\n-\t * freeing memory; it may cause a deadlock if the signal\n-\t * arrived while libc's allocator lock is held.\n-\t */\n-\tif (!on_signal)\n-\t\ttmp_objdir_free(t);\n+\ttmp_objdir_free(t);\n \n \treturn err;\n }\n \n int tmp_objdir_destroy(struct tmp_objdir *t)\n {\n-\treturn tmp_objdir_destroy_1(t, 0);\n+\treturn tmp_objdir_destroy_1(t);\n }\n \n static void remove_tmp_objdir(void)\n@@ -72,13 +66,6 @@ static void remove_tmp_objdir(void)\n \ttmp_objdir_destroy(the_tmp_objdir);\n }\n \n-static void remove_tmp_objdir_on_signal(int signo)\n-{\n-\ttmp_objdir_destroy_1(the_tmp_objdir, 1);\n-\tsigchain_pop(signo);\n-\traise(signo);\n-}\n-\n void tmp_objdir_discard_objects(struct tmp_objdir *t)\n {\n \tremove_dir_recursively(&t->path, REMOVE_DIR_KEEP_TOPLEVEL);\n@@ -169,7 +156,6 @@ struct tmp_objdir *tmp_objdir_create(const char *prefix)\n \tthe_tmp_objdir = t;\n \tif (!installed_handlers) {\n \t\tatexit(remove_tmp_objdir);\n-\t\tsigchain_push_common(remove_tmp_objdir_on_signal);\n \t\tinstalled_handlers++;\n \t}\n \n\nbase-commit: 4fd6c5e44459e6444c2cd93383660134c95aabd1\n-- \ngitgitgadget\n"},{"id":"463757","messageId":"YzNRMbaM40i/6tPa@coredump.intra.peff.net","threadId":"58517","inReplyTo":"pull.1348.v2.git.git.1664306341425.gitgitgadget@gmail.com","subject":"Re: [PATCH v2] tmp-objdir: skip clean up when handling a signal","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-09-27T19:38:25Z","receivedAt":"2022-09-27T19:38:32Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Sep 27, 2022 at 07:19:01PM +0000, John Cai via GitGitGadget wrote:\n\n> Since we can't do the cleanup in a portable and signal-safe way, skip\n> the cleanup when we're handling a signal.\n\nThanks, this looks fine to me, though I think there are a few extra\ncleanup opportunities that could be squashed in:\n\ndiff --git a/tmp-objdir.c b/tmp-objdir.c\nindex 5d5f15f6d7..2fb0ec8317 100644\n--- a/tmp-objdir.c\n+++ b/tmp-objdir.c\n@@ -18,7 +18,7 @@ struct tmp_objdir {\n \n /*\n  * Allow only one tmp_objdir at a time in a running process, which simplifies\n- * our signal/atexit cleanup routines.  It's doubtful callers will ever need\n+ * our atexit cleanup routine.  It's doubtful callers will ever need\n  * more than one, and we can expand later if so.  You can have many such\n  * tmp_objdirs simultaneously in many processes, of course.\n  */\n@@ -31,7 +31,7 @@ static void tmp_objdir_free(struct tmp_objdir *t)\n \tfree(t);\n }\n \n-static int tmp_objdir_destroy_1(struct tmp_objdir *t)\n+static int tmp_objdir_destroy(struct tmp_objdir *t)\n {\n \tint err;\n \n@@ -44,23 +44,13 @@ static int tmp_objdir_destroy_1(struct tmp_objdir *t)\n \tif (t->prev_odb)\n \t\trestore_primary_odb(t->prev_odb, t->path.buf);\n \n-\t/*\n-\t * This may use malloc via strbuf_grow(), but we should\n-\t * have pre-grown t->path sufficiently so that this\n-\t * doesn't happen in practice.\n-\t */\n \terr = remove_dir_recursively(&t->path, 0);\n \n \ttmp_objdir_free(t);\n \n \treturn err;\n }\n \n-int tmp_objdir_destroy(struct tmp_objdir *t)\n-{\n-\treturn tmp_objdir_destroy_1(t);\n-}\n-\n static void remove_tmp_objdir(void)\n {\n \ttmp_objdir_destroy(the_tmp_objdir);\n@@ -139,14 +129,6 @@ struct tmp_objdir *tmp_objdir_create(const char *prefix)\n \t */\n \tstrbuf_addf(&t->path, \"%s/tmp_objdir-%s-XXXXXX\", get_object_directory(), prefix);\n \n-\t/*\n-\t * Grow the strbuf beyond any filename we expect to be placed in it.\n-\t * If tmp_objdir_destroy() is called by a signal handler, then\n-\t * we should be able to use the strbuf to remove files without\n-\t * having to call malloc.\n-\t */\n-\tstrbuf_grow(&t->path, 1024);\n-\n \tif (!mkdtemp(t->path.buf)) {\n \t\t/* free, not destroy, as we never touched the filesystem */\n \t\ttmp_objdir_free(t);\n"},{"id":"463758","messageId":"YzNWbGomhzI/fFMx@coredump.intra.peff.net","threadId":"58517","inReplyTo":"YzNRMbaM40i/6tPa@coredump.intra.peff.net","subject":"Re: [PATCH v2] tmp-objdir: skip clean up when handling a signal","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-09-27T20:00:44Z","receivedAt":"2022-09-27T20:01:59Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Sep 27, 2022 at 03:38:26PM -0400, Jeff King wrote:\n\n> > Since we can't do the cleanup in a portable and signal-safe way, skip\n> > the cleanup when we're handling a signal.\n> \n> Thanks, this looks fine to me, though I think there are a few extra\n> cleanup opportunities that could be squashed in:\n\n> -static int tmp_objdir_destroy_1(struct tmp_objdir *t)\n> +static int tmp_objdir_destroy(struct tmp_objdir *t)\n\nWhoops, you'd obviously want to drop the \"static\" here, as well.\n\n-Peff\n"},{"id":"463845","messageId":"pull.1348.v3.git.git.1664376956780.gitgitgadget@gmail.com","threadId":"58517","inReplyTo":"pull.1348.v2.git.git.1664306341425.gitgitgadget@gmail.com","subject":"[PATCH v3] tmp-objdir: skip clean up when handling a signal","fromName":"John Cai via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-09-28T14:55:56Z","receivedAt":"2022-09-28T14:56:05Z","isPatch":true,"sender":{"key":"johncai86@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2354211?v=4"},"body":"From: John Cai <johncai86@gmail.com>\n\nIn the tmp-objdir api, tmp_objdir_create will create a temporary\ndirectory but also register signal handlers responsible for removing\nthe directory's contents and the directory itself. However, the\nfunction responsible for recursively removing the contents and\ndirectory, remove_dir_recurse() calls opendir(3) and closedir(3).\nThis can be problematic because these functions allocate and free\nmemory, which are not async-signal-safe functions. This can lead to\ndeadlocks.\n\nOne place we call tmp_objdir_create() is in git-receive-pack, where\nwe create a temporary quarantine directory \"incoming\". Incoming\nobjects will be written to this directory before they get moved to\nthe object directory.\n\nWe have observed this code leading to a deadlock:\n\n\tThread 1 (Thread 0x7f621ba0b200 (LWP 326305)):\n\t#0  __lll_lock_wait_private (futex=futex@entry=0x7f621bbf8b80\n\t\t<main_arena>) at ./lowlevellock.c:35\n\t#1  0x00007f621baa635b in __GI___libc_malloc\n\t\t(bytes=bytes@entry=32816) at malloc.c:3064\n\t#2  0x00007f621bae9f49 in __alloc_dir (statp=0x7fff2ea7ed60,\n\t\tflags=0, close_fd=true, fd=5)\n\t\tat ../sysdeps/posix/opendir.c:118\n\t#3  opendir_tail (fd=5) at ../sysdeps/posix/opendir.c:69\n\t#4  __opendir (name=<optimized out>)\n\t\tat ../sysdeps/posix/opendir.c:92\n\t#5  0x0000557c19c77de1 in remove_dir_recurse ()\n\tgit#6  0x0000557c19d81a4f in remove_tmp_objdir_on_signal ()\n\t#7  <signal handler called>\n\tgit#8  _int_malloc (av=av@entry=0x7f621bbf8b80 <main_arena>,\n\t\tbytes=bytes@entry=7160) at malloc.c:4116\n\tgit#9  0x00007f621baa62c9 in __GI___libc_malloc (bytes=7160)\n\t\tat malloc.c:3066\n\tgit#10 0x00007f621bd1e987 in inflateInit2_ ()\n\t\tfrom /opt/gitlab/embedded/lib/libz.so.1\n\tgit#11 0x0000557c19dbe5f4 in git_inflate_init ()\n\tgit#12 0x0000557c19cee02a in unpack_compressed_entry ()\n\tgit#13 0x0000557c19cf08cb in unpack_entry ()\n\tgit#14 0x0000557c19cf0f32 in packed_object_info ()\n\tgit#15 0x0000557c19cd68cd in do_oid_object_info_extended ()\n\tgit#16 0x0000557c19cd6e2b in read_object_file_extended ()\n\tgit#17 0x0000557c19cdec2f in parse_object ()\n\tgit#18 0x0000557c19c34977 in lookup_commit_reference_gently ()\n\tgit#19 0x0000557c19d69309 in mark_uninteresting ()\n\tgit#20 0x0000557c19d2d180 in do_for_each_repo_ref_iterator ()\n\tgit#21 0x0000557c19d21678 in for_each_ref ()\n\tgit#22 0x0000557c19d6a94f in assign_shallow_commits_to_refs ()\n\tgit#23 0x0000557c19bc02b2 in cmd_receive_pack ()\n\tgit#24 0x0000557c19b29fdd in handle_builtin ()\n\tgit#25 0x0000557c19b2a526 in cmd_main ()\n\tgit#26 0x0000557c19b28ea2 in main ()\n\nSince we can't do the cleanup in a portable and signal-safe way, skip\nthe cleanup when we're handling a signal.\n\nThis means that when signal handling, the temporary directory may not\nget cleaned up properly. This is mitigated by b3cecf49ea (tmp-objdir: new\nAPI for creating temporary writable databases, 2021-12-06) which changed\nthe default name and allows gc to clean up these temporary directories.\n\nSigned-off-by: John Cai <johncai86@gmail.com>\n---\n    tmp-objdir: do not closedir() when handling a signal\n    \n    We have recently observed a Git process hanging around for weeks. A\n    backtrace revealed that a git-receive-pack(1) process was deadlocked\n    when trying to remove the quarantine directory \"incoming.\" It turns out\n    that the tmp_objdir API calls opendir(3) and closedir(3) to observe a\n    directory's contents in order to remove all the contents before removing\n    the directory itself. These functions are not async signal save as they\n    allocate and free memory.\n    \n    The fix is to avoid calling these functions when handling a signal in\n    order to avoid a deadlock. The implication of such a fix however, is\n    that temporary object directories may not get cleaned up properly when a\n    signal is being handled. The tradeoff this fix is making is to prevent\n    deadlocks at the cost of temporary object directory cleanup.\n    \n    This is similar to 58d4d7f1c5 (2022-01-07 fetch: fix deadlock when\n    cleaning up lockfiles in async signals) The difference being in this\n    case, the cleanup of the files will not happen when handling a signal.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1348%2Fjohn-cai%2Fjc%2Ffix-tmp-objdir-signal-handling-v3\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1348/john-cai/jc/fix-tmp-objdir-signal-handling-v3\nPull-Request: https://github.com/git/git/pull/1348\n\nRange-diff vs v2:\n\n 1:  ae58df8c938 ! 1:  d96c44e309f tmp-objdir: skip clean up when handling a signal\n     @@ Commit message\n          Signed-off-by: John Cai <johncai86@gmail.com>\n      \n       ## tmp-objdir.c ##\n     +@@ tmp-objdir.c: struct tmp_objdir {\n     + \n     + /*\n     +  * Allow only one tmp_objdir at a time in a running process, which simplifies\n     +- * our signal/atexit cleanup routines.  It's doubtful callers will ever need\n     ++ * our atexit cleanup routines.  It's doubtful callers will ever need\n     +  * more than one, and we can expand later if so.  You can have many such\n     +  * tmp_objdirs simultaneously in many processes, of course.\n     +  */\n      @@ tmp-objdir.c: static void tmp_objdir_free(struct tmp_objdir *t)\n       \tfree(t);\n       }\n       \n      -static int tmp_objdir_destroy_1(struct tmp_objdir *t, int on_signal)\n     -+static int tmp_objdir_destroy_1(struct tmp_objdir *t)\n     ++int tmp_objdir_destroy(struct tmp_objdir *t)\n       {\n       \tint err;\n       \n     @@ tmp-objdir.c: static int tmp_objdir_destroy_1(struct tmp_objdir *t, int on_signa\n      +\tif (t->prev_odb)\n       \t\trestore_primary_odb(t->prev_odb, t->path.buf);\n       \n     - \t/*\n     -@@ tmp-objdir.c: static int tmp_objdir_destroy_1(struct tmp_objdir *t, int on_signal)\n     - \t */\n     +-\t/*\n     +-\t * This may use malloc via strbuf_grow(), but we should\n     +-\t * have pre-grown t->path sufficiently so that this\n     +-\t * doesn't happen in practice.\n     +-\t */\n       \terr = remove_dir_recursively(&t->path, 0);\n       \n      -\t/*\n     @@ tmp-objdir.c: static int tmp_objdir_destroy_1(struct tmp_objdir *t, int on_signa\n       \treturn err;\n       }\n       \n     - int tmp_objdir_destroy(struct tmp_objdir *t)\n     - {\n     +-int tmp_objdir_destroy(struct tmp_objdir *t)\n     +-{\n      -\treturn tmp_objdir_destroy_1(t, 0);\n     -+\treturn tmp_objdir_destroy_1(t);\n     - }\n     - \n     +-}\n     +-\n       static void remove_tmp_objdir(void)\n     -@@ tmp-objdir.c: static void remove_tmp_objdir(void)\n     + {\n       \ttmp_objdir_destroy(the_tmp_objdir);\n       }\n       \n     @@ tmp-objdir.c: static void remove_tmp_objdir(void)\n       void tmp_objdir_discard_objects(struct tmp_objdir *t)\n       {\n       \tremove_dir_recursively(&t->path, REMOVE_DIR_KEEP_TOPLEVEL);\n     +@@ tmp-objdir.c: struct tmp_objdir *tmp_objdir_create(const char *prefix)\n     + \t */\n     + \tstrbuf_addf(&t->path, \"%s/tmp_objdir-%s-XXXXXX\", get_object_directory(), prefix);\n     + \n     +-\t/*\n     +-\t * Grow the strbuf beyond any filename we expect to be placed in it.\n     +-\t * If tmp_objdir_destroy() is called by a signal handler, then\n     +-\t * we should be able to use the strbuf to remove files without\n     +-\t * having to call malloc.\n     +-\t */\n     +-\tstrbuf_grow(&t->path, 1024);\n     +-\n     + \tif (!mkdtemp(t->path.buf)) {\n     + \t\t/* free, not destroy, as we never touched the filesystem */\n     + \t\ttmp_objdir_free(t);\n      @@ tmp-objdir.c: struct tmp_objdir *tmp_objdir_create(const char *prefix)\n       \tthe_tmp_objdir = t;\n       \tif (!installed_handlers) {\n\n\n tmp-objdir.c | 40 ++++------------------------------------\n 1 file changed, 4 insertions(+), 36 deletions(-)\n\ndiff --git a/tmp-objdir.c b/tmp-objdir.c\nindex adf6033549e..2a2012eb6d0 100644\n--- a/tmp-objdir.c\n+++ b/tmp-objdir.c\n@@ -18,7 +18,7 @@ struct tmp_objdir {\n \n /*\n  * Allow only one tmp_objdir at a time in a running process, which simplifies\n- * our signal/atexit cleanup routines.  It's doubtful callers will ever need\n+ * our atexit cleanup routines.  It's doubtful callers will ever need\n  * more than one, and we can expand later if so.  You can have many such\n  * tmp_objdirs simultaneously in many processes, of course.\n  */\n@@ -31,7 +31,7 @@ static void tmp_objdir_free(struct tmp_objdir *t)\n \tfree(t);\n }\n \n-static int tmp_objdir_destroy_1(struct tmp_objdir *t, int on_signal)\n+int tmp_objdir_destroy(struct tmp_objdir *t)\n {\n \tint err;\n \n@@ -41,44 +41,21 @@ static int tmp_objdir_destroy_1(struct tmp_objdir *t, int on_signal)\n \tif (t == the_tmp_objdir)\n \t\tthe_tmp_objdir = NULL;\n \n-\tif (!on_signal && t->prev_odb)\n+\tif (t->prev_odb)\n \t\trestore_primary_odb(t->prev_odb, t->path.buf);\n \n-\t/*\n-\t * This may use malloc via strbuf_grow(), but we should\n-\t * have pre-grown t->path sufficiently so that this\n-\t * doesn't happen in practice.\n-\t */\n \terr = remove_dir_recursively(&t->path, 0);\n \n-\t/*\n-\t * When we are cleaning up due to a signal, we won't bother\n-\t * freeing memory; it may cause a deadlock if the signal\n-\t * arrived while libc's allocator lock is held.\n-\t */\n-\tif (!on_signal)\n-\t\ttmp_objdir_free(t);\n+\ttmp_objdir_free(t);\n \n \treturn err;\n }\n \n-int tmp_objdir_destroy(struct tmp_objdir *t)\n-{\n-\treturn tmp_objdir_destroy_1(t, 0);\n-}\n-\n static void remove_tmp_objdir(void)\n {\n \ttmp_objdir_destroy(the_tmp_objdir);\n }\n \n-static void remove_tmp_objdir_on_signal(int signo)\n-{\n-\ttmp_objdir_destroy_1(the_tmp_objdir, 1);\n-\tsigchain_pop(signo);\n-\traise(signo);\n-}\n-\n void tmp_objdir_discard_objects(struct tmp_objdir *t)\n {\n \tremove_dir_recursively(&t->path, REMOVE_DIR_KEEP_TOPLEVEL);\n@@ -152,14 +129,6 @@ struct tmp_objdir *tmp_objdir_create(const char *prefix)\n \t */\n \tstrbuf_addf(&t->path, \"%s/tmp_objdir-%s-XXXXXX\", get_object_directory(), prefix);\n \n-\t/*\n-\t * Grow the strbuf beyond any filename we expect to be placed in it.\n-\t * If tmp_objdir_destroy() is called by a signal handler, then\n-\t * we should be able to use the strbuf to remove files without\n-\t * having to call malloc.\n-\t */\n-\tstrbuf_grow(&t->path, 1024);\n-\n \tif (!mkdtemp(t->path.buf)) {\n \t\t/* free, not destroy, as we never touched the filesystem */\n \t\ttmp_objdir_free(t);\n@@ -169,7 +138,6 @@ struct tmp_objdir *tmp_objdir_create(const char *prefix)\n \tthe_tmp_objdir = t;\n \tif (!installed_handlers) {\n \t\tatexit(remove_tmp_objdir);\n-\t\tsigchain_push_common(remove_tmp_objdir_on_signal);\n \t\tinstalled_handlers++;\n \t}\n \n\nbase-commit: 4fd6c5e44459e6444c2cd93383660134c95aabd1\n-- \ngitgitgadget\n"},{"id":"463850","messageId":"220928.86leq3wlnf.gmgdl@evledraar.gmail.com","threadId":"58517","inReplyTo":"pull.1348.v3.git.git.1664376956780.gitgitgadget@gmail.com","subject":"Re: [PATCH v3] tmp-objdir: skip clean up when handling a signal","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-09-28T15:38:21Z","receivedAt":"2022-09-28T15:42:36Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Sep 28 2022, John Cai via GitGitGadget wrote:\n\n> From: John Cai <johncai86@gmail.com>\n\nThis looks good! Just a clarifying question/comment:\n\n> Since we can't do the cleanup in a portable and signal-safe way, skip\n> the cleanup when we're handling a signal.\n>\n> This means that when signal handling, the temporary directory may not\n> get cleaned up properly. This is mitigated by b3cecf49ea (tmp-objdir: new\n> API for creating temporary writable databases, 2021-12-06) which changed\n> the default name and allows gc to clean up these temporary directories.\n\nI think this still doesn't cover the common case of the \"atexit\" handler\nsaving the day, as Jeff King pointed out in v1:\n\n\thttps://lore.kernel.org/git/YzLiI1HZeBszsIJq@coredump.intra.peff.net/\n\nI think it's fine to have this proceed as-is (although if you're doing a\nv4 anyway, maybe we want to update tho commit message). I.e. is it\ncorrect that we'll now only skip this if we get a signal, *and* it's a\nfatal signal, or we otherwise die/exit/abort/whatever before we can get\nto our atexit() handler?\n"},{"id":"464013","messageId":"pull.1348.v4.git.git.1664570831583.gitgitgadget@gmail.com","threadId":"58517","inReplyTo":"pull.1348.v3.git.git.1664376956780.gitgitgadget@gmail.com","subject":"[PATCH v4] tmp-objdir: skip clean up when handling a signal","fromName":"John Cai via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-09-30T20:47:11Z","receivedAt":"2022-09-30T20:47:21Z","isPatch":true,"sender":{"key":"johncai86@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2354211?v=4"},"body":"From: John Cai <johncai86@gmail.com>\n\nIn the tmp-objdir api, tmp_objdir_create will create a temporary\ndirectory but also register signal handlers responsible for removing\nthe directory's contents and the directory itself. However, the\nfunction responsible for recursively removing the contents and\ndirectory, remove_dir_recurse() calls opendir(3) and closedir(3).\nThis can be problematic because these functions allocate and free\nmemory, which are not async-signal-safe functions. This can lead to\ndeadlocks.\n\nOne place we call tmp_objdir_create() is in git-receive-pack, where\nwe create a temporary quarantine directory \"incoming\". Incoming\nobjects will be written to this directory before they get moved to\nthe object directory.\n\nWe have observed this code leading to a deadlock:\n\n\tThread 1 (Thread 0x7f621ba0b200 (LWP 326305)):\n\t#0  __lll_lock_wait_private (futex=futex@entry=0x7f621bbf8b80\n\t\t<main_arena>) at ./lowlevellock.c:35\n\t#1  0x00007f621baa635b in __GI___libc_malloc\n\t\t(bytes=bytes@entry=32816) at malloc.c:3064\n\t#2  0x00007f621bae9f49 in __alloc_dir (statp=0x7fff2ea7ed60,\n\t\tflags=0, close_fd=true, fd=5)\n\t\tat ../sysdeps/posix/opendir.c:118\n\t#3  opendir_tail (fd=5) at ../sysdeps/posix/opendir.c:69\n\t#4  __opendir (name=<optimized out>)\n\t\tat ../sysdeps/posix/opendir.c:92\n\t#5  0x0000557c19c77de1 in remove_dir_recurse ()\n\tgit#6  0x0000557c19d81a4f in remove_tmp_objdir_on_signal ()\n\t#7  <signal handler called>\n\tgit#8  _int_malloc (av=av@entry=0x7f621bbf8b80 <main_arena>,\n\t\tbytes=bytes@entry=7160) at malloc.c:4116\n\tgit#9  0x00007f621baa62c9 in __GI___libc_malloc (bytes=7160)\n\t\tat malloc.c:3066\n\tgit#10 0x00007f621bd1e987 in inflateInit2_ ()\n\t\tfrom /opt/gitlab/embedded/lib/libz.so.1\n\tgit#11 0x0000557c19dbe5f4 in git_inflate_init ()\n\tgit#12 0x0000557c19cee02a in unpack_compressed_entry ()\n\tgit#13 0x0000557c19cf08cb in unpack_entry ()\n\tgit#14 0x0000557c19cf0f32 in packed_object_info ()\n\tgit#15 0x0000557c19cd68cd in do_oid_object_info_extended ()\n\tgit#16 0x0000557c19cd6e2b in read_object_file_extended ()\n\tgit#17 0x0000557c19cdec2f in parse_object ()\n\tgit#18 0x0000557c19c34977 in lookup_commit_reference_gently ()\n\tgit#19 0x0000557c19d69309 in mark_uninteresting ()\n\tgit#20 0x0000557c19d2d180 in do_for_each_repo_ref_iterator ()\n\tgit#21 0x0000557c19d21678 in for_each_ref ()\n\tgit#22 0x0000557c19d6a94f in assign_shallow_commits_to_refs ()\n\tgit#23 0x0000557c19bc02b2 in cmd_receive_pack ()\n\tgit#24 0x0000557c19b29fdd in handle_builtin ()\n\tgit#25 0x0000557c19b2a526 in cmd_main ()\n\tgit#26 0x0000557c19b28ea2 in main ()\n\nSince we can't do the cleanup in a portable and signal-safe way, skip\nthe cleanup when we're handling a signal.\n\nThis means that when signal handling, the temporary directory may not\nget cleaned up properly. This is mitigated by b3cecf49ea (tmp-objdir: new\nAPI for creating temporary writable databases, 2021-12-06) which changed\nthe default name and allows gc to clean up these temporary directories.\n\nIn the event of a normal exit, we should still be cleaning up via the\natexit() handler.\n\nHelped-by: Jeff King <peff@peff.net>\nSigned-off-by: John Cai <johncai86@gmail.com>\n---\n    tmp-objdir: do not closedir() when handling a signal\n    \n    We have recently observed a Git process hanging around for weeks. A\n    backtrace revealed that a git-receive-pack(1) process was deadlocked\n    when trying to remove the quarantine directory \"incoming.\" It turns out\n    that the tmp_objdir API calls opendir(3) and closedir(3) to observe a\n    directory's contents in order to remove all the contents before removing\n    the directory itself. These functions are not async signal save as they\n    allocate and free memory.\n    \n    The fix is to avoid calling these functions when handling a signal in\n    order to avoid a deadlock. The implication of such a fix however, is\n    that temporary object directories may not get cleaned up properly when a\n    signal is being handled. The tradeoff this fix is making is to prevent\n    deadlocks at the cost of temporary object directory cleanup.\n    \n    This is similar to 58d4d7f1c5 (2022-01-07 fetch: fix deadlock when\n    cleaning up lockfiles in async signals) The difference being in this\n    case, the cleanup of the files will not happen when handling a signal.\n    \n    Changes since v3:\n    \n     * Updated commit message to clarify that we still clean up the tmp dir\n       on atexit\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1348%2Fjohn-cai%2Fjc%2Ffix-tmp-objdir-signal-handling-v4\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1348/john-cai/jc/fix-tmp-objdir-signal-handling-v4\nPull-Request: https://github.com/git/git/pull/1348\n\nRange-diff vs v3:\n\n 1:  d96c44e309f ! 1:  0ea467f25ca tmp-objdir: skip clean up when handling a signal\n     @@ Commit message\n          API for creating temporary writable databases, 2021-12-06) which changed\n          the default name and allows gc to clean up these temporary directories.\n      \n     +    In the event of a normal exit, we should still be cleaning up via the\n     +    atexit() handler.\n     +\n     +    Helped-by: Jeff King <peff@peff.net>\n          Signed-off-by: John Cai <johncai86@gmail.com>\n      \n       ## tmp-objdir.c ##\n\n\n tmp-objdir.c | 40 ++++------------------------------------\n 1 file changed, 4 insertions(+), 36 deletions(-)\n\ndiff --git a/tmp-objdir.c b/tmp-objdir.c\nindex adf6033549e..2a2012eb6d0 100644\n--- a/tmp-objdir.c\n+++ b/tmp-objdir.c\n@@ -18,7 +18,7 @@ struct tmp_objdir {\n \n /*\n  * Allow only one tmp_objdir at a time in a running process, which simplifies\n- * our signal/atexit cleanup routines.  It's doubtful callers will ever need\n+ * our atexit cleanup routines.  It's doubtful callers will ever need\n  * more than one, and we can expand later if so.  You can have many such\n  * tmp_objdirs simultaneously in many processes, of course.\n  */\n@@ -31,7 +31,7 @@ static void tmp_objdir_free(struct tmp_objdir *t)\n \tfree(t);\n }\n \n-static int tmp_objdir_destroy_1(struct tmp_objdir *t, int on_signal)\n+int tmp_objdir_destroy(struct tmp_objdir *t)\n {\n \tint err;\n \n@@ -41,44 +41,21 @@ static int tmp_objdir_destroy_1(struct tmp_objdir *t, int on_signal)\n \tif (t == the_tmp_objdir)\n \t\tthe_tmp_objdir = NULL;\n \n-\tif (!on_signal && t->prev_odb)\n+\tif (t->prev_odb)\n \t\trestore_primary_odb(t->prev_odb, t->path.buf);\n \n-\t/*\n-\t * This may use malloc via strbuf_grow(), but we should\n-\t * have pre-grown t->path sufficiently so that this\n-\t * doesn't happen in practice.\n-\t */\n \terr = remove_dir_recursively(&t->path, 0);\n \n-\t/*\n-\t * When we are cleaning up due to a signal, we won't bother\n-\t * freeing memory; it may cause a deadlock if the signal\n-\t * arrived while libc's allocator lock is held.\n-\t */\n-\tif (!on_signal)\n-\t\ttmp_objdir_free(t);\n+\ttmp_objdir_free(t);\n \n \treturn err;\n }\n \n-int tmp_objdir_destroy(struct tmp_objdir *t)\n-{\n-\treturn tmp_objdir_destroy_1(t, 0);\n-}\n-\n static void remove_tmp_objdir(void)\n {\n \ttmp_objdir_destroy(the_tmp_objdir);\n }\n \n-static void remove_tmp_objdir_on_signal(int signo)\n-{\n-\ttmp_objdir_destroy_1(the_tmp_objdir, 1);\n-\tsigchain_pop(signo);\n-\traise(signo);\n-}\n-\n void tmp_objdir_discard_objects(struct tmp_objdir *t)\n {\n \tremove_dir_recursively(&t->path, REMOVE_DIR_KEEP_TOPLEVEL);\n@@ -152,14 +129,6 @@ struct tmp_objdir *tmp_objdir_create(const char *prefix)\n \t */\n \tstrbuf_addf(&t->path, \"%s/tmp_objdir-%s-XXXXXX\", get_object_directory(), prefix);\n \n-\t/*\n-\t * Grow the strbuf beyond any filename we expect to be placed in it.\n-\t * If tmp_objdir_destroy() is called by a signal handler, then\n-\t * we should be able to use the strbuf to remove files without\n-\t * having to call malloc.\n-\t */\n-\tstrbuf_grow(&t->path, 1024);\n-\n \tif (!mkdtemp(t->path.buf)) {\n \t\t/* free, not destroy, as we never touched the filesystem */\n \t\ttmp_objdir_free(t);\n@@ -169,7 +138,6 @@ struct tmp_objdir *tmp_objdir_create(const char *prefix)\n \tthe_tmp_objdir = t;\n \tif (!installed_handlers) {\n \t\tatexit(remove_tmp_objdir);\n-\t\tsigchain_push_common(remove_tmp_objdir_on_signal);\n \t\tinstalled_handlers++;\n \t}\n \n\nbase-commit: 4fd6c5e44459e6444c2cd93383660134c95aabd1\n-- \ngitgitgadget\n"},{"id":"464063","messageId":"Yzqix7sKRhpTAMKV@coredump.intra.peff.net","threadId":"58517","inReplyTo":"pull.1348.v4.git.git.1664570831583.gitgitgadget@gmail.com","subject":"Re: [PATCH v4] tmp-objdir: skip clean up when handling a signal","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-10-03T08:52:23Z","receivedAt":"2022-10-03T09:02:58Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Sep 30, 2022 at 08:47:11PM +0000, John Cai via GitGitGadget wrote:\n\n>     Changes since v3:\n>     \n>      * Updated commit message to clarify that we still clean up the tmp dir\n>        on atexit\n\nThanks, this version looks great to me!\n\n-Peff\n"},{"id":"465289","messageId":"00af268377fb7c3b8efd059482ee7449b71f48b1.camel@fnusa.cz","threadId":"58517","inReplyTo":"pull.1348.git.git.1664236383785.gitgitgadget@gmail.com","subject":"Another possible instance of async-signal-safe opendir path callstack? (Was: [PATCH] tmp-objdir: do not opendir() when handling a signal)","fromName":"Jan Pokorný","fromEmail":"poki@fnusa.cz","sentAt":"2022-10-20T11:58:58Z","receivedAt":"2022-10-20T12:04:42Z","isPatch":true,"sender":{"key":"poki@fnusa.cz","avatar":null},"body":"John Cai via GitGitGadget píše v Po 26. 09. 2022 v 23:53 +0000:\n> From: John Cai <johncai86@gmail.com>\n> \n> In the tmp-objdir api, tmp_objdir_create will create a temporary\n> directory but also register signal handlers responsible for removing\n> the directory's contents and the directory itself. However, the\n> function responsible for recursively removing the contents and\n> directory, remove_dir_recurse() calls opendir(3) and closedir(3).\n> This can be problematic because these functions allocate and free\n> memory, which are not async-signal-safe functions. This can lead to\n> deadlocks.\n> \n> One place we call tmp_objdir_create() is in git-receive-pack, where\n> we create a temporary quarantine directory \"incoming\". Incoming\n> objects will be written to this directory before they get moved to\n> the object directory.\n\nJust noticed this unattended git crash in the logs and I think it might\nactually be another occurrence of the same problem in principle, so\nshamelessly piggy-backing the stack trace here (no coredump preserved\nat this point, sorry):\n\n    #0  0x00007f08df0ea06c __pthread_kill_implementation (libc.so.6 + 0x8b06c)\n    #1  0x00007f08df098046 raise (libc.so.6 + 0x39046)\n    #2  0x00007f08df0817fc abort (libc.so.6 + 0x227fc)\n    #3  0x00007f08df082533 __libc_message.cold (libc.so.6 + 0x23533)\n    #4  0x00007f08df090a67 __libc_assert_fail (libc.so.6 + 0x31a67)\n    #5  0x00007f08df0f68f2 sysmalloc (libc.so.6 + 0x978f2)\n    #6  0x00007f08df0f7789 _int_malloc (libc.so.6 + 0x98789)\n    #7  0x00007f08df0f80d2 __libc_malloc (libc.so.6 + 0x990d2)\n    #8  0x00007f08df12fa55 __alloc_dir (libc.so.6 + 0xd0a55)\n  ->#9  0x00007f08df12fac2 opendir_tail (libc.so.6 + 0xd0ac2)\n    #10 0x00005632ea10c823 remove_temporary_files.lto_priv.0 (git + 0xdc823)\n  ->#11 0x00005632ea10c97c remove_pack_on_signal.lto_priv.0 (git + 0xdc97c)\n    #12 0x00007f08df0980f0 __restore_rt (libc.so.6 + 0x390f0)\n    #13 0x00007f08df0f7775 _int_malloc (libc.so.6 + 0x98775)\n    #14 0x00007f08df0f80d2 __libc_malloc (libc.so.6 + 0x990d2)\n    #15 0x00007f08df0d2b44 _IO_file_doallocate (libc.so.6 + 0x73b44)\n    #16 0x00007f08df0e1d20 _IO_doallocbuf (libc.so.6 + 0x82d20)\n    #17 0x00007f08df0e0a8c _IO_file_underflow@@GLIBC_2.2.5 (libc.so.6 + 0x81a8c)\n    #18 0x00007f08df0d4598 __getdelim (libc.so.6 + 0x75598)\n    #19 0x00005632ea29f372 strbuf_getwholeline (git + 0x26f372)\n    #20 0x00005632ea117915 cmd_repack (git + 0xe7915)\n    #21 0x00005632ea0556fa handle_builtin.lto_priv.0 (git + 0x256fa)\n    #22 0x00005632ea050551 main (git + 0x20551)\n    #23 0x00007f08df082a50 __libc_start_call_main (libc.so.6 + 0x23a50)\n    #24 0x00007f08df082b09 __libc_start_main@@GLIBC_2.34 (libc.so.6 + 0x23b09)\n    #25 0x00005632ea051555 _start (git + 0x21555)\n\nAs per the captured info, following Fedora x86_64 packages were involved:\n\n  git-2.37.3-1.fc38\n  zlib-1.2.12-5.fc38\n  pcre2-10.40-1.fc37.1\n\nFull command at hand:\n\n  /usr/libexec/git-core/git repack -d -l --no-write-bitmap-index\n\nIt happened twice, actually, about a week's time apart, but that was\na month ago while this unattended task runs hourly till today and\ngit hasn't been updated since.\n\nNot subscribed to the list and I don't think I can provide more info\non this, but feel free to contact me directly.\n\n-- poki\n"},{"id":"465313","messageId":"Y1GRjH9d9FR5zbrv@coredump.intra.peff.net","threadId":"58517","inReplyTo":"00af268377fb7c3b8efd059482ee7449b71f48b1.camel@fnusa.cz","subject":"Re: Another possible instance of async-signal-safe opendir path callstack? (Was: [PATCH] tmp-objdir: do not opendir() when handling a signal)","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2022-10-20T18:21:00Z","receivedAt":"2022-10-20T18:21:09Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Oct 20, 2022 at 01:58:58PM +0200, Jan Pokorný wrote:\n\n> Just noticed this unattended git crash in the logs and I think it might\n> actually be another occurrence of the same problem in principle, so\n> shamelessly piggy-backing the stack trace here (no coredump preserved\n> at this point, sorry):\n> \n>     #0  0x00007f08df0ea06c __pthread_kill_implementation (libc.so.6 + 0x8b06c)\n>     #1  0x00007f08df098046 raise (libc.so.6 + 0x39046)\n>     #2  0x00007f08df0817fc abort (libc.so.6 + 0x227fc)\n>     #3  0x00007f08df082533 __libc_message.cold (libc.so.6 + 0x23533)\n>     #4  0x00007f08df090a67 __libc_assert_fail (libc.so.6 + 0x31a67)\n>     #5  0x00007f08df0f68f2 sysmalloc (libc.so.6 + 0x978f2)\n>     #6  0x00007f08df0f7789 _int_malloc (libc.so.6 + 0x98789)\n>     #7  0x00007f08df0f80d2 __libc_malloc (libc.so.6 + 0x990d2)\n>     #8  0x00007f08df12fa55 __alloc_dir (libc.so.6 + 0xd0a55)\n>   ->#9  0x00007f08df12fac2 opendir_tail (libc.so.6 + 0xd0ac2)\n>     #10 0x00005632ea10c823 remove_temporary_files.lto_priv.0 (git + 0xdc823)\n>   ->#11 0x00005632ea10c97c remove_pack_on_signal.lto_priv.0 (git + 0xdc97c)\n>     #12 0x00007f08df0980f0 __restore_rt (libc.so.6 + 0x390f0)\n>     #13 0x00007f08df0f7775 _int_malloc (libc.so.6 + 0x98775)\n>     #14 0x00007f08df0f80d2 __libc_malloc (libc.so.6 + 0x990d2)\n>     #15 0x00007f08df0d2b44 _IO_file_doallocate (libc.so.6 + 0x73b44)\n>     #16 0x00007f08df0e1d20 _IO_doallocbuf (libc.so.6 + 0x82d20)\n>     #17 0x00007f08df0e0a8c _IO_file_underflow@@GLIBC_2.2.5 (libc.so.6 + 0x81a8c)\n>     #18 0x00007f08df0d4598 __getdelim (libc.so.6 + 0x75598)\n>     #19 0x00005632ea29f372 strbuf_getwholeline (git + 0x26f372)\n>     #20 0x00005632ea117915 cmd_repack (git + 0xe7915)\n>     #21 0x00005632ea0556fa handle_builtin.lto_priv.0 (git + 0x256fa)\n>     #22 0x00005632ea050551 main (git + 0x20551)\n>     #23 0x00007f08df082a50 __libc_start_call_main (libc.so.6 + 0x23a50)\n>     #24 0x00007f08df082b09 __libc_start_main@@GLIBC_2.34 (libc.so.6 + 0x23b09)\n>     #25 0x00005632ea051555 _start (git + 0x21555)\n\nGood find. This is definitely the same issue.\n\nUnfortunately it's another hard one to fix.\n\ngit-repack asks git-pack-objects to use .tmp-pack-$$ as a prefix. So I\nthought we might be able to get away with using register_tempfile() to\njust record the names of potential files it creates by appending .pack,\n.idx, and so forth. That code is well-tested and is careful to be\nsignal-safe.\n\nBut the actual name will include the checksum of the packfile, so\n.tmp-pack-$$-1234abcd.pack, etc. And repack doesn't know those until\npack-objects reports the hash to us. OTOH, I think pack-objects doesn't\ncreate the files with that name until the very last minute (it has its\nown tempfiles[1]). So we could probably get away with expanding the\nfilenames once it tells us the hash.\n\nSomething like the patch below.\n\ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex a5bacc7797..8d6121b438 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -122,13 +122,6 @@ static void remove_temporary_files(void)\n \tstrbuf_release(&buf);\n }\n \n-static void remove_pack_on_signal(int signo)\n-{\n-\tremove_temporary_files();\n-\tsigchain_pop(signo);\n-\traise(signo);\n-}\n-\n /*\n  * Adds all packs hex strings to either fname_nonkept_list or\n  * fname_kept_list based on whether each pack has a corresponding\n@@ -268,6 +261,17 @@ static unsigned populate_pack_exts(char *name)\n \treturn ret;\n }\n \n+static void register_tmp_pack_for_cleanup(const char *name)\n+{\n+\tint i;\n+\n+\tfor (i = 0; i < ARRAY_SIZE(exts); i++) {\n+\t\tchar *path = xstrfmt(\"%s-%s%s\", packtmp, name, exts[i].name);\n+\t\tregister_tempfile(path);\n+\t\tfree(path);\n+\t}\n+}\n+\n static void repack_promisor_objects(const struct pack_objects_args *args,\n \t\t\t\t    struct string_list *names)\n {\n@@ -714,6 +718,7 @@ static int write_cruft_pack(const struct pack_objects_args *args,\n \t\t\tdie(_(\"repack: Expecting full hex object ID lines only \"\n \t\t\t      \"from pack-objects.\"));\n \t\tstring_list_append(names, line.buf);\n+\t\tregister_tmp_pack_for_cleanup(line.buf);\n \t}\n \tfclose(out);\n \n@@ -859,8 +864,6 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \t\tsplit_pack_geometry(geometry, geometric_factor);\n \t}\n \n-\tsigchain_push_common(remove_pack_on_signal);\n-\n \tprepare_pack_objects(&cmd, &po_args);\n \n \tshow_progress = !po_args.quiet && isatty(2);\n@@ -955,6 +958,7 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \t\tif (line.len != the_hash_algo->hexsz)\n \t\t\tdie(_(\"repack: Expecting full hex object ID lines only from pack-objects.\"));\n \t\tstring_list_append(&names, line.buf);\n+\t\tregister_tmp_pack_for_cleanup(line.buf);\n \t}\n \tfclose(out);\n \tret = finish_command(&cmd);\n@@ -1025,6 +1029,16 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \t\t\telse if (unlink(fname) < 0 && errno != ENOENT)\n \t\t\t\tdie_errno(_(\"could not unlink: %s\"), fname);\n \n+\t\t\t/*\n+\t\t\t * We should deregister pack tempfiles here; leaving\n+\t\t\t * them kind of works in that we'll try to unlink\n+\t\t\t * something that is already gone, but it would be\n+\t\t\t * cleaner to note that we expect them to be gone here.\n+\t\t\t * We could perhaps even just use rename_tempfile() and\n+\t\t\t * delete_tempfile() here, if we modified item->util to\n+\t\t\t * keep track of the tempfile objects (rather than\n+\t\t\t * single bits).\n+\t\t\t */\n \t\t\tfree(fname);\n \t\t\tfree(fname_old);\n \t\t}\n\n-Peff\n\n[1] Note that to some degree, this cleanup is already a bit of a fool's\n    errand. It is only catching the \"finished\" packfiles before we move\n    them to their final names. Whereas pack-objects is perfectly happy\n    to write bytes into tmp_pack_XXXXXX, and does not bother to clean it\n    if it runs into an error or signal. And that is where most of the\n    time will be spent.\n\n    So one option would be to just drop this signal handling entirely.\n    But IMHO we should go the other way, and teach pack-objects to also\n    clean up after itself. I know GitHub has been running with patches\n    to call register_tempfile() like this for years (but it is hacky the\n    way the above patch is; it never unregisters, which is why we never\n    sent them upstream).\n\n    Another problem is that repack.c only catches signals, not die(),\n    which is the more likely culprit (e.g., you generate the \"main\"\n    pack, and then something goes wrong while renaming or generating\n    an ancillary cruft-packs). Switching to register_tempfile() does fix\n    that, though.\n"}]}