[CI] Add shellcheck and fix up warnings (#2809)

# Motivation

We have quite a lot of shell scripts in our repo and want to make sure that they all pass `shellcheck`.

# Modification

This PR adds a GH action workflow to the soundness script for `shellcheck` and fixes up all errors and warnings.

# Result

No more shell/bash discussions

.github/workflows/soundness.yml

@@ -130,3 +130,17 @@ jobs:
       run:  |
         apt-get -qq update && apt-get -qq -y install curl
         curl -s https://raw.githubusercontent.com/apple/swift-nio/main/scripts/check-swift-format.sh | bash
+
+  shell-check:
+    name: Shell check
+    if: ${{ inputs.format_check_enabled }}
+    runs-on: ubuntu-latest
+    timeout-minutes: 5
+    steps:
+    - name: Checkout repository
+      uses: actions/checkout@v4
+    - name: Mark the workspace as safe
+    # https://github.com/actions/checkout/issues/766
+      run: git config --global --add safe.directory ${GITHUB_WORKSPACE}
+    - name: Run shellcheck
+      run:  git ls-files -z '*.sh' | xargs -0 shellcheck

IntegrationTests/allocation-counter-tests-framework/run-allocation-counter.sh

@@ -226,7 +226,7 @@ while getopts "ns:p:m:d:t:" opt; do
             do_hooking=false
             ;;
         s)
-            shared_files+=( $(abs_path "$OPTARG") )
+            shared_files+=( "$(abs_path "$OPTARG")" )
             ;;
         p)
             pkg_root=$(abs_path "$OPTARG")

IntegrationTests/plugin_junit_xml.sh

@@ -44,9 +44,9 @@ function junit_output_replace() {
 
 function plugin_junit_xml_test_suite_begin() {
     junit_testsuite_time=0
-    junit_output_write "<testsuite name='$1' hostname='$(hostname)' "\
-"timestamp='$(date -u +"%Y-%m-%dT%H:%M:%S")' tests='XXX-TESTS-XXX' "\
-"failures='XXX-FAILURES-XXX' time='XXX-TIME-XXX' errors='0' id='$(date +%s)'"\
+    junit_output_write "<testsuite name='$1' hostname='$(hostname)' " \
+"timestamp='$(date -u +"%Y-%m-%dT%H:%M:%S")' tests='XXX-TESTS-XXX' " \
+"failures='XXX-FAILURES-XXX' time='XXX-TIME-XXX' errors='0' id='$(date +%s)'" \
 " package='NIOIntegrationTests.$1'>"
 }
 

IntegrationTests/run-single-test.sh

@@ -20,12 +20,17 @@ set -x
 set -o pipefail
 
 test="$1"
+# shellcheck disable=SC2034 # Used by whatever we source transpile in
 tmp="$2"
+# shellcheck disable=SC2034 # Used by whatever we source transpile in
 root="$3"
+# shellcheck disable=SC2034 # Used by whatever we source transpile in
 g_show_info="$4"
 here="$( cd "$( dirname "${BASH_SOURCE[0]}" )" && pwd )"
 
+# shellcheck source=IntegrationTests/test_functions.sh
 source "$here/test_functions.sh"
+# shellcheck source=/dev/null
 source "$test"
 wait
 )

IntegrationTests/run-tests.sh

@@ -36,7 +36,9 @@ function plugins_do() {
     done
 }
 
+# shellcheck source=IntegrationTests/plugin_echo.sh
 source "$here/plugin_echo.sh"
+# shellcheck source=/dev/null
 source "$here/plugin_junit_xml.sh"
 
 plugins="echo"
