[PATCH] git submodule foreach: Skip eval for more than one argument

‘eval "$@"’ created an extra layer of shell interpretation, which was
probably not expected by a user who passed multiple arguments to git
submodule foreach:

$ git grep "'"
[searches for single quotes]
$ git submodule foreach git grep "'"
Entering '[submodule]'
/usr/lib/git-core/git-submodule: 1: eval: Syntax error: Unterminated quoted string
Stopping at '[submodule]'; script returned non-zero status.

To fix this, if the user passed more than one argument, just execute
"$@" directly instead of passing it to eval.

Signed-off-by: Anders Kaseorg <redacted>
---
 git-submodule.sh | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/git-submodule.sh b/git-submodule.sh
index c17bef1..3381864 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh

@@ -545,7 +545,12 @@ cmd_foreach()
 				sm_path=$(relative_path "$sm_path") &&
 				# we make $path available to scripts ...
 				path=$sm_path &&
-				eval "$@" &&
+				if [ $# -eq 1 ]
+				then
+					eval "$1"
+				else
+					"$@"
+				fi &&
 				if test -n "$recursive"
 				then
 					cmd_foreach "--recursive" "$@"

-- 
1.8.4

On Thu, Sep 26, 2013 at 10:10 PM, Anders Kaseorg [off-list ref] wrote:

‘eval "$@"’ created an extra layer of shell interpretation, which was
probably not expected by a user who passed multiple arguments to git
submodule foreach:

$ git grep "'"
[searches for single quotes]
$ git submodule foreach git grep "'"
Entering '[submodule]'
/usr/lib/git-core/git-submodule: 1: eval: Syntax error: Unterminated quoted string
Stopping at '[submodule]'; script returned non-zero status.

To fix this, if the user passed more than one argument, just execute
"$@" directly instead of passing it to eval.

Signed-off-by: Anders Kaseorg <redacted>

The change looks good, and the existing tests (in t7407) pass. :-)

Two comments, however:

1. Please add the use case you mention above as a new test case, so
that we can easily catch future regressions.

2. If we are unlucky there might be existing users that work around
the existing behavior by adding an extra level of quoting (i.e. doing
the equivalent of git submodule foreach git grep "\'" in your example
above). Will their workaround break as a result of your change? Is
that acceptable?


Have fun! :)

...Johan

-- 
Johan Herland, [off-list ref]
www.herland.net

‘eval "$@"’ created an extra layer of shell interpretation, which was
probably not expected by a user who passed multiple arguments to git
submodule foreach:

$ git grep "'"
[searches for single quotes]
$ git submodule foreach git grep "'"
Entering '[submodule]'
/usr/lib/git-core/git-submodule: 1: eval: Syntax error: Unterminated quoted string
Stopping at '[submodule]'; script returned non-zero status.

To fix this, if the user passed more than one argument, just execute
"$@" directly instead of passing it to eval.

Signed-off-by: Anders Kaseorg <redacted>
---

On Fri, 27 Sep 2013, Johan Herland wrote:

1. Please add the use case you mention above as a new test case, so
that we can easily catch future regressions.

Test added.

2. If we are unlucky there might be existing users that work around the 
existing behavior by adding an extra level of quoting (i.e. doing the 
equivalent of git submodule foreach git grep "\'" in your example 
above). Will their workaround break as a result of your change? Is that 
acceptable?

Anyone adding an extra level of quoting ought to realize that they should 
be passing a single argument to submodule foreach, so that the reason for 
the extra quoting is clear:
  git submodule foreach "git grep \'"
will not break.  If someone is actually doing
  git submodule foreach git grep "\'"
then this will change in behavior.  I think this change is important.

(One could even imagine someone feeding untrusted input to
  git submodule foreach git grep "$variable"
which, without my patch, results in a nonobvious shell code injection 
vulnerability.)

I considered an alternative fix where the first argument is always 
shell-evaulated and any others are not (i.e. cmd=$1 && shift && eval 
"$cmd \"\$@\""), which is potentially more useful in case the command 
needs to use $path.  But that may be too confusing, and this way has some 
precedent (e.g. perl’s system()).

Anders


 git-submodule.sh             | 7 ++++++-
 t/t7407-submodule-foreach.sh | 9 +++++++++
 2 files changed, 15 insertions(+), 1 deletion(-)

diff --git a/git-submodule.sh b/git-submodule.sh
index c17bef1..3381864 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh

@@ -545,7 +545,12 @@ cmd_foreach()
 				sm_path=$(relative_path "$sm_path") &&
 				# we make $path available to scripts ...
 				path=$sm_path &&
