Re: [PATCH 2/2] run-command: mark path lookup errors with ENOENT
- From
Johannes Schindelin <johannes.schindelin@gmx.de>
- Date
- Oct 24, 2018, 09:01 UTC
- Message-ID
- <nycvar.QRO.7.76.6.1810241054110.4546@tvgsbejvaqbjf.bet>
- In-Reply-To
- <20181024073800.GB31202@sigill.intra.peff.net>
Hi Peff,
On Wed, 24 Oct 2018, Jeff King wrote:
Show 12 quoted lines
> 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.
Show 21 quoted lines
> 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 <peff@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.
Show 11 quoted lines
> {
> 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.
Show 43 quoted lines
> */
> 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 >