@@ -88,7 +90,7 @@ function run_test() {
     if $verbose; then
         "$@" 2>&1 | tee -a "$out"
         # we need to return the return value of the first command
-        return ${PIPESTATUS[0]}
+        return "${PIPESTATUS[0]}"
     else
         "$@" >> "$out" 2>&1
     fi
@@ -113,13 +115,13 @@ for f in tests_*; do
         plugins_do test_begin "$t" "$f"
         start=$(date +%s)
         if run_test "$here/run-single-test.sh" "$here/$f/$t" "$test_tmp" "$here/.." "$show_info"; then
-            plugins_do test_ok "$(time_diff_to_now $start)"
+            plugins_do test_ok "$(time_diff_to_now "$start")"
             suite_ok=$((suite_ok+1))
             if $verbose; then
                 cat "$out"
             fi
         else
-            plugins_do test_fail "$(time_diff_to_now $start)" "$out"
+            plugins_do test_fail "$(time_diff_to_now "$start")" "$out"
             suite_fail=$((suite_fail+1))
         fi
         if ! $debug; then
@@ -131,7 +133,7 @@ for f in tests_*; do
     cnt_ok=$((cnt_ok + suite_ok))
     cnt_fail=$((cnt_fail + suite_fail))
     cd ..
-    plugins_do test_suite_end "$(time_diff_to_now $start_suite)" "$suite_ok" "$suite_fail"
+    plugins_do test_suite_end "$(time_diff_to_now "$start_suite")" "$suite_ok" "$suite_fail"
 done
 
 if ! $debug; then
@@ -142,7 +144,7 @@ fi
 
 
 # report
-if [[ $cnt_fail > 0 ]]; then
+if [[ $cnt_fail -gt 0 ]]; then
     # terminate leftovers (the whole process group)
     trap '' TERM
     kill 0 # ignore-unacceptable-language
@@ -152,7 +154,7 @@ else
     plugins_do summary_ok "$cnt_ok" "$cnt_fail"
 fi
 
-if [[ $cnt_fail > 0 ]]; then
+if [[ $cnt_fail -gt 0 ]]; then
     exit 1
 else
     exit 0

IntegrationTests/test_functions.sh

@@ -64,6 +64,7 @@ function assert_greater_than_or_equal() {
 g_has_previously_infoed=false
 
 function info() {
+    # shellcheck disable=SC2154 # Defined by an include our by being source transpiled in
     if $g_show_info; then
         if ! $g_has_previously_infoed; then
             echo >&3 || true # echo an extra newline so it looks better

IntegrationTests/tests_01_http/defines.sh

@@ -58,7 +58,7 @@ function do_netstat() {
 }
 
 function create_token() {
-    mktemp "$tmp/server_token_XXXXXX"
+    mktemp "${tmp:?}/server_token_XXXXXX"
 }
 
 function start_server() {
@@ -84,6 +84,7 @@ function start_server() {
 
     mkdir "$tmp/htdocs"
     swift build
+    # shellcheck disable=SC2086 # Disabled to properly pass the args
     "$(swift build --show-bin-path)/NIOHTTP1Server" $extra_args $maybe_nio_host "$port" "$tmp/htdocs" &
     tmp_server_pid=$!
     case "$type" in
@@ -114,9 +115,11 @@ function start_server() {
     esac
     echo "port: $port"
     echo "curl port: $curl_port"
-    echo "local token_port;   local token_htdocs;         local token_pid;"      >> "$token"
-    echo "      token_port='$port'; token_htdocs='$tmp/htdocs'; token_pid='$!';" >> "$token"
-    echo "      token_type='$type'; token_server_ip='$maybe_nio_host'" >> "$token"
+    {
+        echo "local token_port;   local token_htdocs;         local token_pid;"
+        echo "      token_port='$port'; token_htdocs='$tmp/htdocs'; token_pid='$!';"
+        echo "      token_type='$type'; token_server_ip='$maybe_nio_host'"
+    } >> "$token"
     tmp_server_pid=$(get_server_pid "$token")
     echo "local token_open_fds" >> "$token"
     echo "token_open_fds='$(get_number_of_open_fds_for_pid "$tmp_server_pid")'" >> "$token"
@@ -125,21 +128,25 @@ function start_server() {
 }
 
 function get_htdocs() {
+    # shellcheck source=/dev/null
     source "$1"
-    echo "$token_htdocs"
+    # shellcheck disable=SC2154
+    echo "${token_htdocs:?}"
 }
 
 function get_socket() {
+    # shellcheck source=/dev/null
     source "$1"
-    echo "$token_port"
+    echo "${token_port:?}"
 }
 
 function stop_server() {
+    # shellcheck source=/dev/null
     source "$1"
     sleep 0.5 # just to make sure all the fds could be closed
     if command -v lsof > /dev/null 2> /dev/null; then
-        do_netstat "$token_type"
-        assert_number_of_open_fds_for_pid_equals "$token_pid" "$token_open_fds"
+        do_netstat "${token_type:?}"
+        assert_number_of_open_fds_for_pid_equals "${token_pid:?}" "${token_open_fds:?}"
     fi
     # assert server is still running
     kill -0 "$token_pid" # ignore-unacceptable-language
@@ -149,6 +156,7 @@ function stop_server() {
         if ! kill -0 "$token_pid" 2> /dev/null; then # ignore-unacceptable-language
             break # good, dead
         fi
+        # shellcheck disable=SC2009
         ps auxw | grep "$token_pid" || true
         sleep 0.1
     done
@@ -158,21 +166,26 @@ function stop_server() {
 }
 
 function get_server_pid() {
+    # shellcheck source=/dev/null
     source "$1"
     echo "$token_pid"
 }
 
 function get_server_port() {
+    # shellcheck source=/dev/null
     source "$1"
     echo "$token_port"
 }
 
 function get_server_ip() {
+    # shellcheck source=/dev/null
     source "$1"
+    # shellcheck disable=SC2154
     echo "$token_server_ip"
 }
 
 function do_curl() {
+    # shellcheck source=/dev/null
     source "$1"
     shift
     case "$token_type" in

IntegrationTests/tests_01_http/test_01_get_file.sh

@@ -13,6 +13,7 @@
 ##
 ##===----------------------------------------------------------------------===##
 
+# shellcheck source=IntegrationTests/tests_01_http/defines.sh
 source defines.sh
 
 token=$(create_token)
@@ -22,7 +23,7 @@ echo FOO BAR > "$htdocs/some_file.txt"
 touch "$htdocs/empty"
 for file in some_file.txt empty; do
     for method in sendfile fileio; do
-        do_curl "$token" "http://foobar.com/$method/$file" > "$tmp/out.txt"
+        do_curl "$token" "http://foobar.com/$method/$file" > "${tmp:?"tmp variable not set"}/out.txt"
         assert_equal_files "$htdocs/$file" "$tmp/out.txt"
     done
 done

IntegrationTests/tests_01_http/test_02_get_random_bytes.sh

@@ -13,6 +13,7 @@
 ##
 ##===----------------------------------------------------------------------===##
 
+# shellcheck source=IntegrationTests/tests_01_http/defines.sh
 source defines.sh
 
 token=$(create_token)
@@ -22,7 +23,7 @@ base="s/o/m/e/r/a/n/d/o/m/f/o/l/d/e/r"
 mkdir -p "$htdocs/$base"
 dd if=/dev/urandom of="$htdocs/$base/random.bytes" bs=$((1024 * 1024)) count=2
 for method in sendfile fileio; do
-    do_curl "$token" "http://foobar.com/$method/$base/random.bytes" > "$tmp/random.bytes"
+    do_curl "$token" "http://foobar.com/$method/$base/random.bytes" > "${tmp:?"tmp variable not set"}/random.bytes"
     assert_equal_files "$htdocs/$base/random.bytes" "$tmp/random.bytes"
 done
 stop_server "$token"

IntegrationTests/tests_01_http/test_03_post_random_bytes.sh

@@ -13,11 +13,12 @@
 ##
 ##===----------------------------------------------------------------------===##
 
+# shellcheck source=IntegrationTests/tests_01_http/defines.sh
 source defines.sh
 
 token=$(create_token)
 start_server "$token"
-dd if=/dev/urandom of="$tmp/random.bytes" bs=$((64*1024)) count=1
+dd if=/dev/urandom of="${tmp:?"tmp variable not set"}/random.bytes" bs=$((64*1024)) count=1
 do_curl "$token" --data-binary "@$tmp/random.bytes" \
     "http://foobar.com/dynamic/echo" > "$tmp/random.bytes.out"
 cmp "$tmp/random.bytes" "$tmp/random.bytes.out"

IntegrationTests/tests_01_http/test_04_keep_alive_works.sh

@@ -13,14 +13,15 @@
 ##
 ##===----------------------------------------------------------------------===##
 
+# shellcheck source=IntegrationTests/tests_01_http/defines.sh
 source defines.sh
 
 token=$(create_token)
 start_server "$token"
 server_pid=$(get_server_pid "$token")
 echo -n \
     "$server_pid$server_pid$server_pid$server_pid$server_pid$server_pid$server_pid$server_pid$server_pid$server_pid" \
-    > "$tmp/out_expected"
+    > "${tmp:?"tmp variable not set"}/out_expected"
 do_curl "$token" \
     "http://foobar.com/dynamic/pid" \
     "http://foobar.com/dynamic/pid" \

IntegrationTests/tests_01_http/test_05_repeated_reqs_work.sh

@@ -13,12 +13,13 @@
 ##
 ##===----------------------------------------------------------------------===##
 
+# shellcheck source=IntegrationTests/tests_01_http/defines.sh
 source defines.sh
 
 token=$(create_token)
 start_server "$token"
-do_curl "$token" "http://foobar.com/dynamic/pid" > "$tmp/out"
-for f in $(seq 100); do
+do_curl "$token" "http://foobar.com/dynamic/pid" > "${tmp:?"tmp variable not set"}/out"
+for _ in $(seq 100); do
     do_curl "$token" "http://foobar.com/dynamic/pid" > "$tmp/out2"
     assert_equal_files "$tmp/out" "$tmp/out2"
 done

IntegrationTests/tests_01_http/test_06_http_1.0.sh

@@ -13,11 +13,12 @@
 ##
 ##===----------------------------------------------------------------------===##
 
+# shellcheck source=IntegrationTests/tests_01_http/defines.sh
 source defines.sh
 
 token=$(create_token)
 start_server "$token"
-echo foo > "$tmp/out_expected"
+echo foo > "${tmp:?"tmp variable not set"}/out_expected"
 do_curl "$token" --data-binary "@$tmp/out_expected" --http1.0 \
     "http://foobar.com/dynamic/echo_balloon" > "$tmp/out_actual"
 assert_equal_files "$tmp/out_expected" "$tmp/out_actual"

IntegrationTests/tests_01_http/test_07_headers_work.sh

@@ -13,12 +13,13 @@
 ##
 ##===----------------------------------------------------------------------===##
 
+# shellcheck source=IntegrationTests/tests_01_http/defines.sh
 source defines.sh
 
 token=$(create_token)
 start_server "$token"
 do_curl "$token" -H "foo: bar" --http1.0 \
-    "http://foobar.com/dynamic/info" > "$tmp/out"
+    "http://foobar.com/dynamic/info" > "${tmp:?"tmp variable not set"}/out"
 if ! grep -q '("foo", "bar")' "$tmp/out"; then
     fail "couldn't find header in response"
 fi

IntegrationTests/tests_01_http/test_08_survive_signals.sh

@@ -13,6 +13,7 @@
 ##
 ##===----------------------------------------------------------------------===##
 
+# shellcheck source=IntegrationTests/tests_01_http/defines.sh
 source defines.sh
 
 token=$(create_token)
@@ -21,16 +22,16 @@ htdocs=$(get_htdocs "$token")
 server_pid=$(get_server_pid "$token")
 echo FOO BAR > "$htdocs/some_file.txt"
 
-for f in $(seq 20); do
+for _ in $(seq 20); do
     # send some signals that are usually discarded
     kill -CHLD "$server_pid" # ignore-unacceptable-language
     kill -URG "$server_pid" # ignore-unacceptable-language
     kill -CONT "$server_pid" # ignore-unacceptable-language
     kill -WINCH "$server_pid" # ignore-unacceptable-language
 
-    do_curl "$token" "http://foobar.com/fileio/some_file.txt" > "$tmp/out.txt" &
+    do_curl "$token" "http://foobar.com/fileio/some_file.txt" > "${tmp:?"tmp variable not set"}/out.txt" &
     curl_pid=$!
-    for g in $(seq 20); do
+    for _ in $(seq 20); do
         kill -URG "$server_pid" # ignore-unacceptable-language
     done
     wait $curl_pid

IntegrationTests/tests_01_http/test_09_curl_happy_with_trailers.sh

@@ -13,6 +13,7 @@
 ##
 ##===----------------------------------------------------------------------===##
 
+# shellcheck source=IntegrationTests/tests_01_http/defines.sh
 source defines.sh
 
 token=$(create_token)
@@ -31,6 +32,6 @@ do_curl "$token" \
     "http://foobar.com/dynamic/trailers" \
     "http://foobar.com/dynamic/trailers" \
     "http://foobar.com/dynamic/trailers" \
-    > "$tmp/out.txt"
-assert_equal_files "$htdocs/some_file.txt" "$tmp/out.txt"
+    > "${tmp:?"tmp variable not set"}/out.txt"
+assert_equal_files "$htdocs/some_file.txt" "${tmp}/out.txt"
 stop_server "$token"