-				eval "$@" &&
+				if [ $# -eq 1 ]
+				then
+					eval "$1"
+				else
+					"$@"
+				fi &&
 				if test -n "$recursive"
 				then
 					cmd_foreach "--recursive" "$@"

diff --git a/t/t7407-submodule-foreach.sh b/t/t7407-submodule-foreach.sh
index be93f10..6b2fd39 100755
--- a/t/t7407-submodule-foreach.sh
+++ b/t/t7407-submodule-foreach.sh

@@ -329,4 +329,13 @@ test_expect_success 'command passed to foreach --recursive retains notion of std
 	test_cmp expected actual
 '
 
+test_expect_success 'multi-argument command passed to foreach is not shell-evaluated twice' '
+	(
+		cd super &&
+		git submodule foreach "echo \\\"quoted\\\"" > ../expected &&
+		git submodule foreach echo \"quoted\" > ../actual
+	) &&
+	test_cmp expected actual
+'
+
 test_done

-- 
1.8.4

On Fri, Sep 27, 2013 at 12:23 PM, Anders Kaseorg [off-list ref] wrote:

‘eval "$@"’ created an extra layer of shell interpretation, which was
probably not expected by a user who passed multiple arguments to git
submodule foreach:

$ git grep "'"
[searches for single quotes]
$ git submodule foreach git grep "'"
Entering '[submodule]'
/usr/lib/git-core/git-submodule: 1: eval: Syntax error: Unterminated quoted string
Stopping at '[submodule]'; script returned non-zero status.

To fix this, if the user passed more than one argument, just execute
"$@" directly instead of passing it to eval.

Signed-off-by: Anders Kaseorg <redacted>

Acked-by: Johan Herland <redacted>

On Fri, 27 Sep 2013, Johan Herland wrote:

quoted

2. If we are unlucky there might be existing users that work around the
existing behavior by adding an extra level of quoting (i.e. doing the
equivalent of git submodule foreach git grep "\'" in your example
above). Will their workaround break as a result of your change? Is that
acceptable?

Anyone adding an extra level of quoting ought to realize that they should
be passing a single argument to submodule foreach, so that the reason for
the extra quoting is clear:
  git submodule foreach "git grep \'"
will not break.  If someone is actually doing
  git submodule foreach git grep "\'"
then this will change in behavior.  I think this change is important.

(One could even imagine someone feeding untrusted input to
  git submodule foreach git grep "$variable"
which, without my patch, results in a nonobvious shell code injection
vulnerability.)

I considered an alternative fix where the first argument is always
shell-evaulated and any others are not (i.e. cmd=$1 && shift && eval
"$cmd \"\$@\""), which is potentially more useful in case the command
needs to use $path.  But that may be too confusing, and this way has some
precedent (e.g. perl’s system()).

Ok. I have nothing to add.

...Johan

-- 
Johan Herland, [off-list ref]
www.herland.net

Hey folks,

On Thu, Sep 26, 2013 at 04:10:15PM -0400, Anders Kaseorg wrote:

‘eval "$@"’ created an extra layer of shell interpretation, which was
probably not expected by a user who passed multiple arguments to git
submodule foreach:

It seems this patch has broken the use of $name, $path, etc. inside the
command ran by foreach (when it contains more than one argument):


matthijs@grubby:~/test$ git --version
git version 1.9.0
matthijs@grubby:~/test$ git submodule foreach echo '$name'
Entering 'test'
$name

But it works on the single-argument version:

matthijs@grubby:~/test$ git submodule foreach 'echo $name'
Entering 'test'
test

And it used to work in older versions:

matthijs@login:~/test$ git --version
git version 1.7.5.4
matthijs@login:~/test$ git submodule foreach 'echo $name'
Entering 'test'
test
matthijs@login:~/test$ git submodule foreach echo '$name'
Entering 'test'
test


I'm not sure how to fix this exactly. Adding "export" for the variables in
git-submodule.sh seems obvious but doesn't seem to be a complete solution. This
makes the variables available in the environment of any commands called (so git
submodule sh -c 'echo $name') works, but the git submodule foreach echo '$name'
above still doesn't work, since the "$@" used does not do any substitution, it
just executes $@ as a commandline unmodified. Ideally, you would do variable
substitution, but not word splitting, but I'm not sure how to do that. Also,
you'd still need one more layer of backslash escapes, which is probably what
this commit wanted to prevent...

Note that saying "you should use the single argument version if you need
those variables" doesn't seem possible in all cases. In particular, I'm
creating an alias that calls git submodule foreach, where the alias
contains part of the command and the rest of command comes from
arguments to the alias, meaning we always have at least two arguments...

Finally, the new behaviour (e.g., eval with one argument, directly
execute with multiple) is not documented in the manpage, but it seems
relevant enough to need documentation?

Gr.

Matthijs

On Tue, Mar 4, 2014 at 2:51 PM, Matthijs Kooijman [off-list ref] wrote:

matthijs@grubby:~/test$ git submodule foreach echo '$name'
Entering 'test'
$name

jherland@beta ~/test$ echo '$name'
$name

What would you expect echo '$name' to do? What happens if you use
double instead of single quotes?


...Johan

-- 
Johan Herland, [off-list ref]
www.herland.net

On Tue, Mar 04, 2014 at 03:53:24PM +0100, Johan Herland wrote:

On Tue, Mar 4, 2014 at 2:51 PM, Matthijs Kooijman [off-list ref] wrote:

quoted

matthijs@grubby:~/test$ git submodule foreach echo '$name'
Entering 'test'
$name

jherland@beta ~/test$ echo '$name'
$name

What would you expect echo '$name' to do?

If I run git submodule foreach each '$name', then my shell eats the
single quotes (which are only to prevent my shell from interpreting
$name). git submodule will see $name, so it will run echo $name, not
echo '$name'.

What happens if you use double instead of single quotes?

Then my shell eats up the double quotes _and_ replaces $name with
nothing, so I can't expect git submodule to replace it with the
submodule name then :-)

