Merge branch 'ps/lockfile-cleanup-fix'

Some lockfile code called free() in signal-death code path, which
has been corrected.

* ps/lockfile-cleanup-fix:
  fetch: fix deadlock when cleaning up lockfiles in async signals

4 files changed, 33 insertions, 11 deletions

diff --git a/builtin/clone.c b/builtin/clone.c
index 13d230d299..727e16e0ae 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 eaab8056bf..5f06b21f8e 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,