patchtmp-objdir: do not opendir() when handling a signal

From: John Cai <johncai86@gmail.com>

In the tmp-objdir api, tmp_objdir_create will create a temporary directory but also register signal handlers responsible for removing the directory's contents and the directory itself. However, the function responsible for recursively removing the contents and directory, remove_dir_recurse() calls opendir(3) and closedir(3). This can be problematic because these functions allocate and free memory, which are not async-signal-safe functions. This can lead to deadlocks.

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.

We have observed this code leading to a deadlock:

	Thread 1 (Thread 0x7f621ba0b200 (LWP 326305)):
	#0  __lll_lock_wait_private (futex=futex@entry=0x7f621bbf8b80
		<main_arena>) at ./lowlevellock.c:35
	#1  0x00007f621baa635b in __GI___libc_malloc
		(bytes=bytes@entry=32816) at malloc.c:3064
	#2  0x00007f621bae9f49 in __alloc_dir (statp=0x7fff2ea7ed60,
		flags=0, close_fd=true, fd=5)
		at ../sysdeps/posix/opendir.c:118
	#3  opendir_tail (fd=5) at ../sysdeps/posix/opendir.c:69
	#4  __opendir (name=<optimized out>)
		at ../sysdeps/posix/opendir.c:92
	#5  0x0000557c19c77de1 in remove_dir_recurse ()
	#6  0x0000557c19d81a4f in remove_tmp_objdir_on_signal ()
	#7  <signal handler called>
	#8  _int_malloc (av=av@entry=0x7f621bbf8b80 <main_arena>,
		bytes=bytes@entry=7160) at malloc.c:4116
	#9  0x00007f621baa62c9 in __GI___libc_malloc (bytes=7160)
		at malloc.c:3066
	#10 0x00007f621bd1e987 in inflateInit2_ ()
		from /opt/gitlab/embedded/lib/libz.so.1
	#11 0x0000557c19dbe5f4 in git_inflate_init ()
	#12 0x0000557c19cee02a in unpack_compressed_entry ()
	#13 0x0000557c19cf08cb in unpack_entry ()
	#14 0x0000557c19cf0f32 in packed_object_info ()
	#15 0x0000557c19cd68cd in do_oid_object_info_extended ()
	#16 0x0000557c19cd6e2b in read_object_file_extended ()
	#17 0x0000557c19cdec2f in parse_object ()
	#18 0x0000557c19c34977 in lookup_commit_reference_gently ()
	#19 0x0000557c19d69309 in mark_uninteresting ()
	#20 0x0000557c19d2d180 in do_for_each_repo_ref_iterator ()
	#21 0x0000557c19d21678 in for_each_ref ()
	#22 0x0000557c19d6a94f in assign_shallow_commits_to_refs ()
	#23 0x0000557c19bc02b2 in cmd_receive_pack ()
	#24 0x0000557c19b29fdd in handle_builtin ()
	#25 0x0000557c19b2a526 in cmd_main ()
	#26 0x0000557c19b28ea2 in main ()

To prevent this, add a flag REMOVE_DIR_SIGNAL that allows remove_dir_recurse() to know that a signal is being handled and avoid calling opendir(3). One implication of this change is that when signal handling, the temporary directory may not get cleaned up properly.

Signed-off-by: John Cai <johncai86@gmail.com>
---
    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)

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1348%2Fjohn-cai%2Fjc%2Ffix-tmp-objdir-signal-handling-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1348/john-cai/jc/fix-tmp-objdir-signal-handling-v1
Pull-Request: https://github.com/git/git/pull/1348

 dir.c        | 7 +++++--
 dir.h        | 3 +++
 tmp-objdir.c | 7 ++++++-
 3 files changed, 14 insertions(+), 3 deletions(-)

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

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

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.

Comparing to the zero value is discouraged. Consider:

    if (!(flag & REMOVE_DIR_SIGNAL))

instead.

Perhaps REMOVE_DIR_IN_SIGNAL would be slightly more descriptive.

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

On Mon, Sep 26, 2022 at 08:18:47PM -0400, Taylor Blau wrote:

You'd need to hold multiple such DIRs, since the removal is recursive. It's easy-ish for the "pack" directory, but the sub-process index-pack may have created 00-ff directories, too. You'd have to pre-create and opendir() all of them.

