From: "brian m. carlson" <sandals@crustytoothpaste•net>
To: Patrick Steinhardt <ps@pks•im>
Cc: git@vger•kernel.org, iwiedler@gitlab•com
Subject: Re: [PATCH 1/1] fetch: fix deadlock when cleaning up lockfiles in async signals
Date: Fri, 7 Jan 2022 11:14:37 +0000 [thread overview]
Message-ID: <YdggneOlWXf2Z+gf@camp.crustytoothpaste.net> (raw)
In-Reply-To: <555ec6717ecab0fe6ef5660bcf0d61d59f84ef8b.1641552500.git.ps@pks.im>
[-- Attachment #1: Type: text/plain, Size: 3235 bytes --]
On 2022-01-07 at 10:55:47, Patrick Steinhardt wrote:
> [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.
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.
--
brian m. carlson (he/him or they/them)
Toronto, Ontario, CA
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 262 bytes --]
next prev parent reply other threads:[~2022-01-07 11:14 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-01-07 10:53 [PATCH 0/1] Async-signal safety in signal handlers Patrick Steinhardt
2022-01-07 10:55 ` [PATCH 1/1] fetch: fix deadlock when cleaning up lockfiles in async signals Patrick Steinhardt
2022-01-07 11:14 ` brian m. carlson [this message]
2022-01-07 22:41 ` Taylor Blau
2022-01-08 10:54 ` Phillip Wood
2022-01-11 2:11 ` Taylor Blau
[not found] <cover.1641551066.git.ps@pks.im>
2022-01-07 10:53 ` Patrick Steinhardt
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=YdggneOlWXf2Z+gf@camp.crustytoothpaste.net \
--to=sandals@crustytoothpaste$(echo .)net \
--cc=git@vger$(echo .)kernel.org \
--cc=iwiedler@gitlab$(echo .)com \
--cc=ps@pks$(echo .)im \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox