‘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 
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 <ande...@mit.edu>

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 

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()).


 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"
                                        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

To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Reply via email to