diff options
author | Edd Salkield <edd@salkield.uk> | 2020-01-18 18:40:45 +0000 |
---|---|---|
committer | Mike Burns <mburns@thoughtbot.com> | 2020-04-03 16:21:07 -0400 |
commit | cbc346b279b5c08b281c9901b994a58eb037a60d (patch) | |
tree | 387a6fd63a9332e43ca9f29a4b65ada39f85f45d | |
parent | f2fb351c391dca7c188a8623e71519619c2ce9a0 (diff) |
Fix shell globbing bugs
There are several problems leading to the unintentional globbing issue:
Firstly, within `rcup` and `rcdn`, when constructing arguments to pass
to `lsrc`, the _for_ loops over the arguments do not have quoted
variables, leading to globbing. I have quoted these accordingly.
Secondly, `lsrc` is invoked as follows:
```sh
dests_and_srcs="$(lsrc $LS_ARGS)"
```
When shells use command substitution like this, they go through two stages:
- Word expansion. This is useful because it splits `LS_ARGS` back up
into its constituent strings.
- File name expansion. The side effect of this is to introduce globbing.
You can read more about how this works [here](https://www.tldp.org/LDP/Bash-Beginners-Guide/html/sect_03_04.html#sect_03_04_07).
To fix this, I have passed `lsrc` and its arguments to `eval`. This
involves quoting the relevant arguments, so:
```sh
for dotfiles_dir in "$DOTFILES_DIRS"; do
LS_ARGS="$LS_ARGS -d $dotfiles_dir"
done
```
becomes
```sh
for dotfiles_dir in "$DOTFILES_DIRS"; do
LS_ARGS="$LS_ARGS -d \"$dotfiles_dir\""
done
```
Then `lsrc` is invoked as follows:
```sh
dests_and_srcs="$(eval "lsrc $LS_ARGS")"
```
There is one final non-globbing issue: the parsing of arguments can
introduce extra spaces in the variables, which then trip up the
`dotfiles_dir_excludes` function. For example:
```sh
I) includes="$includes $OPTARG";;
```
introduces a space if `includes` is empty or null.
I have introduced the function `append_variable`, which allows two
variables to be appended without introducing unnecessary whitespace.
Then the additional whitespace is never added in the first place.
Fixes #256.
-rw-r--r-- | Makefile.am | 2 | ||||
-rwxr-xr-x | bin/lsrc.in | 18 | ||||
-rwxr-xr-x | bin/mkrc.in | 2 | ||||
-rwxr-xr-x | bin/rcdn.in | 54 | ||||
-rwxr-xr-x | bin/rcup.in | 50 | ||||
-rw-r--r-- | man/lsrc.1 | 7 | ||||
-rw-r--r-- | share/rcm.sh.in | 8 | ||||
-rw-r--r-- | test/lsrc-globs.t | 7 | ||||
-rw-r--r-- | test/lsrc-undotted-star.t | 14 |
9 files changed, 93 insertions, 69 deletions
diff --git a/Makefile.am b/Makefile.am index 4af524d..fb68ca8 100644 --- a/Makefile.am +++ b/Makefile.am @@ -19,7 +19,9 @@ TESTS = \ test/lsrc-tags.t \ test/lsrc-usage.t \ test/lsrc-undotted.t \ + test/lsrc-undotted-star.t \ test/lsrc-host-tags-default.t \ + test/lsrc-globs.t \ test/mkrc-alternate-dotfiles-dir.t \ test/mkrc-copy-file.t \ test/mkrc-host-file.t \ diff --git a/bin/lsrc.in b/bin/lsrc.in index 6fb6162..eed4c0a 100755 --- a/bin/lsrc.in +++ b/bin/lsrc.in @@ -166,7 +166,7 @@ dotfiles_dir_excludes() { $DEBUG "dotfiles_dir_excludes $dotfiles_dir" $DEBUG " with excludes: $excludes" - for exclude in $excludes; do + for exclude in "$excludes"; do if echo "$exclude" | grep ':' >/dev/null; then dotfiles_dir_pat="$(echo "$exclude" | sed 's/:.*//')" file_glob="$(echo "$exclude" | sed 's/.*://')" @@ -259,17 +259,17 @@ handle_command_line() { case "$opt" in F) show_sigils=1;; h) show_help ;; - I) includes="$includes $OPTARG";; - t) arg_tags="$arg_tags $OPTARG";; + I) includes="$(append_variable "$includes" "$OPTARG")" ;; + t) arg_tags="$(append_variable "$arg_tags" "$OPTARG")" ;; v) verbosity=$(($verbosity + 1));; q) verbosity=$(($verbosity - 1));; - d) dotfiles_dirs="$dotfiles_dirs $OPTARG";; + d) dotfiles_dirs="$(append_variable "$dotfiles_dirs" "$OPTARG")" ;; V) version=1;; - x) excludes="$excludes $OPTARG";; - S) symlink_dirs="$symlink_dirs $OPTARG";; - s) never_symlink_dirs="$never_symlink_dirs $OPTARG";; - U) undotted="$undotted $OPTARG";; - u) never_undotted="$never_undotted $OPTARG";; + x) excludes="$(append_variable "$excludes" "$OPTARG")" ;; + S) symlink_dirs="$(append_variable "$symlink_dirs" "$OPTARG")" ;; + s) never_symlink_dirs="$(append_variable "$never_symlink_dirs" "$OPTARG")" ;; + U) undotted="$(append_variable "$undotted" "$OPTARG")" ;; + u) never_undotted="$(append_variable "$never_undotted" "$OPTARG")" ;; B) hostname="$OPTARG";; ?) show_help 64 ;; esac diff --git a/bin/mkrc.in b/bin/mkrc.in index d246c33..5628f14 100755 --- a/bin/mkrc.in +++ b/bin/mkrc.in @@ -86,7 +86,7 @@ while getopts :ChSsUuVvqot:d:B: opt; do B) in_host=1 hostname="$OPTARG" - install_args="-B $hostname" + install_args=$(append_variable "$install_args" "-B $hostname") ;; ?) show_help 64 ;; esac diff --git a/bin/rcdn.in b/bin/rcdn.in index a2a8472..3ebbd27 100755 --- a/bin/rcdn.in +++ b/bin/rcdn.in @@ -48,19 +48,19 @@ handle_command_line() { case "$opt" in h) show_help ;; B) hostname="$OPTARG" ;; - I) includes="$includes $OPTARG";; + I) includes="$(append_variable "$includes" "$OPTARG")" ;; k) run_hooks=1 ;; K) run_hooks=0 ;; - t) arg_tags="$arg_tags $OPTARG" ;; - S) symlink_dirs="$symlink_dirs $OPTARG";; - s) never_symlink_dirs="$never_symlink_dirs $OPTARG";; - U) undotted="$undotted $OPTARG";; - u) never_undotted="$never_undotted $OPTARG";; + t) arg_tags="$(append_variable "$arg_tags" "$OPTARG")" ;; + S) symlink_dirs="$(append_variable "$symlink_dirs" "$OPTARG")" ;; + s) never_symlink_dirs="$(append_variable "$never_symlink_dirs" "$OPTARG")" ;; + U) undotted="$(append_variable "$undotted" "$OPTARG")" ;; + u) never_undotted="$(append_variable "$never_undotted" "$OPTARG")";; v) verbosity=$(($verbosity + 1));; q) verbosity=$(($verbosity - 1));; - d) dotfiles_dirs="$dotfiles_dirs $OPTARG" ;; + d) dotfiles_dirs="$(append_variable "$dotfiles_dirs" "$OPTARG")" ;; V) version=1 ;; - x) excludes="$excludes $OPTARG" ;; + x) excludes="$(append_variable "$excludes" "$OPTARG")" ;; ?) show_help 64 ;; esac done @@ -72,34 +72,34 @@ handle_command_line() { tags="${arg_tags:-$TAGS}" dotfiles_dirs="${dotfiles_dirs:-$DOTFILES_DIRS}" files="$@" - RUN_HOOKS=$run_hooks + RUN_HOOKS="$run_hooks" - for tag in $tags; do - LS_ARGS="$LS_ARGS -t $tag" + for tag in "$tags"; do + LS_ARGS="$LS_ARGS -t \"$tag\"" done - for dotfiles_dir in $dotfiles_dirs; do - LS_ARGS="$LS_ARGS -d $dotfiles_dir" + for dotfiles_dir in "$dotfiles_dirs"; do + LS_ARGS="$LS_ARGS -d \"$dotfiles_dir\"" done - for exclude in $excludes; do - LS_ARGS="$LS_ARGS -x $exclude" + for exclude in "$excludes"; do + LS_ARGS="$LS_ARGS -x \"$exclude\"" done - for include in $includes; do - LS_ARGS="$LS_ARGS -I $include" + for include in "$includes"; do + LS_ARGS="$LS_ARGS -I \"$include\"" done - for symlink_dir in $symlink_dirs; do - LS_ARGS="$LS_ARGS -S $symlink_dir" + for symlink_dir in "$symlink_dirs"; do + LS_ARGS="$LS_ARGS -S \"$symlink_dir\"" done - for never_symlink_dir in $symlink_dirs; do - LS_ARGS="$LS_ARGS -s $never_symlink_dir" + for never_symlink_dir in "$symlink_dirs"; do + LS_ARGS="$LS_ARGS -s \"$never_symlink_dir\"" done - for undot in $undotted; do - LS_ARGS="$LS_ARGS -U $undot" + for undot in "$undotted"; do + LS_ARGS="$LS_ARGS -U \"$undot\"" done - for never_undot in $never_undotted; do - LS_ARGS="$LS_ARGS -u $never_undot" + for never_undot in "$never_undotted"; do + LS_ARGS="$LS_ARGS -u \"$never_undot\"" done - LS_ARGS="$LS_ARGS -B $hostname $files" + LS_ARGS="$LS_ARGS -B \"$hostname\" $files" $DEBUG "LS_ARGS: $LS_ARGS" } @@ -111,7 +111,7 @@ handle_command_line "$@" run_hooks pre down -dests_and_srcs="$(lsrc $LS_ARGS)" +dests_and_srcs="$(eval "lsrc $LS_ARGS")" saved_ifs="$IFS" IFS=' diff --git a/bin/rcup.in b/bin/rcup.in index 78f5faa..65f0ab8 100755 --- a/bin/rcup.in +++ b/bin/rcup.in @@ -213,23 +213,23 @@ handle_command_line() { case "$opt" in B) hostname="$OPTARG" ;; C) always_copy=1 ;; - d) dotfiles_dirs="$dotfiles_dirs $OPTARG" ;; + d) dotfiles_dirs="$(append_variable "$dotfiles_dirs" "$OPTARG")" ;; f) REPLACE_ALL=1 ;; g) generate=1 ;; h) show_help ;; i) REPLACE_ALL=0 ;; - I) includes="$includes $OPTARG" ;; + I) includes="$(append_variable "$includes" "$OPTARG")" ;; k) run_hooks=1 ;; K) run_hooks=0 ;; q) verbosity=$(($verbosity - 1)) ;; - t) arg_tags="$arg_tags $OPTARG" ;; - S) symlink_dirs="$symlink_dirs $OPTARG";; - s) never_symlink_dirs="$never_symlink_dirs $OPTARG";; - U) undotted="$undotted $OPTARG";; - u) never_undotted="$never_undotted $OPTARG";; + t) arg_tags="$(append_variable "$arg_tags" "$OPTARG")" ;; + S) symlink_dirs="$(append_variable "$symlink_dirs" "$OPTARG")" ;; + s) never_symlink_dirs="$(append_variable "$never_symlink_dirs" "$OPTARG")";; + U) undotted="$(append_variable "$undotted" "$OPTARG")" ;; + u) never_undotted="$(append_variable "$never_undotted" "$OPTARG")" ;; v) verbosity=$(($verbosity + 1)) ;; V) version=1 ;; - x) excludes="$excludes $OPTARG" ;; + x) excludes="$(append_variable "$excludes" "$OPTARG")" ;; ?) show_help 64 ;; esac done @@ -261,31 +261,31 @@ handle_command_line() { done for tag in $tags; do - LS_ARGS="$LS_ARGS -t $tag" + LS_ARGS="$LS_ARGS -t \"$tag\"" done - for dotfiles_dir in $DOTFILES_DIRS; do - LS_ARGS="$LS_ARGS -d $dotfiles_dir" + for dotfiles_dir in "$DOTFILES_DIRS"; do + LS_ARGS="$LS_ARGS -d \"$dotfiles_dir\"" done - for exclude in $excludes; do - LS_ARGS="$LS_ARGS -x $exclude" + for exclude in "$excludes"; do + LS_ARGS="$LS_ARGS -x \"$exclude\"" done - for include in $includes; do - LS_ARGS="$LS_ARGS -I $include" + for include in "$includes"; do + LS_ARGS="$LS_ARGS -I \"$include\"" done - for symlink_dir in $symlink_dirs; do - LS_ARGS="$LS_ARGS -S $symlink_dir" + for symlink_dir in "$symlink_dirs"; do + LS_ARGS="$LS_ARGS -S \"$symlink_dir\"" done - for never_symlink_dir in $never_symlink_dirs; do - LS_ARGS="$LS_ARGS -s $never_symlink_dir" + for never_symlink_dir in "$never_symlink_dirs"; do + LS_ARGS="$LS_ARGS -s \"$never_symlink_dir\"" done - for undot in $undotted; do - LS_ARGS="$LS_ARGS -U $undot" + for undot in "$undotted"; do + LS_ARGS="$LS_ARGS -U \"$undot\"" done - for never_undot in $never_undotted; do - LS_ARGS="$LS_ARGS -u $never_undot" + for never_undot in "$never_undotted"; do + LS_ARGS="$LS_ARGS -u \"$never_undot\"" done - LS_ARGS="$LS_ARGS -B $hostname $files" + LS_ARGS="$LS_ARGS -B \"$hostname\" $files" $DEBUG "LS_ARGS: $LS_ARGS" } @@ -297,7 +297,7 @@ handle_command_line "$@" run_hooks pre up -dests_and_srcs="$(lsrc $LS_ARGS)" +dests_and_srcs="$(eval "lsrc $LS_ARGS")" saved_ifs="$IFS" IFS=' @@ -190,10 +190,3 @@ We use the program to determine the unique identifier for the host. This program is not specified by POSIX and can vary by system. On macOS the hostname is unpredictable, and can even change as part of the DHCP handshake. -.Pp -There are a few bugs around shell globs. Anything involving an exclude -pattern is unpredictable, so use -.Fl v -when dealing with patterns. Specifically, globs may expand at any -time and remain expanded for the duration of the run, which means they -cannot be applied more than once. diff --git a/share/rcm.sh.in b/share/rcm.sh.in index f7c6137..5d0a373 100644 --- a/share/rcm.sh.in +++ b/share/rcm.sh.in @@ -171,6 +171,14 @@ decode() { echo "$file" | tr "$DELIMITER" " " } +append_variable() { + if [ -z "$1" ]; then + echo "$2" + else + echo "$1 $2" + fi +} + : ${RCRC:=$HOME/.rcrc} if [ -r "$RCRC" ]; then diff --git a/test/lsrc-globs.t b/test/lsrc-globs.t new file mode 100644 index 0000000..cf04385 --- /dev/null +++ b/test/lsrc-globs.t @@ -0,0 +1,7 @@ + $ . "$TESTDIR/helper.sh" + +Keeps globs as globs + + $ mkdir vimulator + > lsrc -vvv -x '*vim*' 2>&1 | grep exclude_file_globs + exclude_file_globs: *vim* diff --git a/test/lsrc-undotted-star.t b/test/lsrc-undotted-star.t new file mode 100644 index 0000000..04db053 --- /dev/null +++ b/test/lsrc-undotted-star.t @@ -0,0 +1,14 @@ + $ . "$TESTDIR/helper.sh" + +Should undot files with -U, with wildcard * expansion + + $ touch .dotfiles/example + > touch .dotfiles/undotted + + $ lsrc -v -U '*' + /*/example:/*/.dotfiles/example (glob) + /*/undotted:/*/.dotfiles/undotted (glob) + + $ lsrc -v -U '*:*' + /*/example:/*/.dotfiles/example (glob) + /*/undotted:/*/.dotfiles/undotted (glob) |