koalaman / shellcheck

ShellCheck, a static analysis tool for shell scripts
https://www.shellcheck.net
GNU General Public License v3.0
36.4k stars 1.78k forks source link

Warn about ambiguity of `grep "$var"` (recommend -e) #1342

Open AloisMahdal opened 6 years ago

AloisMahdal commented 6 years ago

For new checks and feature suggestions

Here's a snippet or screenshot that shows the problem:

#!/bin/bash
grep -xF "$1"

The option bundle may be missing or different, but as long as it does not end with -e or -f (and maybe some others), the code applies.

Here's what shellcheck currently says:

(nothing)

Here's what I wanted or expected to see:

Warning about the ambiguilty when variable contents are used as regular expression. Recommend always adding -e so that the variable content will always be interpreted correctly.

Rationale

When the variable passed as regex has value that looks like grep option, grep will treat it as such, which will result in something that user almost never wants. For example, if the variable is --help, the grep prints help message but exits with 0, producing empty stdout. Another example would be something like -s that would be "consumed" as option, causing the next argument to be interpreted as the regex, etc.

I found this bug by accident, and never realized this, despite having written hundreds of Bash scripts in the last ~5 years. So I'm currently going through all that I can find, and turns oout I was using this pattern really, really often.

This is what I'm using to find the offending scripts:

grep  '\bgrep  *[a-zA-Z-][a-dg-zA-Z]\+  *"[$]' -l -R

This looks for grep called with option bundles ending with something else than -e or -f. (Needless to say, it's not perfect but it's all I have now.)

AloisMahdal commented 6 years ago

Just checking the manpage:

-e PATTERN, --regexp=PATTERN
        Use PATTERN as the pattern.  If this option is used multiple
        times or is combined with the -f (--file) option, search for
        all patterns given.  This option can be  used  to protect
        a pattern beginning with “-”.

So indeed the -e seems to be the correct way to prevent this issue. Also it's worth noting that many expressions will be relatively safe, ie. if the quoted argument cannot end up starting with - eg. "^$var" -- ShellCheck could leave these cases alone. (That said, I myself found a scary amount of cases in my scripts when I just pass the variable as is.)

AloisMahdal commented 4 years ago

Small update; here's a script I've been using to search for this problem recursuvely in my .git projects.

#!/bin/sh

GREP_OPT_BUNDLE='[a-zA-Z-][a-dg-zA-Z]\+'
GREPE_RE1='\<grep  *'$GREP_OPT_BUNDLE'  *"[$]'
GREPE_RE2='\<grep  *"[$]'
grep --color -n --exclude-dir=".git" -e "$GREPE_RE1" -e "$GREPE_RE2" -R

It's a little bit enhanced version of the one above.