I'm also not sure what timing guarantees we have. If I opendir() a directory, then wait a long time while somebody else creates entries, is a readdir() guaranteed to see those new entries?

-Peff

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

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.

The fix looks quite straight-forward.

Thanks for spotting and working on this issue.

Hi John

On 27/09/2022 00:53, John Cai via GitGitGadget wrote:

This is setting the bits for REMOVE_DIR_KEEP_NESTED_GIT and REMOVE_DIR_KEEP_TOPLEVEL is that intentional? (it looks like you've doubled 8 to 16 to get the next free bit but used a hex constant, the earlier constants use decimal)

Best Wishes

Phillip

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

Yuck. Your analysis looks right, and certainly opendir() can't really work without allocating memory for the pointer-to-DIR.

It's not really "may not" here, is it? It will never get cleaned up on a signal now. I don't think remove_dir_recursively() will try to rmdir() in this case. But even if it did, we'll always have a "pack" subdirectory (minus the small race before we've created it).

That's unfortunate, but I don't think we really have a portable alternative. We can't keep an exact list of files to be deleted, because some of them will be created by sub-processes. We could perhaps exec a helper that does the deletion, but that seems like a race and portability nightmare. On Linux, we could probably use open() and getdents64() to traverse, but obviously that won't work everywhere. It _might_ be worth having some kind of compat/ wrapper here, to let supported systems do as good a job as they can. But it's not like there aren't already cases where we might leave the tmp-objdir directory around (say, SIGKILL), so this is really just extending the existing problem to more signals.

I was going to suggest we should do a better job of cleaning up these directories via git-gc. But it looks like b3cecf49ea (tmp-objdir: new API for creating temporary writable databases, 2021-12-06) changed the default name such that a regular git-gc should do so. So I think we're covered there.

The main case we care about is normal exit when index-pack or a hook sees an error, in which case we should still be cleaning up via the atexit() handler.

So I think your patch is going in the right direction, but...

We skip calling opendir() entirely, so "dir" will still be NULL. But we immediately start looking at errno, which will have some undefined value (based on some previous syscall).

If we set "errno" to "EACCES" in this case, then we'd at least hit the rmdir() below:

         if (!dir) {
                  if (errno == ENOENT)
                          return keep_toplevel ? -1 : 0;
                  else if (errno == EACCES && !keep_toplevel)
                          /*
                           * An empty dir could be removable even if it
                           * is unreadable:
                           */
                          return rmdir(path->buf);
                  else
                          return -1;
          }

but we know it won't really do anything for our proposed caller, since it will have files inside the directory that need to be removed before rmdir() can work.

Moreover, if you were to combine REMOVE_DIR_SIGNAL with REMOVE_DIR_KEEP_NESTED_GIT, I suspect that the call to resolve_gitlink_ref() would end up with similar deadlocks. Obviously nobody is proposing to do that, but it is a pitfall in the API.

So all of that makes me think we should not add a new flag here, but instead just avoid calling the function entirely from tmp_objdir_destroy_1().

But then we can observe that tmp_objdir_destroy_1() is basically doing nothing if on_signal is set. So there is really no point in setting up the signal handler at all. We should just set up the atexit() handler. I.e., something like:

Hey Peff,

On 27 Sep 2022, at 7:44, Jeff King wrote:

I was wondering about this as well. Thanks for checking on this--that's reassuring.

yeah, I suppose the only case it would help is if the directory is already empty.

This makes sense and is a clean solution. I guess the only case where we would benefit in calling into remove_tmp_objdir_on_signal() is if the temp dir exists but is empty. I'm not sure how often this would happen to make it worth it? Probably not...

On Tue, Sep 27, 2022 at 09:50:43AM -0400, John Cai wrote:

It's more or less never, as the first thing we do after creating it is call tmp_objdir_setup(), which creates the pack subdirectory. We _could_ try removing them both manually, like:

Jeff King <peff@peff.net> writes:

Thanks. I missed that undefined access to errno that breaks the intention of the patch.

;-)  I like the simplification.

From: John Cai <johncai86@gmail.com>

In the tmp-objdir api, tmp_objdir_create will create a temporary directory but also register signal handlers responsible for removing the directory's contents and the directory itself. However, the function responsible for recursively removing the contents and directory, remove_dir_recurse() calls opendir(3) and closedir(3). This can be problematic because these functions allocate and free memory, which are not async-signal-safe functions. This can lead to deadlocks.

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.

