Re: [PATCH v2 2/6] run-command: prepare command before forking
On 04/13, Jonathan Nieder wrote:
Show 37 quoted lines
> 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.
Show 60 quoted lines
>
> [...]
> > +++ 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