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

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