We have observed this code leading to a deadlock:

	Thread 1 (Thread 0x7f621ba0b200 (LWP 326305)):
	#0  __lll_lock_wait_private (futex=futex@entry=0x7f621bbf8b80
		<main_arena>) at ./lowlevellock.c:35
	#1  0x00007f621baa635b in __GI___libc_malloc
		(bytes=bytes@entry=32816) at malloc.c:3064
	#2  0x00007f621bae9f49 in __alloc_dir (statp=0x7fff2ea7ed60,
		flags=0, close_fd=true, fd=5)
		at ../sysdeps/posix/opendir.c:118
	#3  opendir_tail (fd=5) at ../sysdeps/posix/opendir.c:69
	#4  __opendir (name=<optimized out>)
		at ../sysdeps/posix/opendir.c:92
	#5  0x0000557c19c77de1 in remove_dir_recurse ()
	git#6  0x0000557c19d81a4f in remove_tmp_objdir_on_signal ()
	#7  <signal handler called>
	git#8  _int_malloc (av=av@entry=0x7f621bbf8b80 <main_arena>,
		bytes=bytes@entry=7160) at malloc.c:4116
	git#9  0x00007f621baa62c9 in __GI___libc_malloc (bytes=7160)
		at malloc.c:3066
	git#10 0x00007f621bd1e987 in inflateInit2_ ()
		from /opt/gitlab/embedded/lib/libz.so.1
	git#11 0x0000557c19dbe5f4 in git_inflate_init ()
	git#12 0x0000557c19cee02a in unpack_compressed_entry ()
	git#13 0x0000557c19cf08cb in unpack_entry ()
	git#14 0x0000557c19cf0f32 in packed_object_info ()
	git#15 0x0000557c19cd68cd in do_oid_object_info_extended ()
	git#16 0x0000557c19cd6e2b in read_object_file_extended ()
	git#17 0x0000557c19cdec2f in parse_object ()
	git#18 0x0000557c19c34977 in lookup_commit_reference_gently ()
	git#19 0x0000557c19d69309 in mark_uninteresting ()
	git#20 0x0000557c19d2d180 in do_for_each_repo_ref_iterator ()
	git#21 0x0000557c19d21678 in for_each_ref ()
	git#22 0x0000557c19d6a94f in assign_shallow_commits_to_refs ()
	git#23 0x0000557c19bc02b2 in cmd_receive_pack ()
	git#24 0x0000557c19b29fdd in handle_builtin ()
	git#25 0x0000557c19b2a526 in cmd_main ()
	git#26 0x0000557c19b28ea2 in main ()

Since we can't do the cleanup in a portable and signal-safe way, skip the cleanup when we're handling a signal.

This means that when signal handling, the temporary directory may not get cleaned up properly. This is mitigated by b3cecf49ea (tmp-objdir: new API for creating temporary writable databases, 2021-12-06) which changed the default name and allows gc to clean up these temporary directories.

Signed-off-by: John Cai <johncai86@gmail.com>
---
    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) The difference being in this
    case, the cleanup of the files will not happen when handling a signal.

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1348%2Fjohn-cai%2Fjc%2Ffix-tmp-objdir-signal-handling-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1348/john-cai/jc/fix-tmp-objdir-signal-handling-v2
Pull-Request: https://github.com/git/git/pull/1348

Range-diff vs v1:

 1:  78ee805c5aa < -:  ----------- tmp-objdir: do not opendir() when handling a signal
 -:  ----------- > 1:  ae58df8c938 tmp-objdir: skip clean up when handling a signal

 tmp-objdir.c | 22 ++++------------------
 1 file changed, 4 insertions(+), 18 deletions(-)

On Tue, Sep 27, 2022 at 07:19:01PM +0000, John Cai via GitGitGadget wrote:

Thanks, this looks fine to me, though I think there are a few extra cleanup opportunities that could be squashed in:

On Tue, Sep 27, 2022 at 03:38:26PM -0400, Jeff King wrote:

Whoops, you'd obviously want to drop the "static" here, as well.

-Peff

From: John Cai <johncai86@gmail.com>