Does that help to clarify what I mean?

Gr.

Matthijs

On Tue, Mar 4, 2014 at 3:57 PM, Matthijs Kooijman [off-list ref] wrote:

On Tue, Mar 04, 2014 at 03:53:24PM +0100, Johan Herland wrote:

quoted

What would you expect echo '$name' to do?

If I run git submodule foreach each '$name', then my shell eats the
single quotes (which are only to prevent my shell from interpreting
$name). git submodule will see $name, so it will run echo $name, not
echo '$name'.

quoted

What happens if you use double instead of single quotes?

Then my shell eats up the double quotes _and_ replaces $name with
nothing, so I can't expect git submodule to replace it with the
submodule name then :-)

Does that help to clarify what I mean?

Ok, so IINM, Anders' original commit was about making "git submodule
foreach <command>" behave more like "<command>" (from a naive user's
perspective), while you rather expect to insert quotes/escapes to
finely control exactly when shell interpretation happens. Aren't these
POVs mutually incompatible? Is the only 'real' solution to forbid
multitple arguments, and force everybody to quote the entire command?

I don't particularly care which way it goes, as long as (a) the common
case behaves as most users would expect, (b) the uncommon/complicated
case is still _possible_ (though not necessarily simple), and (c) we
don't break a sizable number of existing users.

...Johan

-- 
Johan Herland, [off-list ref]
www.herland.net

Hey Johan,

Ok, so IINM, Anders' original commit was about making "git submodule
foreach <command>" behave more like "<command>" (from a naive user's
perspective),

Ok, that makes sense.

while you rather expect to insert quotes/escapes to finely control
exactly when shell interpretation happens.

Well, I mostly expect that the $name and $path that git submodule makes
available to each command invocation can actually be used by the
command.

Aren't these POVs mutually incompatible? Is the only 'real' solution
to forbid multitple arguments, and force everybody to quote the entire
command?

Yes, I think you're right that they're mutually exclusive. Specifically,
if you expect git submodule foreach <command> to behave like <command>,
that means you expect the (interactive) shell to do all the
interpolation, word-splitting, etc. If so, you can't then later still do
interpolation (of course, you could do sed magic to just replace $name
and $path, etc., but that's broken).

I don't particularly care which way it goes, as long as (a) the common
case behaves as most users would expect, (b) the uncommon/complicated
case is still _possible_ (though not necessarily simple), and (c) we
don't break a sizable number of existing users.

Well, if you call submodule directly, you can now just put everything in
a single command and get $name interpolation.

As I mentioned, I couldn't do this because I was using a git alias.
However, a bit of fiddling showed a solution to that using a shell
function:

[alias]
	each = "!f(){ git submodule foreach --quiet \"echo \\$name $*\";}; f"

This uses a shell function to collect all alias arguments and then uses
$* to expand them again into the single submodule foreach argument. Note
that $* is expanded when evaluating the alias, while \\$name is expanded
later inside submodule.

This suggests that with the current code, the more complicated cases are
still possible. There is one catch in this approach, in that the
original word splitting is not preserved ($* expands to just the
unquoted arguments as a single word). I'm not sure if this is fixable
($@ expands to multiple quoted words, but then foreach sees multiple
arguments and doesn't do the eval). One would need to escape the output
of $@ somehow (e.g., add \ before ", but that would become terribly
complicated I expect...).


Perhaps an explicit --eval switch to git submodule makes sense for
complete control? If it has a correspondning --no-eval, you can even
pass a single-argument command without evalling, while still keeping the
current "least surprise" approach as the default?

Whatever behaviour is settled for, it should be documented in the
submodule manpage (which I think is not the case now).

Gr.

Matthijs