[PATCH v2] git submodule foreach: Skip eval for more than one argument
- From
Anders Kaseorg <andersk@mit.edu>
- Date
- Sep 27, 2013, 10:23 UTC
- Message-ID
- <alpine.DEB.2.00.1309270606290.20647@dr-wily.mit.edu>
- In-Reply-To
- <CALKQrgfhUEE+E5KsAWbP_zj6tozk+V=qvNU1PX9Z73Vu8unTiQ@mail.gmail.com>
‘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 <andersk@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.
Show 5 quoted lines
> 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