Jonathan Nieder [off-list ref] wrote:

Hi,

Brandon Williams wrote:

quoted

--- a/run-command.c
+++ b/run-command.c

@@ -458,6 +458,14 @@ int start_command(struct child_process *cmd)
 		argv_array_pushv(&argv, cmd->argv);
 	}
 
+	/*
+	 * NOTE: In order to prevent deadlocking when using threads special
+	 * care should be taken with the function calls made in between the
+	 * fork() and exec() calls.  No calls should be made to functions which
+	 * require acquiring a lock (e.g. malloc) as the lock could have been
+	 * held by another thread at the time of forking, causing the lock to
+	 * never be released in the child process.
+	 */
 	cmd->pid = fork();

Why can't git use e.g. posix_spawn to avoid this?

posix_spawn does not support chdir, and it seems we run non-git
commands so no using "git -C" for those.

fork()-ing in a threaded context is very painful for maintainability.
Any library function you are using could start taking a lock, and then
you have a deadlock.  So you have to make use of a very small
whitelisted list of library functions for this to work.

Completely agreed.

On the other hand, I believe we should make run-command
vfork-compatible (and Brandon's series is a big (but incomplete)
step in the (IMHO) right direction); as anything which is
vfork-safe would also be safe in the presence of threads+(plain) fork.
With vfork; the two processes share heap until execve.

I posted some notes about it last year:

  https://public-inbox.org/git/20160629200142.GA17878@dcvr.yhbt.net/

The function calls you have to audit are not only between fork() and
exec() in the normal control flow.  You have to worry about signal
handlers, too.

Yes, all that auditing is necessary for vfork; too, but totally
doable.  The mainline Ruby implementation has been using vfork
for spawning subprocesses for several years, now; and I think the
ruby-core developers (myself included) have fixed all the
problems with it; even in multi-threaded code which calls malloc.