test-lib: avoid accidental globbing in match_pattern_list()

We have a custom match_pattern_list() function which we use for matching test names (like "t1234") against glob-like patterns (like "t1???") for $GIT_SKIP_TESTS, --verbose-only, etc. Those patterns may have multiple whitespace-separated elements (e.g., "t0* t1234 t5?78"). The callers of match_pattern_list thus pass the strings unquoted, so that the shell does the usual field-splitting into separate arguments. But this also means the shell will do the usual globbing for each argument, which can result in us seeing an expansion based on what's in the filesystem, rather than the real pattern. For example, if I have the path "t5000" in the filesystem, and you feed the pattern "t?000", that _should_ match the string "t0000", but it won't after the shell has expanded it to "t5000". This has been a bug ever since that function was introduced. But it didn't usually trigger since we typically use the function inside the trash directory, which has a very limited set of files that are unlikely to match. It became a lot easier to trigger after edc23840b0 (test-lib: bring $remove_trash out of retirement, 2021-05-10), because now we match $GIT_SKIP_TESTS before even entering the trash directory. So the t5000 example above can be seen with: GIT_SKIP_TESTS=t?000 ./t0000-basic.sh which should skip all tests but doesn't. We can fix this by using "set -f" to ask the shell not to glob (which is in POSIX, so should hopefully be portable enough). We only want to do this in a subshell (to avoid polluting the rest of the script), which means we need to get the whole string intact into the match_pattern_list function by quoting it. Arguably this is a good idea anyway, since it makes it much more obvious that we intend to split, and it's not simply sloppy scripting. Diagnosed-by: Junio C Hamano <gitster@pobox.com> Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Jeff King committed Jun 16, 2021 at 06:23 UTC 560bf518923e0bf40b43d71bb6238cbe13b016b1
1 file changed +22 -12
t/test-lib.sh
+22 -12
@@ -732,14 +732,24 @@ match_pattern_list () {
732 arg="$1"
733 shift
734 test -z "$*" && return 1
735 - for pattern_
736 - do
737 - case "$arg" in
738 - $pattern_)
739 - return 0
740 - esac
741 - done
742 - return 1
735 + # We need to use "$*" to get field-splitting, but we want to
736 + # disable globbing, since we are matching against an arbitrary
737 + # $arg, not what's in the filesystem. Using "set -f" accomplishes
738 + # that, but we must do it in a subshell to avoid impacting the
739 + # rest of the script. The exit value of the subshell becomes
740 + # the function's return value.
741 + (
742 + set -f
743 + for pattern_ in $*
744 + do
745 + case "$arg" in
746 + $pattern_)
747 + exit 0
748 + ;;
749 + esac
750 + done
751 + exit 1
752 + )
753 }
754
755 match_test_selector_list () {
@@ -848,7 +858,7 @@ maybe_teardown_verbose () {
858 last_verbose=t
859 maybe_setup_verbose () {
860 test -z "$verbose_only" && return
851 - if match_pattern_list $test_count $verbose_only
861 + if match_pattern_list $test_count "$verbose_only"
862 then
863 exec 4>&2 3>&1
864 # Emit a delimiting blank line when going from
@@ -878,7 +888,7 @@ maybe_setup_valgrind () {
888 return
889 fi
890 GIT_VALGRIND_ENABLED=
881 - if match_pattern_list $test_count $valgrind_only
891 + if match_pattern_list $test_count "$valgrind_only"
892 then
893 GIT_VALGRIND_ENABLED=t
894 fi
@@ -1006,7 +1016,7 @@ test_finish_ () {
1016 test_skip () {
1017 to_skip=
1018 skipped_reason=
1009 - if match_pattern_list $this_test.$test_count $GIT_SKIP_TESTS
1019 + if match_pattern_list $this_test.$test_count "$GIT_SKIP_TESTS"
1020 then
1021 to_skip=t
1022 skipped_reason="GIT_SKIP_TESTS"
@@ -1346,7 +1356,7 @@ fi
1356 remove_trash=
1357 this_test=${0##*/}
1358 this_test=${this_test%%-*}
1349 -if match_pattern_list "$this_test" $GIT_SKIP_TESTS
1359 +if match_pattern_list "$this_test" "$GIT_SKIP_TESTS"
1360 then
1361 say_color info >&3 "skipping test $this_test altogether"
1362 skip_all="skip all tests in $this_test"