Re: [PATCH 5/5] run-command: add note about forking and threading

[Eric Wong <e@80x24.org>]

Jonathan Nieder <jrnieder@gmail.com> 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@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.