Re: [PATCH v2 2/6] run-command: prepare command before forking

[Brandon Williams <bmwill@google.com>]

On 04/13, Jonathan Nieder wrote:
> Hi,
> 
> Brandon Williams wrote:
> 
> > In order to avoid allocation between 'fork()' and 'exec()' the argv
> > array used in the exec call is prepared prior to forking the process.
> 
> nit: s/(the argv array.*) is prepared/prepare \1/
> 
> Git's commit messages are in the imperative mood, as if they are
> ordering the code or the computer to do something.
> 
> More importantly, the commit message is a good place to explain some
> of the motivation behind the patch so that people can understand what
> the patch is for by reading it without having to dig into mailing list
> archives and get the discussion there.
> 
> E.g. this could say
> 
> - that grep tests are intermittently failing in configurations using
>   some versions of tcmalloc
> 
> - that the cause is interaction between fork and threads: malloc holds
>   a lock that those versions of tcmalloc doesn't release in a
>   pthread_atfork handler
> 
> - that according to [1] we need to only call async-signal-safe
>   operations between fork and exec.  Using malloc to build the argv
>   array isn't async-signal-safe
> 
> [1] http://pubs.opengroup.org/onlinepubs/009695399/functions/fork.html
> 
> > In addition to this, the function used to exec is changed from
> > 'execvp()' to 'execv()' as the (p) variant of exec has the potential to
> > call malloc during the path resolution it performs.
> 
> *puzzled* is execvp actually allowed to call malloc?

It could possible as it isn't async-signal-safe.

> 
> Could this part go in a separate patch?  That would make it easier to
> review.

I'll break this conversion out to a different patch.

> 
> [...]
> > +++ b/run-command.c
> [...]
> > +	/*
> > +	 * If there are no '/' characters in the command then perform a path
> > +	 * lookup and use the resolved path as the command to exec.  If there
> > +	 * are no '/' characters or if the command wasn't found in the path,
> > +	 * have exec attempt to invoke the command directly.
> > +	 */
> > +	if (!strchr(out->argv[0], '/')) {
> > +		char *program = locate_in_PATH(out->argv[0]);
> > +		if (program) {
> > +			free((char *)out->argv[0]);
> > +			out->argv[0] = program;
> > +		}
> > +	}
> 
> This does one half of what execvp does but leaves out the other half.
> http://pubs.opengroup.org/onlinepubs/009695399/functions/exec.html
> explains:
> 
>   There are two distinct ways in which the contents of the process
>   image file may cause the execution to fail, distinguished by the
>   setting of errno to either [ENOEXEC] or [EINVAL] (see the ERRORS
>   section). In the cases where the other members of the exec family of
>   functions would fail and set errno to [ENOEXEC], the execlp() and
>   execvp() functions shall execute a command interpreter and the
>   environment of the executed command shall be as if the process
>   invoked the sh utility using execl() as follows:
> 
>   execl(<shell path>, arg0, file, arg1, ..., (char *)0);
> 
> I think this is what the first patch in the series was about.  Do we
> want to drop that support?
> 
> I think we need to keep it, since it is easy for authors of e.g.
> credential helpers to accidentally rely on it.
> 
> [...]
> > @@ -437,12 +458,9 @@ int start_command(struct child_process *cmd)
> >  					unsetenv(*cmd->env);
> >  			}
> >  		}
> > -		if (cmd->git_cmd)
> > -			execv_git_cmd(cmd->argv);
> > -		else if (cmd->use_shell)
> > -			execv_shell_cmd(cmd->argv);
> > -		else
> > -			sane_execvp(cmd->argv[0], (char *const*) cmd->argv);
> > +
> > +		execv(argv.argv[0], (char *const *) argv.argv);
> 
> What happens in the case sane_execvp was trying to handle?  Does
> prepare_cmd need error handling for when the command isn't found?
> 
> Sorry this got so fussy.
> 
> Thanks and hope that helps,
> Jonathan

-- 
Brandon Williams