In the tmp-objdir api, tmp_objdir_create will create a temporary directory but also register signal handlers responsible for removing the directory's contents and the directory itself. However, the function responsible for recursively removing the contents and directory, remove_dir_recurse() calls opendir(3) and closedir(3). This can be problematic because these functions allocate and free memory, which are not async-signal-safe functions. This can lead to deadlocks.

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.

We have observed this code leading to a deadlock:

	Thread 1 (Thread 0x7f621ba0b200 (LWP 326305)):
	#0  __lll_lock_wait_private (futex=futex@entry=0x7f621bbf8b80
		<main_arena>) at ./lowlevellock.c:35
	#1  0x00007f621baa635b in __GI___libc_malloc
		(bytes=bytes@entry=32816) at malloc.c:3064
	#2  0x00007f621bae9f49 in __alloc_dir (statp=0x7fff2ea7ed60,
		flags=0, close_fd=true, fd=5)
		at ../sysdeps/posix/opendir.c:118
	#3  opendir_tail (fd=5) at ../sysdeps/posix/opendir.c:69
	#4  __opendir (name=<optimized out>)
		at ../sysdeps/posix/opendir.c:92
	#5  0x0000557c19c77de1 in remove_dir_recurse ()
	git#6  0x0000557c19d81a4f in remove_tmp_objdir_on_signal ()
	#7  <signal handler called>
	git#8  _int_malloc (av=av@entry=0x7f621bbf8b80 <main_arena>,
		bytes=bytes@entry=7160) at malloc.c:4116
	git#9  0x00007f621baa62c9 in __GI___libc_malloc (bytes=7160)
		at malloc.c:3066
	git#10 0x00007f621bd1e987 in inflateInit2_ ()
		from /opt/gitlab/embedded/lib/libz.so.1
	git#11 0x0000557c19dbe5f4 in git_inflate_init ()
	git#12 0x0000557c19cee02a in unpack_compressed_entry ()
	git#13 0x0000557c19cf08cb in unpack_entry ()
	git#14 0x0000557c19cf0f32 in packed_object_info ()
	git#15 0x0000557c19cd68cd in do_oid_object_info_extended ()
	git#16 0x0000557c19cd6e2b in read_object_file_extended ()
	git#17 0x0000557c19cdec2f in parse_object ()
	git#18 0x0000557c19c34977 in lookup_commit_reference_gently ()
	git#19 0x0000557c19d69309 in mark_uninteresting ()
	git#20 0x0000557c19d2d180 in do_for_each_repo_ref_iterator ()
	git#21 0x0000557c19d21678 in for_each_ref ()
	git#22 0x0000557c19d6a94f in assign_shallow_commits_to_refs ()
	git#23 0x0000557c19bc02b2 in cmd_receive_pack ()
	git#24 0x0000557c19b29fdd in handle_builtin ()
	git#25 0x0000557c19b2a526 in cmd_main ()
	git#26 0x0000557c19b28ea2 in main ()

Since we can't do the cleanup in a portable and signal-safe way, skip the cleanup when we're handling a signal.

This means that when signal handling, the temporary directory may not get cleaned up properly. This is mitigated by b3cecf49ea (tmp-objdir: new API for creating temporary writable databases, 2021-12-06) which changed the default name and allows gc to clean up these temporary directories.

Signed-off-by: John Cai <johncai86@gmail.com>
---
    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) The difference being in this
    case, the cleanup of the files will not happen when handling a signal.

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1348%2Fjohn-cai%2Fjc%2Ffix-tmp-objdir-signal-handling-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1348/john-cai/jc/fix-tmp-objdir-signal-handling-v3
Pull-Request: https://github.com/git/git/pull/1348

