On Fri, Sep 27, 2013 at 12:23 PM, Anders Kaseorg <andersk@xxxxxxx> 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 <andersk@xxxxxxx> Acked-by: Johan Herland <johan@xxxxxxxxxxx> > On Fri, 27 Sep 2013, Johan Herland wrote: >> 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, <johan@xxxxxxxxxxx> www.herland.net -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html