fetch: fix deadlock when cleaning up lockfiles in async signals

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. Signed-off-by: Patrick Steinhardt <ps@pks.im> Reviewed-by: brian m. carlson <sandals@crustytoothpaste.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Patrick Steinhardt committed Jan 7, 2022 at 11:55 UTC 58d4d7f1c5a665111f05c61901a11a555703fe11
4 files changed +33 -11
builtin/clone.c
+1 -1
@@ -1290,7 +1290,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
1290 */
1291 submodule_progress = transport->progress;
1292
1293 - transport_unlock_pack(transport);
1293 + transport_unlock_pack(transport, 0);
1294 transport_disconnect(transport);
1295
1296 if (option_dissociate) {
builtin/fetch.c
+11 -6
@@ -222,17 +222,22 @@ static struct option builtin_fetch_options[] = {
222 OPT_END()
223 };
224
225 -static void unlock_pack(void)
225 +static void unlock_pack(unsigned int flags)
226 {
227 if (gtransport)
228 - transport_unlock_pack(gtransport);
228 + transport_unlock_pack(gtransport, flags);
229 if (gsecondary)
230 - transport_unlock_pack(gsecondary);
230 + transport_unlock_pack(gsecondary, flags);
231 +}
232 +
233 +static void unlock_pack_atexit(void)
234 +{
235 + unlock_pack(0);
236 }
237
238 static void unlock_pack_on_signal(int signo)
239 {
235 - unlock_pack();
240 + unlock_pack(TRANSPORT_UNLOCK_PACK_IN_SIGNAL_HANDLER);
241 sigchain_pop(signo);
242 raise(signo);
243 }
@@ -1326,7 +1331,7 @@ static int fetch_and_consume_refs(struct transport *transport, struct ref *ref_m
1331 trace2_region_leave("fetch", "consume_refs", the_repository);
1332
1333 out:
1329 - transport_unlock_pack(transport);
1334 + transport_unlock_pack(transport, 0);
1335 return ret;
1336 }
1337
@@ -1962,7 +1967,7 @@ static int fetch_one(struct remote *remote, int argc, const char **argv,
1967 gtransport->server_options = &server_options;
1968
1969 sigchain_push_common(unlock_pack_on_signal);
1965 - atexit(unlock_pack);
1970 + atexit(unlock_pack_atexit);
1971 sigchain_push(SIGPIPE, SIG_IGN);
1972 exit_code = do_fetch(gtransport, &rs);
1973 sigchain_pop(SIGPIPE);
transport.c
+8 -3
@@ -1457,13 +1457,18 @@ int transport_fetch_refs(struct transport *transport, struct ref *refs)
1457 return rc;
1458 }
1459
1460 -void transport_unlock_pack(struct transport *transport)
1460 +void transport_unlock_pack(struct transport *transport, unsigned int flags)
1461 {
1462 + int in_signal_handler = !!(flags & TRANSPORT_UNLOCK_PACK_IN_SIGNAL_HANDLER);
1463 int i;
1464
1465 for (i = 0; i < transport->pack_lockfiles.nr; i++)
1465 - unlink_or_warn(transport->pack_lockfiles.items[i].string);
1466 - string_list_clear(&transport->pack_lockfiles, 0);
1466 + if (in_signal_handler)
1467 + unlink(transport->pack_lockfiles.items[i].string);
1468 + else
1469 + unlink_or_warn(transport->pack_lockfiles.items[i].string);
1470 + if (!in_signal_handler)
1471 + string_list_clear(&transport->pack_lockfiles, 0);
1472 }
1473
1474 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,
279 */
280 const struct git_hash_algo *transport_get_hash_algo(struct transport *transport);
281 int transport_fetch_refs(struct transport *transport, struct ref *refs);
282 -void transport_unlock_pack(struct transport *transport);
282 +
283 +/*
284 + * If this flag is set, unlocking will avoid to call non-async-signal-safe
285 + * functions. This will necessarily leave behind some data structures which
286 + * cannot be cleaned up.
287 + */
288 +#define TRANSPORT_UNLOCK_PACK_IN_SIGNAL_HANDLER (1 << 0)
289 +
290 +/*
291 + * Unlock all packfiles locked by the transport.
292 + */
293 +void transport_unlock_pack(struct transport *transport, unsigned int flags);
294 +
295 int transport_disconnect(struct transport *transport);
296 char *transport_anonymize_url(const char *url);
297 void transport_take_over(struct transport *transport,