On 04/25, Jonathan Nieder wrote:
> Brandon Williams wrote:
>
> > Subject: run-command: don't try to execute directories
>
> nit: this is also about non-executable files, now. That would mean
> something like
>
> run-command: don't try to execute directories or non-executable files
>
> or
>
> run-command: restrict PATH search to files we can execute
>
> [...]
> > Reported-by: Brian Hatfield <bhatfi...@google.com>
> > Signed-off-by: Brandon Williams <bmw...@google.com>
> > ---
> > run-command.c | 2 +-
> > t/t0061-run-command.sh | 23 +++++++++++++++++++++++
> > 2 files changed, 24 insertions(+), 1 deletion(-)
> >
> > diff --git a/run-command.c b/run-command.c
> > index a97d7bf9f..ec08e0951 100644
> > --- a/run-command.c
> > +++ b/run-command.c
> > @@ -137,7 +137,7 @@ static char *locate_in_PATH(const char *file)
> > }
> > strbuf_addstr(&buf, file);
> >
> > - if (!access(buf.buf, F_OK))
> > + if (is_executable(buf.buf))
> > return strbuf_detach(&buf, NULL);
>
> It's probably worth a docstring for this function to explain
> that this is not a complete emulation of execvp on Windows, since
> it doesn't look for .com and .exe files.
>
>
> >
> > if (!*end)
> > diff --git a/t/t0061-run-command.sh b/t/t0061-run-command.sh
> > index 98c09dd98..fd5e43766 100755
> > --- a/t/t0061-run-command.sh
> > +++ b/t/t0061-run-command.sh
> > @@ -37,6 +37,29 @@ test_expect_success !MINGW 'run_command can run a script
> > without a #! line' '
> > test_cmp empty err
> > '
> >
> > +test_expect_success 'run_command should not try to execute a directory' '
> > + test_when_finished "rm -rf bin1 bin2 bin3" &&
> > + mkdir -p bin1/greet bin2 bin3 &&
> > + write_script bin2/greet <<-\EOF &&
> > + cat bin2/greet
> > + EOF
> > + chmod -x bin2/greet &&
>
> This probably implies that the test needs a POSIXPERM dependency.
> Should it be a separate test_expect_success case so that the other
> part can still run on Windows?
>
> The rest looks good. Thanks for your patient work.
>
> With whatever subset of the changes described makes sense,
> Reviewed-by: Jonathan Nieder <jrnie...@gmail.com>
>
> Thanks.
>
> diff --git i/run-command.c w/run-command.c
> index ec08e09518..dbbaec932e 100644
> --- i/run-command.c
> +++ w/run-command.c
> @@ -117,6 +117,21 @@ static inline void close_pair(int fd[2])
> close(fd[1]);
> }
>
> +/*
> + * Search $PATH for a command. This emulates the path search that
> + * execvp would perform, without actually executing the command so it
> + * can be used before fork() to prepare to run a command using
> + * execve() or after execvp() to diagnose why it failed.
> + *
> + * The caller should ensure that file contains no directory
> + * separators.
> + *
> + * Returns NULL if the command could not be found.
> + *
> + * This should not be used on Windows, where the $PATH search rules
> + * are more complicated (e.g., a search for "foo" should find
> + * "foo.exe").
> + */
> static char *locate_in_PATH(const char *file)
> {
> const char *p = getenv("PATH");
> diff --git i/t/t0061-run-command.sh w/t/t0061-run-command.sh
> index fd5e43766a..e48a207fae 100755
> --- i/t/t0061-run-command.sh
> +++ w/t/t0061-run-command.sh
> @@ -37,26 +37,33 @@ test_expect_success !MINGW 'run_command can run a script
> without a #! line' '
> test_cmp empty err
> '
>
> -test_expect_success 'run_command should not try to execute a directory' '
> +test_expect_success 'run_command does not try to execute a directory' '
> test_when_finished "rm -rf bin1 bin2" &&
> - mkdir -p bin1/greet bin2 bin3 &&
> + mkdir -p bin1/greet bin2 &&
> write_script bin2/greet <<-\EOF &&
> cat bin2/greet
> EOF
> - chmod -x bin2/greet &&
> - write_script bin3/greet <<-\EOF &&
> - cat bin3/greet
> +
> + PATH=$PWD/bin1:$PWD/bin2:$PATH \
> + test-run-command run-command greet >actual 2>err &&
> + test_cmp bin2/greet actual &&
> + test_cmp empty err
> +'
> +
> +test_expect_success POSIXPERM 'run_command passes over non-executable file' '
> + test_when_finished "rm -rf bin1 bin2" &&
> + mkdir -p bin1 bin2 &&
> + write_script bin1/greet <<-\EOF &&
> + cat bin1/greet
> + EOF
> + chmod -x bin1/greet &&
> + write_script bin2/greet <<-\EOF &&
> + cat bin2/greet
> EOF
>
> - # Test that run-command does not try to execute the "greet" directory in
> - # "bin1", or the non-executable file "greet" in "bin2", but rather
> - # correcty executes the "greet" script located in bin3.
> - (
> - PATH=$PWD/bin1:$PWD/bin2:$PWD/bin3:$PATH &&
> - export PATH &&
> - test-run-command run-command greet >actual 2>err
> - ) &&
> - test_cmp bin3/greet actual &&
> + PATH=$PWD/bin1:$PWD/bin2:$PATH \
> + test-run-command run-command greet >actual 2>err &&
> + test_cmp bin2/greet actual &&
> test_cmp empty err
> '
>
Yeah the POSIXPERM is going to be necessary. I'll roll in some of these
changes when I do a reroll.
--
Brandon Williams
