Async-signal safety in signal handlers
cover Async-signal safety in signal handlers
Hi,
we have recently observed a Git process which has been hanging around for more than a month on one of our servers in production. A backtrace showed that the git-fetch(1) process was deadlocked in its signal handler while trying to free memory. Functions like malloc, free and most I/O functions aren't reentrant though, which means they must not be executed in async signal handlers as specified in signal-safety(7).
The fix for git-fetch(1) is rather simple: we can just unlink(2) the lockfiles, which is indeed allowed, but skip free'ing memory. But in fact, this is a wider issue we have: we mostly didn't pay attention to those restrictions, and thus we freely call non-async-signal-safe functions. It's less clear what to do about this in most of the cases though:
- git-clone(1) tries to clean up the ".git" directory and its worktree on being killed, but needs to allocate memory to compute corresponding paths. We can try to preallocate the buffer, but it's not clear whether there is a proper upper boundary.
- git-gc(1) will try to commit "gc.log" and write to stderr, both of which aren't allowed. I think we'll have to just bail and leave it behind in a partially-written state.
- git-repack(1) tries to remove "pack/.tmp-*" files, calling opendir(3P), readdir(3P), closedir(3P) and allocates memory. We probably have to keep track of all temporary files we create in a global list, which we can then access in our signal handler.
- git-worktree(1) is doing the same as git-clone(1), trying to prune the new worktree if it's killed. Again, we'd probably have to preallocate a buffer to compute paths.
- HTTP pushes do all sorts of HTTP requests in their signal handler to unlock the remote server. I don't really see what to do about this except drop the code -- setting a global "please clean up and exit now" flags is probably not going to fly well.
The tempfiles and tmp-objdir code already handles signals correctly.
Patrick
Patrick Steinhardt (1):
fetch: fix deadlock when cleaning up lockfiles in async signals
builtin/clone.c | 2 +- builtin/fetch.c | 17 +++++++++++------ transport.c | 11 ++++++++--- transport.h | 14 +++++++++++++- 4 files changed, 33 insertions(+), 11 deletions(-)
Signature · 1 line
2.34.1
PATCH 1/1 fetch: fix deadlock when cleaning up lockfiles in async signals
[Resend with the correct In-Reply-To header set to fix threading]
When fetching packfiles, we write a bunch of lockfiles for the packfiles we're writing into the repository. In order to not leave behind any cruft in case we exit or receive a signal, we register both an exit handler as well as signal handlers for common signals like SIGINT. These handlers will then unlink the locks and free the data structure tracking them. We have observed a deadlock in this logic though:
(gdb) bt
#0 __lll_lock_wait_private () at ../sysdeps/unix/sysv/linux/x86_64/lowlevellock.S:95
#1 0x00007f4932bea2cd in _int_free (av=0x7f4932f2eb20 <main_arena>, p=0x3e3e4200, have_lock=0) at malloc.c:3969
#2 0x00007f4932bee58c in __GI___libc_free (mem=<optimized out>) at malloc.c:2975
#3 0x0000000000662ab1 in string_list_clear ()
#4 0x000000000044f5bc in unlock_pack_on_signal ()
#5 <signal handler called>
#6 _int_free (av=0x7f4932f2eb20 <main_arena>, p=<optimized out>, have_lock=0) at malloc.c:4024
#7 0x00007f4932bee58c in __GI___libc_free (mem=<optimized out>) at malloc.c:2975
#8 0x000000000065afd5 in strbuf_release ()
#9 0x000000000066ddb9 in delete_tempfile ()
#10 0x0000000000610d0b in files_transaction_cleanup.isra ()
#11 0x0000000000611718 in files_transaction_abort ()
#12 0x000000000060d2ef in ref_transaction_abort ()
#13 0x000000000060d441 in ref_transaction_prepare ()
#14 0x000000000060e0b5 in ref_transaction_commit ()
#15 0x00000000004511c2 in fetch_and_consume_refs ()
#16 0x000000000045279a in cmd_fetch ()
#17 0x0000000000407c48 in handle_builtin ()
#18 0x0000000000408df2 in cmd_main ()
#19 0x00000000004078b5 in main ()The process was killed with a signal, which caused the signal handler to kick in and try free the data structures after we have unlinked the locks. It then deadlocks while calling free(3P).
The root cause of this is that it is not allowed to call certain functions in async-signal handlers, as specified by signal-safety(7). Next to most I/O functions, this list of disallowed functions also includes memory-handling functions like malloc(3P) and free(3P) because they may not be reentrant. As a result, if we execute such functions in the signal handler, then they may operate on inconistent state and fail in unexpected ways.
Fix this bug by not calling non-async-signal-safe functions when running in the signal handler. We're about to re-raise the signal anyway and will thus exit, so it's not much of a problem to keep the string list of lockfiles untouched. Note that it's fine though to call unlink(2), so we'll still clean up the lockfiles correctly.
--- builtin/clone.c | 2 +- builtin/fetch.c | 17 +++++++++++------ transport.c | 11 ++++++++--- transport.h | 14 +++++++++++++- 4 files changed, 33 insertions(+), 11 deletions(-)
builtin/clone.c +1 -1
@@ -1290,7 +1290,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)*/ submodule_progress = transport->progress; -transport_unlock_pack (transport); +transport_unlock_pack (transport,0 );transport_disconnect (transport);if (option_dissociate) {
builtin/fetch.c +11 -6
@@ -223,17 +223,22 @@ static struct option builtin_fetch_options[] = { OPT_END() }; -static void unlock_pack (void ) +static void unlock_pack (unsigned int flags) {if (gtransport) -transport_unlock_pack (gtransport); +transport_unlock_pack (gtransport, flags);if (gsecondary) -transport_unlock_pack (gsecondary); +transport_unlock_pack (gsecondary, flags); +} + +static void unlock_pack_atexit (void ) +{ +unlock_pack (0 ); }static void unlock_pack_on_signal (int signo) { -unlock_pack (); +unlock_pack (TRANSPORT_UNLOCK_PACK_IN_SIGNAL_HANDLER );sigchain_pop (signo);raise (signo); }
@@ -1329,7 +1334,7 @@ static int fetch_and_consume_refs(struct transport *transport,trace2_region_leave ("fetch" ,"consume_refs" , the_repository); out: -transport_unlock_pack (transport); +transport_unlock_pack (transport,0 );return ret; }
@@ -1978,7 +1983,7 @@ static int fetch_one(struct remote *remote, int argc, const char **argv, gtransport->server_options = &server_options;sigchain_push_common (unlock_pack_on_signal); -atexit (unlock_pack); +atexit (unlock_pack_atexit);sigchain_push (SIGPIPE ,SIG_IGN ); exit_code =do_fetch (gtransport, &rs);sigchain_pop (SIGPIPE );
transport.c +8 -3
@@ -1456,13 +1456,18 @@ int transport_fetch_refs(struct transport *transport, struct ref *refs)return rc; } -void transport_unlock_pack (struct transport *transport) +void transport_unlock_pack (struct transport *transport,unsigned int flags) { +int in_signal_handler = !!(flags &TRANSPORT_UNLOCK_PACK_IN_SIGNAL_HANDLER );int i;for (i =0 ; i < transport->pack_lockfiles.nr; i++) -unlink_or_warn (transport->pack_lockfiles.items[i].string); -string_list_clear (&transport->pack_lockfiles,0 ); +if (in_signal_handler) +unlink (transport->pack_lockfiles.items[i].string); +else +unlink_or_warn (transport->pack_lockfiles.items[i].string); +if (!in_signal_handler) +string_list_clear (&transport->pack_lockfiles,0 ); } int transport_connect(struct transport *transport, const char *name,
transport.h +13 -1
@@ -279,7 +279,19 @@ const struct ref *transport_get_remote_refs(struct transport *transport,*/ const struct git_hash_algo *transport_get_hash_algo (struct transport *transport);int transport_fetch_refs (struct transport *transport,struct ref *refs); -void transport_unlock_pack (struct transport *transport); + +/* +* If this flag is set, unlocking will avoid to call non-async-signal-safe +* functions. This will necessarily leave behind some data structures which +* cannot be cleaned up. +*/ +#define TRANSPORT_UNLOCK_PACK_IN_SIGNAL_HANDLER (1 << 0) + +/* +* Unlock all packfiles locked by the transport. +*/ +void transport_unlock_pack (struct transport *transport,unsigned int flags); +int transport_disconnect (struct transport *transport);char *transport_anonymize_url (const char *url); void transport_take_over(struct transport *transport,
Signature · 1 line
2.34.1
Re: [PATCH 1/1] fetch: fix deadlock when cleaning up lockfiles in async signals
Quoted 43 lines · Patrick Steinhardt
[Resend with the correct In-Reply-To header set to fix threading]
When fetching packfiles, we write a bunch of lockfiles for the packfiles we're writing into the repository. In order to not leave behind any cruft in case we exit or receive a signal, we register both an exit handler as well as signal handlers for common signals like SIGINT. These handlers will then unlink the locks and free the data structure tracking them. We have observed a deadlock in this logic though:
(gdb) bt #0 __lll_lock_wait_private () at ../sysdeps/unix/sysv/linux/x86_64/lowlevellock.S:95 #1 0x00007f4932bea2cd in _int_free (av=0x7f4932f2eb20 <main_arena>, p=0x3e3e4200, have_lock=0) at malloc.c:3969 #2 0x00007f4932bee58c in __GI___libc_free (mem=<optimized out>) at malloc.c:2975 #3 0x0000000000662ab1 in string_list_clear () #4 0x000000000044f5bc in unlock_pack_on_signal () #5 <signal handler called> #6 _int_free (av=0x7f4932f2eb20 <main_arena>, p=<optimized out>, have_lock=0) at malloc.c:4024 #7 0x00007f4932bee58c in __GI___libc_free (mem=<optimized out>) at malloc.c:2975 #8 0x000000000065afd5 in strbuf_release () #9 0x000000000066ddb9 in delete_tempfile () #10 0x0000000000610d0b in files_transaction_cleanup.isra () #11 0x0000000000611718 in files_transaction_abort () #12 0x000000000060d2ef in ref_transaction_abort () #13 0x000000000060d441 in ref_transaction_prepare () #14 0x000000000060e0b5 in ref_transaction_commit () #15 0x00000000004511c2 in fetch_and_consume_refs () #16 0x000000000045279a in cmd_fetch () #17 0x0000000000407c48 in handle_builtin () #18 0x0000000000408df2 in cmd_main () #19 0x00000000004078b5 in main ()The process was killed with a signal, which caused the signal handler to kick in and try free the data structures after we have unlinked the locks. It then deadlocks while calling free(3P).
The root cause of this is that it is not allowed to call certain functions in async-signal handlers, as specified by signal-safety(7). Next to most I/O functions, this list of disallowed functions also includes memory-handling functions like malloc(3P) and free(3P) because they may not be reentrant. As a result, if we execute such functions in the signal handler, then they may operate on inconistent state and fail in unexpected ways.
Quoted 5 lines
Fix this bug by not calling non-async-signal-safe functions when running in the signal handler. We're about to re-raise the signal anyway and will thus exit, so it's not much of a problem to keep the string list of lockfiles untouched. Note that it's fine though to call unlink(2), so we'll still clean up the lockfiles correctly.
I took a look, and this seems reasonable to me. I know in the non-signal case, we'd want to clean up because it means we can check for leaks, but I don't see the utility of running Git under Valgrind and then sending it a signal, and I think it's just safe to ignore that case.
Signature · 2 lines
brian m. carlson (he/him or they/them) Toronto, Ontario, CA
Re: [PATCH 1/1] fetch: fix deadlock when cleaning up lockfiles in async signals
In reply to Patrick SteinhardtQuoted 11 lines · Patrick Steinhardt
transport.c +1
-1@@ -1456,13 +1456,18 @@ int transport_fetch_refs(struct transport *transport, struct ref *refs)return rc; } -void transport_unlock_pack (struct transport *transport) +void transport_unlock_pack (struct transport *transport,unsigned int flags) {
transport.c int transport_fetch_refs(struct transport *transport, struct ref *refs) · line 1468
Quoted 10 lines
transport.c +5
-2+int in_signal_handler = !!(flags &TRANSPORT_UNLOCK_PACK_IN_SIGNAL_HANDLER );int i;for (i =0 ; i < transport->pack_lockfiles.nr; i++) -unlink_or_warn (transport->pack_lockfiles.items[i].string); -string_list_clear (&transport->pack_lockfiles,0 ); +if (in_signal_handler) +unlink (transport->pack_lockfiles.items[i].string); +else +unlink_or_warn (transport->pack_lockfiles.items[i].string);
This puzzled me when I first read it. But unlink_or_warn() isn't reentrant because it performs buffered IO on stderr, so if we reached this signal handler while executing another function call modifying those same buffers, the call within the signal handler would have undefined behavior.
So that makes sense: freeing (with string_list_clear() below) and performing buffered IO (with unlink_or_warn() here as just described) are certainly off the table.
But is unlink() safe as-is? I'm not so sure. Reading signal-safety(7), it's clearly on the list of functions that are OK to call. But reading the errno section:
errno
Fetching and setting the value of errno is async-signal-safe provided that the signal handler saves errno on entry and restores its value before returning.
We certainly not doing that, though that's nothing new, and so I wonder why it doesn't seem to be an issue in practice.
Thanks,
Taylor
Re: [PATCH 1/1] fetch: fix deadlock when cleaning up lockfiles in async signals
Hi Taylor
Quoted 37 lines · Taylor Blau
Quoted 11 lines · Patrick Steinhardt
transport.c +1
-1@@ -1456,13 +1456,18 @@ int transport_fetch_refs(struct transport *transport, struct ref *refs)return rc; } -void transport_unlock_pack (struct transport *transport) +void transport_unlock_pack (struct transport *transport,unsigned int flags) {transport.c int transport_fetch_refs(struct transport *transport, struct ref *refs) · line 1468
Quoted 10 lines
transport.c +5
-2+int in_signal_handler = !!(flags &TRANSPORT_UNLOCK_PACK_IN_SIGNAL_HANDLER );int i;for (i =0 ; i < transport->pack_lockfiles.nr; i++) -unlink_or_warn (transport->pack_lockfiles.items[i].string); -string_list_clear (&transport->pack_lockfiles,0 ); +if (in_signal_handler) +unlink (transport->pack_lockfiles.items[i].string); +else +unlink_or_warn (transport->pack_lockfiles.items[i].string);This puzzled me when I first read it. But
unlink_or_warn()isn't reentrant because it performs buffered IO on stderr, so if we reached this signal handler while executing another function call modifying those same buffers, the call within the signal handler would have undefined behavior.So that makes sense: freeing (with
string_list_clear()below) and performing buffered IO (withunlink_or_warn()here as just described) are certainly off the table.But is
unlink()safe as-is? I'm not so sure. Reading signal-safety(7), it's clearly on the list of functions that are OK to call. But reading the errno section:
Quoted 7 lines
errno
Fetching and setting the value of errno is async-signal-safe provided that the signal handler saves errno on entry and restores its value before returning.We certainly not doing that, though that's nothing new, and so I wonder why it doesn't seem to be an issue in practice.
Because in this case we re-raise the signal and exit it does not matter if unlink() clobbers errno. If instead the program were to continue after handling the signal then we would have to save and restore errno to avoid interfering with the code that was running when the signal handler was called.
Best Wishes
Phillip
Quoted 2 lines
Thanks,
Taylor
Re: [PATCH 1/1] fetch: fix deadlock when cleaning up lockfiles in async signals
Quoted 6 lines · Phillip Wood
Quoted 6 lines
But is
unlink()safe as-is? I'm not so sure. Reading signal-safety(7), it's clearly on the list of functions that are OK to call. But reading the errno section:(snip)
Quoted 8 lines
Quoted 2 lines
We certainly not doing that, though that's nothing new, and so I wonder why it doesn't seem to be an issue in practice.
Because in this case we re-raise the signal and exit it does not matter if
unlink()clobbers errno. If instead the program were to continue after handling the signal then we would have to save and restore errno to avoid interfering with the code that was running when the signal handler was called.
That makes perfect sense to me. Thanks for a clear explanation.
Taylor