Re: [PATCH v7 2/2] run-command: don't try to execute directories

[Brandon Williams <bmwill@google.com>]

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 <bhatfield@google.com>
> > Signed-off-by: Brandon Williams <bmwill@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 <jrnieder@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