Range-diff vs v2:

 1:  ae58df8c938 ! 1:  d96c44e309f tmp-objdir: skip clean up when handling a signal
     @@ Commit message
          Signed-off-by: John Cai <johncai86@gmail.com>
      
       ## tmp-objdir.c ##
     +@@ tmp-objdir.c: struct tmp_objdir {
     + 
     + /*
     +  * Allow only one tmp_objdir at a time in a running process, which simplifies
     +- * our signal/atexit cleanup routines.  It's doubtful callers will ever need
     ++ * our atexit cleanup routines.  It's doubtful callers will ever need
     +  * more than one, and we can expand later if so.  You can have many such
     +  * tmp_objdirs simultaneously in many processes, of course.
     +  */
      @@ tmp-objdir.c: static void tmp_objdir_free(struct tmp_objdir *t)
       	free(t);
       }
       
      -static int tmp_objdir_destroy_1(struct tmp_objdir *t, int on_signal)
     -+static int tmp_objdir_destroy_1(struct tmp_objdir *t)
     ++int tmp_objdir_destroy(struct tmp_objdir *t)
       {
       	int err;
       
     @@ tmp-objdir.c: static int tmp_objdir_destroy_1(struct tmp_objdir *t, int on_signa
      +	if (t->prev_odb)
       		restore_primary_odb(t->prev_odb, t->path.buf);
       
     - 	/*
     -@@ tmp-objdir.c: static int tmp_objdir_destroy_1(struct tmp_objdir *t, int on_signal)
     - 	 */
     +-	/*
     +-	 * This may use malloc via strbuf_grow(), but we should
     +-	 * have pre-grown t->path sufficiently so that this
     +-	 * doesn't happen in practice.
     +-	 */
       	err = remove_dir_recursively(&t->path, 0);
       
      -	/*
     @@ tmp-objdir.c: static int tmp_objdir_destroy_1(struct tmp_objdir *t, int on_signa
       	return err;
       }
       
     - int tmp_objdir_destroy(struct tmp_objdir *t)
     - {
     +-int tmp_objdir_destroy(struct tmp_objdir *t)
     +-{
      -	return tmp_objdir_destroy_1(t, 0);
     -+	return tmp_objdir_destroy_1(t);
     - }
     - 
     +-}
     +-
       static void remove_tmp_objdir(void)
     -@@ tmp-objdir.c: static void remove_tmp_objdir(void)
     + {
       	tmp_objdir_destroy(the_tmp_objdir);
       }
       
     @@ tmp-objdir.c: static void remove_tmp_objdir(void)
       void tmp_objdir_discard_objects(struct tmp_objdir *t)
       {
       	remove_dir_recursively(&t->path, REMOVE_DIR_KEEP_TOPLEVEL);
     +@@ tmp-objdir.c: struct tmp_objdir *tmp_objdir_create(const char *prefix)
     + 	 */
     + 	strbuf_addf(&t->path, "%s/tmp_objdir-%s-XXXXXX", get_object_directory(), prefix);
     + 
     +-	/*
     +-	 * Grow the strbuf beyond any filename we expect to be placed in it.
     +-	 * If tmp_objdir_destroy() is called by a signal handler, then
     +-	 * we should be able to use the strbuf to remove files without
     +-	 * having to call malloc.
     +-	 */
     +-	strbuf_grow(&t->path, 1024);
     +-
     + 	if (!mkdtemp(t->path.buf)) {
     + 		/* free, not destroy, as we never touched the filesystem */
     + 		tmp_objdir_free(t);
      @@ tmp-objdir.c: struct tmp_objdir *tmp_objdir_create(const char *prefix)
       	the_tmp_objdir = t;
       	if (!installed_handlers) {

 tmp-objdir.c | 40 ++++------------------------------------
 1 file changed, 4 insertions(+), 36 deletions(-)

On Wed, Sep 28 2022, John Cai via GitGitGadget wrote:

This looks good! Just a clarifying question/comment:

I think this still doesn't cover the common case of the "atexit" handler saving the day, as Jeff King pointed out in v1:

	https://lore.kernel.org/git/YzLiI1HZeBszsIJq@coredump.intra.peff.net/

I think it's fine to have this proceed as-is (although if you're doing a v4 anyway, maybe we want to update tho commit message). I.e. is it correct that we'll now only skip this if we get a signal, *and* it's a fatal signal, or we otherwise die/exit/abort/whatever before we can get to our atexit() handler?

From: John Cai <johncai86@gmail.com>

In the tmp-objdir api, tmp_objdir_create will create a temporary directory but also register signal handlers responsible for removing the directory's contents and the directory itself. However, the function responsible for recursively removing the contents and directory, remove_dir_recurse() calls opendir(3) and closedir(3). This can be problematic because these functions allocate and free memory, which are not async-signal-safe functions. This can lead to deadlocks.

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.

We have observed this code leading to a deadlock:

	Thread 1 (Thread 0x7f621ba0b200 (LWP 326305)):
	#0  __lll_lock_wait_private (futex=futex@entry=0x7f621bbf8b80
		<main_arena>) at ./lowlevellock.c:35
	#1  0x00007f621baa635b in __GI___libc_malloc
		(bytes=bytes@entry=32816) at malloc.c:3064
	#2  0x00007f621bae9f49 in __alloc_dir (statp=0x7fff2ea7ed60,
		flags=0, close_fd=true, fd=5)
		at ../sysdeps/posix/opendir.c:118
	#3  opendir_tail (fd=5) at ../sysdeps/posix/opendir.c:69
	#4  __opendir (name=<optimized out>)
		at ../sysdeps/posix/opendir.c:92
	#5  0x0000557c19c77de1 in remove_dir_recurse ()
	git#6  0x0000557c19d81a4f in remove_tmp_objdir_on_signal ()
	#7  <signal handler called>
	git#8  _int_malloc (av=av@entry=0x7f621bbf8b80 <main_arena>,
		bytes=bytes@entry=7160) at malloc.c:4116
	git#9  0x00007f621baa62c9 in __GI___libc_malloc (bytes=7160)
		at malloc.c:3066
	git#10 0x00007f621bd1e987 in inflateInit2_ ()
		from /opt/gitlab/embedded/lib/libz.so.1
	git#11 0x0000557c19dbe5f4 in git_inflate_init ()
	git#12 0x0000557c19cee02a in unpack_compressed_entry ()
	git#13 0x0000557c19cf08cb in unpack_entry ()
	git#14 0x0000557c19cf0f32 in packed_object_info ()
	git#15 0x0000557c19cd68cd in do_oid_object_info_extended ()
	git#16 0x0000557c19cd6e2b in read_object_file_extended ()
	git#17 0x0000557c19cdec2f in parse_object ()
	git#18 0x0000557c19c34977 in lookup_commit_reference_gently ()
	git#19 0x0000557c19d69309 in mark_uninteresting ()
	git#20 0x0000557c19d2d180 in do_for_each_repo_ref_iterator ()
	git#21 0x0000557c19d21678 in for_each_ref ()
	git#22 0x0000557c19d6a94f in assign_shallow_commits_to_refs ()
	git#23 0x0000557c19bc02b2 in cmd_receive_pack ()
	git#24 0x0000557c19b29fdd in handle_builtin ()
	git#25 0x0000557c19b2a526 in cmd_main ()
	git#26 0x0000557c19b28ea2 in main ()

