From cbc346b279b5c08b281c9901b994a58eb037a60d Mon Sep 17 00:00:00 2001 From: Edd Salkield Date: Sat, 18 Jan 2020 18:40:45 +0000 Subject: 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. --- Makefile.am | 2 ++ bin/lsrc.in | 18 ++++++++-------- bin/mkrc.in | 2 +- bin/rcdn.in | 54 +++++++++++++++++++++++------------------------ bin/rcup.in | 50 +++++++++++++++++++++---------------------- man/lsrc.1 | 7 ------ share/rcm.sh.in | 8 +++++++ test/lsrc-globs.t | 7 ++++++ test/lsrc-undotted-star.t | 14 ++++++++++++ 9 files changed, 93 insertions(+), 69 deletions(-) create mode 100644 test/lsrc-globs.t create mode 100644 test/lsrc-undotted-star.t 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=' diff --git a/man/lsrc.1 b/man/lsrc.1 index 5abf7ee..b938fee 100644 --- a/man/lsrc.1 +++ b/man/lsrc.1 @@ -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) -- cgit v1.2.3