git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 1/1] fetch: fix deadlock when cleaning up lockfiles in async signals
       [not found] <cover.1641551066.git.ps@pks.im>
@ 2022-01-07 10:53 ` Patrick Steinhardt
  0 siblings, 0 replies; 6+ messages in thread
From: Patrick Steinhardt @ 2022-01-07 10:53 UTC (permalink / raw)
  To: git

[-- Attachment #1: Type: text/plain, Size: 6464 bytes --]

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 just not free'ing memory 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>
---
 builtin/clone.c |  2 +-
 builtin/fetch.c | 17 +++++++++++------
 transport.c     |  5 +++--
 transport.h     | 14 +++++++++++++-
 4 files changed, 28 insertions(+), 10 deletions(-)

diff --git a/builtin/clone.c b/builtin/clone.c
index 5bed37f8b5..585eef9b9a 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -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) {
diff --git a/builtin/fetch.c b/builtin/fetch.c
index f1fe73a3e0..2c7a644d69 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -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_SKIP_FREE);
 	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);
diff --git a/transport.c b/transport.c
index 92ab9a3fa6..f636163d05 100644
--- a/transport.c
+++ b/transport.c
@@ -1456,13 +1456,14 @@ 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 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 ((flags & TRANSPORT_UNLOCK_PACK_SKIP_FREE) == 0)
+		string_list_clear(&transport->pack_lockfiles, 0);
 }
 
 int transport_connect(struct transport *transport, const char *name,
diff --git a/transport.h b/transport.h
index 8bb4c8bbc8..8949ea9d04 100644
--- a/transport.h
+++ b/transport.h
@@ -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, then the data structures tracking the locked packfiles
+ * will not be free'd on unlock. This flag should only be passed when executed
+ * in a signal handler where free(3P) cannot be relied upon.
+ */
+#define TRANSPORT_UNLOCK_PACK_SKIP_FREE (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,
-- 
2.34.1


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH 1/1] fetch: fix deadlock when cleaning up lockfiles in async signals
  2022-01-08 10:54     ` Phillip Wood
@ 2022-01-11  2:11       ` Taylor Blau
  0 siblings, 0 replies; 6+ messages in thread
From: Taylor Blau @ 2022-01-11  2:11 UTC (permalink / raw)
  To: phillip.wood; +Cc: Taylor Blau, Patrick Steinhardt, git, iwiedler

On Sat, Jan 08, 2022 at 10:54:35AM +0000, Phillip Wood wrote:
> > 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)
> >
> > 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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH 1/1] fetch: fix deadlock when cleaning up lockfiles in async signals
  2022-01-07 22:41   ` Taylor Blau
@ 2022-01-08 10:54     ` Phillip Wood
  2022-01-11  2:11       ` Taylor Blau
  0 siblings, 1 reply; 6+ messages in thread
From: Phillip Wood @ 2022-01-08 10:54 UTC (permalink / raw)
  To: Taylor Blau, Patrick Steinhardt; +Cc: git, iwiedler

Hi Taylor

On 07/01/2022 22:41, Taylor Blau wrote:
> On Fri, Jan 07, 2022 at 11:55:47AM +0100, Patrick Steinhardt wrote:
>> diff --git a/transport.c b/transport.c
>> index 92ab9a3fa6..2a3e324154 100644
>> --- a/transport.c
>> +++ b/transport.c
>> @@ -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);
> 
> 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.

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

> Thanks,
> Taylor

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH 1/1] fetch: fix deadlock when cleaning up lockfiles in async signals
  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
@ 2022-01-07 22:41   ` Taylor Blau
  2022-01-08 10:54     ` Phillip Wood
  1 sibling, 1 reply; 6+ messages in thread
From: Taylor Blau @ 2022-01-07 22:41 UTC (permalink / raw)
  To: Patrick Steinhardt; +Cc: git, iwiedler

On Fri, Jan 07, 2022 at 11:55:47AM +0100, Patrick Steinhardt wrote:
> diff --git a/transport.c b/transport.c
> index 92ab9a3fa6..2a3e324154 100644
> --- a/transport.c
> +++ b/transport.c
> @@ -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);

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH 1/1] fetch: fix deadlock when cleaning up lockfiles in async signals
  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
  2022-01-07 22:41   ` Taylor Blau
  1 sibling, 0 replies; 6+ messages in thread
From: brian m. carlson @ 2022-01-07 11:14 UTC (permalink / raw)
  To: Patrick Steinhardt; +Cc: git, iwiedler

[-- 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 --]

^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH 1/1] fetch: fix deadlock when cleaning up lockfiles in async signals
  2022-01-07 10:53 [PATCH 0/1] Async-signal safety in signal handlers Patrick Steinhardt
@ 2022-01-07 10:55 ` Patrick Steinhardt
  2022-01-07 11:14   ` brian m. carlson
  2022-01-07 22:41   ` Taylor Blau
  0 siblings, 2 replies; 6+ messages in thread
From: Patrick Steinhardt @ 2022-01-07 10:55 UTC (permalink / raw)
  To: git; +Cc: iwiedler

[-- Attachment #1: Type: text/plain, Size: 6739 bytes --]

[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.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/clone.c |  2 +-
 builtin/fetch.c | 17 +++++++++++------
 transport.c     | 11 ++++++++---
 transport.h     | 14 +++++++++++++-
 4 files changed, 33 insertions(+), 11 deletions(-)

diff --git a/builtin/clone.c b/builtin/clone.c
index 5bed37f8b5..585eef9b9a 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -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) {
diff --git a/builtin/fetch.c b/builtin/fetch.c
index f1fe73a3e0..4bc04522ed 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -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);
diff --git a/transport.c b/transport.c
index 92ab9a3fa6..2a3e324154 100644
--- a/transport.c
+++ b/transport.c
@@ -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,
diff --git a/transport.h b/transport.h
index 8bb4c8bbc8..3f16e50c19 100644
--- a/transport.h
+++ b/transport.h
@@ -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,
-- 
2.34.1


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply related	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2022-01-11  2:11 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <cover.1641551066.git.ps@pks.im>
2022-01-07 10:53 ` [PATCH 1/1] fetch: fix deadlock when cleaning up lockfiles in async signals Patrick Steinhardt
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
2022-01-07 22:41   ` Taylor Blau
2022-01-08 10:54     ` Phillip Wood
2022-01-11  2:11       ` Taylor Blau

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).