Since we can't do the cleanup in a portable and signal-safe way, skip the cleanup when we're handling a signal.

This means that when signal handling, the temporary directory may not get cleaned up properly. This is mitigated by b3cecf49ea (tmp-objdir: new API for creating temporary writable databases, 2021-12-06) which changed the default name and allows gc to clean up these temporary directories.

In the event of a normal exit, we should still be cleaning up via the atexit() handler.

Helped-by: Jeff King <peff@peff.net>
Signed-off-by: John Cai <johncai86@gmail.com>
---
    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) The difference being in this
    case, the cleanup of the files will not happen when handling a signal.
    
    Changes since v3:
    
     * Updated commit message to clarify that we still clean up the tmp dir
       on atexit

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1348%2Fjohn-cai%2Fjc%2Ffix-tmp-objdir-signal-handling-v4
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1348/john-cai/jc/fix-tmp-objdir-signal-handling-v4
Pull-Request: https://github.com/git/git/pull/1348

Range-diff vs v3:

 1:  d96c44e309f ! 1:  0ea467f25ca tmp-objdir: skip clean up when handling a signal
     @@ Commit message
          API for creating temporary writable databases, 2021-12-06) which changed
          the default name and allows gc to clean up these temporary directories.
      
     +    In the event of a normal exit, we should still be cleaning up via the
     +    atexit() handler.
     +
     +    Helped-by: Jeff King <peff@peff.net>
          Signed-off-by: John Cai <johncai86@gmail.com>
      
       ## tmp-objdir.c ##

 tmp-objdir.c | 40 ++++------------------------------------
 1 file changed, 4 insertions(+), 36 deletions(-)

On Fri, Sep 30, 2022 at 08:47:11PM +0000, John Cai via GitGitGadget wrote:

