Re: [PATCH] selftests/kselftest/runner.sh: Add 30 second timeout per test

[Kees Cook <keescook@chromium.org>]

On Wed, Jun 12, 2019 at 06:41:46PM +0200, Anders Roxell wrote:
> Commit a745f7af3cbd ("selftests/harness: Add 30 second timeout per
> test") solves that binary tests doesn't hang forever. However, scripts
> can still hang forever, this adds an timeout to each test script run. This
> assumes that an individual test doesn't take longer than 30 seconds.
> 
> Signed-off-by: Anders Roxell <anders.roxell@linaro.org>
> ---
>  tools/testing/selftests/kselftest/runner.sh | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/testing/selftests/kselftest/runner.sh b/tools/testing/selftests/kselftest/runner.sh
> index 00c9020bdda8..cff7d2d83648 100644
> --- a/tools/testing/selftests/kselftest/runner.sh
> +++ b/tools/testing/selftests/kselftest/runner.sh
> @@ -5,6 +5,7 @@
>  export skip_rc=4
>  export logfile=/dev/stdout
>  export per_test_logging=
> +export TEST_TIMEOUT_DEFAULT=30

I would name this "kselftest_timeout" to avoid namespace collisions and
to drop the "default" name, since while setting it here is a default,
when used by other parts of the test environment, we'll be changing it
to non-default, so it's weird to have "default" in the name.

>  
>  # There isn't a shell-agnostic way to find the path of a sourced file,
>  # so we must rely on BASE_DIR being set to find other tools.
> @@ -24,6 +25,14 @@ tap_prefix()
>  	fi
>  }
>  
> +tap_timeout()
> +{
> +	if [ -x /usr/bin/timeout ] && [ -x "$BASENAME_TEST" ] \

The -x test on BASENAME_TEST is already done earlier in run_one(). Also,
this needs to be "$1" (the argument to tap_timeout).

> +		&& file $BASENAME_TEST |grep -q "shell script"; then

"file" seems pretty heavy here. How about "head -c2":

		&& [ '#!' = "$(head -c2 "$1")" ]

> +		echo -n "timeout $TEST_TIMEOUT_DEFAULT"

And this needs to actually _run_ it, not echo anything.

> +	fi
> +}

I would expect tap_timeout() to be:

tap_timeout()
{
	# If we have the "timeout" tool and the test is a script,
	# set our timeout.
	if [ -x /usr/bin/timeout ] &&
	   [ '#!' = "$(head -c2 "$1")" ] ; then
		/usr/bin/timeout "$kselftest_timeout" "$1"
	else
		"$1"
	fi
}

Another thought: should kselftest be changed so that this is the ONLY
timeout mechanism? i.e. remove the binary timeout code I originally
added, and just use this instead? Then we don't have to distinguish
between script and non-script, etc.

-Kees

> +
>  run_one()
>  {
>  	DIR="$1"
> @@ -44,7 +53,7 @@ run_one()
>  		echo "not ok $test_num $TEST_HDR_MSG"
>  	else
>  		cd `dirname $TEST` > /dev/null
> -		(((((./$BASENAME_TEST 2>&1; echo $? >&3) |
> +		((((( tap_timeout ./$BASENAME_TEST 2>&1; echo $? >&3) |
>  			tap_prefix >&4) 3>&1) |
>  			(read xs; exit $xs)) 4>>"$logfile" &&
>  		echo "ok $test_num $TEST_HDR_MSG") ||
> -- 
> 2.11.0
> 

-- 
Kees Cook