Jonathan Nieder <jrnieder@xxxxxxxxx> wrote: > Hi, > > Brandon Williams wrote: > > > --- 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@xxxxxxxxxxxxx/ > 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.