Thanks, this version looks great to me!

-Peff

John Cai via GitGitGadget píše v Po 26. 09. 2022 v 23:53 +0000:

Just noticed this unattended git crash in the logs and I think it might actually be another occurrence of the same problem in principle, so shamelessly piggy-backing the stack trace here (no coredump preserved at this point, sorry):

    #0  0x00007f08df0ea06c __pthread_kill_implementation (libc.so.6 + 0x8b06c)
    #1  0x00007f08df098046 raise (libc.so.6 + 0x39046)
    #2  0x00007f08df0817fc abort (libc.so.6 + 0x227fc)
    #3  0x00007f08df082533 __libc_message.cold (libc.so.6 + 0x23533)
    #4  0x00007f08df090a67 __libc_assert_fail (libc.so.6 + 0x31a67)
    #5  0x00007f08df0f68f2 sysmalloc (libc.so.6 + 0x978f2)
    #6  0x00007f08df0f7789 _int_malloc (libc.so.6 + 0x98789)
    #7  0x00007f08df0f80d2 __libc_malloc (libc.so.6 + 0x990d2)
    #8  0x00007f08df12fa55 __alloc_dir (libc.so.6 + 0xd0a55)
  ->#9  0x00007f08df12fac2 opendir_tail (libc.so.6 + 0xd0ac2)
    #10 0x00005632ea10c823 remove_temporary_files.lto_priv.0 (git + 0xdc823)
  ->#11 0x00005632ea10c97c remove_pack_on_signal.lto_priv.0 (git + 0xdc97c)
    #12 0x00007f08df0980f0 __restore_rt (libc.so.6 + 0x390f0)
    #13 0x00007f08df0f7775 _int_malloc (libc.so.6 + 0x98775)
    #14 0x00007f08df0f80d2 __libc_malloc (libc.so.6 + 0x990d2)
    #15 0x00007f08df0d2b44 _IO_file_doallocate (libc.so.6 + 0x73b44)
    #16 0x00007f08df0e1d20 _IO_doallocbuf (libc.so.6 + 0x82d20)
    #17 0x00007f08df0e0a8c _IO_file_underflow@@GLIBC_2.2.5 (libc.so.6 + 0x81a8c)
    #18 0x00007f08df0d4598 __getdelim (libc.so.6 + 0x75598)
    #19 0x00005632ea29f372 strbuf_getwholeline (git + 0x26f372)
    #20 0x00005632ea117915 cmd_repack (git + 0xe7915)
    #21 0x00005632ea0556fa handle_builtin.lto_priv.0 (git + 0x256fa)
    #22 0x00005632ea050551 main (git + 0x20551)
    #23 0x00007f08df082a50 __libc_start_call_main (libc.so.6 + 0x23a50)
    #24 0x00007f08df082b09 __libc_start_main@@GLIBC_2.34 (libc.so.6 + 0x23b09)
    #25 0x00005632ea051555 _start (git + 0x21555)

As per the captured info, following Fedora x86_64 packages were involved:

  git-2.37.3-1.fc38
  zlib-1.2.12-5.fc38
  pcre2-10.40-1.fc37.1

Full command at hand:

  /usr/libexec/git-core/git repack -d -l --no-write-bitmap-index

It happened twice, actually, about a week's time apart, but that was a month ago while this unattended task runs hourly till today and git hasn't been updated since.

Not subscribed to the list and I don't think I can provide more info on this, but feel free to contact me directly.

-- poki

On Thu, Oct 20, 2022 at 01:58:58PM +0200, Jan Pokorný wrote:

Good find. This is definitely the same issue.

Unfortunately it's another hard one to fix.

git-repack asks git-pack-objects to use .tmp-pack-$$ as a prefix. So I thought we might be able to get away with using register_tempfile() to just record the names of potential files it creates by appending .pack, .idx, and so forth. That code is well-tested and is careful to be signal-safe.

But the actual name will include the checksum of the packfile, so .tmp-pack-$$-1234abcd.pack, etc. And repack doesn't know those until pack-objects reports the hash to us. OTOH, I think pack-objects doesn't create the files with that name until the very last minute (it has its own tempfiles[1]). So we could probably get away with expanding the filenames once it tells us the hash.

Something like the patch below.