Hi Peff,
On Wed, 24 Oct 2018, Jeff King wrote:
> Since commit e3a434468f (run-command: use the
> async-signal-safe execv instead of execvp, 2017-04-19),
> prepare_cmd() does its own PATH lookup for any commands we
> run (on non-Windows platforms).
>
> However, its logic does not match the old execvp call when
> we fail to find a matching entry in the PATH. Instead of
> feeding the name directly to execv, execvp would consider
> that an ENOENT error. By continuing and passing the name
> directly to execv, we effectively behave as if "." was
> included at the end of the PATH. This can have confusing and
> even dangerous results.
For the record, I tried to come up with an attack vector to exploit this,
and failed to find one.
> The fix itself is pretty straight-forward. There's a new
> test in t0061 to cover this explicitly, and I've also added
> a duplicate of the ENOENT test to ensure that we return the
> correct errno for this case.
>
> Signed-off-by: Jeff King <p...@peff.net>
> ---
> run-command.c | 20 ++++++++++++++++----
> t/t0061-run-command.sh | 13 ++++++++++++-
> 2 files changed, 28 insertions(+), 5 deletions(-)
>
> diff --git a/run-command.c b/run-command.c
> index 84b883c213..639ea5ac33 100644
> --- a/run-command.c
> +++ b/run-command.c
> @@ -380,7 +380,7 @@ static void child_err_spew(struct child_process *cmd,
> struct child_err *cerr)
> set_error_routine(old_errfn);
> }
>
> -static void prepare_cmd(struct argv_array *out, const struct child_process
> *cmd)
> +static int prepare_cmd(struct argv_array *out, const struct child_process
> *cmd)
I always like when we change functions to return a value that can then
indicate an error, making the libification effort so much easier.
> {
> if (!cmd->argv[0])
> BUG("command is empty");
> @@ -403,16 +403,22 @@ static void prepare_cmd(struct argv_array *out, const
> struct child_process *cmd)
> /*
> * 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.
> + * are '/' characters, we have exec attempt to invoke the command
> + * directly.
Nice. I would have probably forgotten about that comment.
> */
> if (!strchr(out->argv[1], '/')) {
> char *program = locate_in_PATH(out->argv[1]);
> if (program) {
> free((char *)out->argv[1]);
> out->argv[1] = program;
> + } else {
> + argv_array_clear(out);
> + errno = ENOENT;
> + return -1;
> }
> }
> +
> + return 0;
> }
>
> static char **prep_childenv(const char *const *deltaenv)
> @@ -719,6 +725,12 @@ int start_command(struct child_process *cmd)
> struct child_err cerr;
> struct atfork_state as;
>
> + if (prepare_cmd(&argv, cmd) < 0) {
> + failed_errno = errno;
> + cmd->pid = -1;
> + goto end_of_spawn;
> + }
> +
> if (pipe(notify_pipe))
> notify_pipe[0] = notify_pipe[1] = -1;
>
> @@ -729,7 +741,6 @@ int start_command(struct child_process *cmd)
> set_cloexec(null_fd);
> }
>
> - prepare_cmd(&argv, cmd);
> childenv = prep_childenv(cmd->env);
> atfork_prepare(&as);
>
> @@ -910,6 +921,7 @@ int start_command(struct child_process *cmd)
> }
> #endif
>
> +end_of_spawn:
Sadly, this fails to build on Windows:
run-command.c: In function 'start_command':
run-command.c:924:1: error: label 'end_of_spawn' defined but not used
[-Werror=unused-label]
end_of_spawn:
^~~~~~~~~~~~
How about squashing in this diff:
-- snip --
diff --git a/run-command.c b/run-command.c
index 639ea5ac3366..3f03795a5995 100644
--- a/run-command.c
+++ b/run-command.c
@@ -918,6 +918,8 @@ int start_command(struct child_process *cmd)
close(fhout);
if (fherr != 2)
close(fherr);
+
+ goto end_of_spawn;
}
#endif
-- snap --
I can confirm that the result compiles and passes t0061.
Thanks,
Dscho
> if (cmd->pid < 0) {
> if (need_in)
> close_pair(fdin);
> diff --git a/t/t0061-run-command.sh b/t/t0061-run-command.sh
> index 3e131c5325..cf932c8514 100755
> --- a/t/t0061-run-command.sh
> +++ b/t/t0061-run-command.sh
> @@ -12,10 +12,14 @@ cat >hello-script <<-EOF
> cat hello-script
> EOF
>
> -test_expect_success 'start_command reports ENOENT' '
> +test_expect_success 'start_command reports ENOENT (slash)' '
> test-tool run-command start-command-ENOENT ./does-not-exist
> '
>
> +test_expect_success 'start_command reports ENOENT (no slash)' '
> + test-tool run-command start-command-ENOENT does-not-exist
> +'
> +
> test_expect_success 'run_command can run a command' '
> cat hello-script >hello.sh &&
> chmod +x hello.sh &&
> @@ -25,6 +29,13 @@ test_expect_success 'run_command can run a command' '
> test_must_be_empty err
> '
>
> +test_expect_success 'run_command is restricted to PATH' '
> + write_script should-not-run <<-\EOF &&
> + echo yikes
> + EOF
> + test_must_fail test-tool run-command run-command should-not-run
> +'
> +
> test_expect_success !MINGW 'run_command can run a script without a #! line' '
> cat >hello <<-\EOF &&
> cat hello-script
> --
> 2.19.1.1094.gd480080bf6
>
