diff mbox series

[1/2] selftests/harness: Run TEARDOWN for ASSERT failures

Message ID 20220324231907.1363887-2-keescook@chromium.org
State Accepted
Commit 63e6b2a42342c3297cce286fb124c99be9e0f3fd
Headers show
Series selftests/harness: Run TEARDOWN for ASSERT failures | expand

Commit Message

Kees Cook March 24, 2022, 11:19 p.m. UTC
The kselftest test harness has traditionally not run the registered
TEARDOWN handler when a test encountered an ASSERT. This creates
unexpected situations and tests need to be very careful about using
ASSERT, which seems a needless hurdle for test writers.

Because of the harness's design for optional failure handlers, the
original implementation of ASSERT used an abort() to immediately
stop execution, but that meant the context for running teardown was
lost. Instead, use setjmp/longjmp so that teardown can be done.

Failed SETUP routines continue to not be followed by TEARDOWN, though.

Cc: Andy Lutomirski <luto@amacapital.net>
Cc: Will Drewry <wad@chromium.org>
Cc: Shuah Khan <shuah@kernel.org>
Cc: linux-kselftest@vger.kernel.org
Signed-off-by: Kees Cook <keescook@chromium.org>
---
 tools/testing/selftests/kselftest_harness.h | 49 ++++++++++++++-------
 1 file changed, 34 insertions(+), 15 deletions(-)

Comments

Shuah Khan March 25, 2022, 7:37 p.m. UTC | #1
On 3/24/22 5:19 PM, Kees Cook wrote:
> The kselftest test harness has traditionally not run the registered
> TEARDOWN handler when a test encountered an ASSERT. This creates
> unexpected situations and tests need to be very careful about using
> ASSERT, which seems a needless hurdle for test writers.
> 
> Because of the harness's design for optional failure handlers, the
> original implementation of ASSERT used an abort() to immediately
> stop execution, but that meant the context for running teardown was
> lost. Instead, use setjmp/longjmp so that teardown can be done.
> 

Thanks for the patch. The change look good to me.

> Failed SETUP routines continue to not be followed by TEARDOWN, though.

Does this mean failed setup() routines have to handle TEARDOWN? What
are guidelines to follow for setup() failures?

Can you add a bit more detail on what you meant by " Failed SETUP
routines continue to not be followed by TEARDOWN, though".

With that:

Reviewed-by: Shuah Khan <skhan@linuxfoundation.org>

thanks,
-- Shuah
Kees Cook March 25, 2022, 10:36 p.m. UTC | #2
On Fri, Mar 25, 2022 at 01:37:20PM -0600, Shuah Khan wrote:
> On 3/24/22 5:19 PM, Kees Cook wrote:
> > The kselftest test harness has traditionally not run the registered
> > TEARDOWN handler when a test encountered an ASSERT. This creates
> > unexpected situations and tests need to be very careful about using
> > ASSERT, which seems a needless hurdle for test writers.
> > 
> > Because of the harness's design for optional failure handlers, the
> > original implementation of ASSERT used an abort() to immediately
> > stop execution, but that meant the context for running teardown was
> > lost. Instead, use setjmp/longjmp so that teardown can be done.
> > 
> 
> Thanks for the patch. The change look good to me.
> 
> > Failed SETUP routines continue to not be followed by TEARDOWN, though.
> 
> Does this mean failed setup() routines have to handle TEARDOWN? What
> are guidelines to follow for setup() failures?
> 
> Can you add a bit more detail on what you meant by " Failed SETUP
> routines continue to not be followed by TEARDOWN, though".

Sure! It means that any failures in a SETUP need to be cleaned up by the
SETUP, as TEARDOWN won't be run. (As in, this is unchanged from how
things behaved prior to this patch.)

> 
> With that:
> 
> Reviewed-by: Shuah Khan <skhan@linuxfoundation.org>

Thanks!
diff mbox series

Patch

diff --git a/tools/testing/selftests/kselftest_harness.h b/tools/testing/selftests/kselftest_harness.h
index 471eaa7b3a3f..bef08f824eb5 100644
--- a/tools/testing/selftests/kselftest_harness.h
+++ b/tools/testing/selftests/kselftest_harness.h
@@ -64,6 +64,7 @@ 
 #include <sys/types.h>
 #include <sys/wait.h>
 #include <unistd.h>
+#include <setjmp.h>
 
 #include "kselftest.h"
 
@@ -183,7 +184,10 @@ 
 		struct __test_metadata *_metadata, \
 		struct __fixture_variant_metadata *variant) \
 	{ \
-		test_name(_metadata); \
+		_metadata->setup_completed = true; \
+		if (setjmp(_metadata->env) == 0) \
+			test_name(_metadata); \
+		__test_check_assert(_metadata); \
 	} \
 	static struct __test_metadata _##test_name##_object = \
 		{ .name = #test_name, \
@@ -356,10 +360,7 @@ 
  * Defines a test that depends on a fixture (e.g., is part of a test case).
  * Very similar to TEST() except that *self* is the setup instance of fixture's
  * datatype exposed for use by the implementation.
- *
- * Warning: use of ASSERT_* here will skip TEARDOWN.
  */
-/* TODO(wad) register fixtures on dedicated test lists. */
 #define TEST_F(fixture_name, test_name) \
 	__TEST_F_IMPL(fixture_name, test_name, -1, TEST_TIMEOUT_DEFAULT)
 
@@ -381,12 +382,17 @@ 
 		/* fixture data is alloced, setup, and torn down per call. */ \
 		FIXTURE_DATA(fixture_name) self; \
 		memset(&self, 0, sizeof(FIXTURE_DATA(fixture_name))); \
-		fixture_name##_setup(_metadata, &self, variant->data); \
-		/* Let setup failure terminate early. */ \
-		if (!_metadata->passed) \
-			return; \
-		fixture_name##_##test_name(_metadata, &self, variant->data); \
-		fixture_name##_teardown(_metadata, &self); \
+		if (setjmp(_metadata->env) == 0) { \
+			fixture_name##_setup(_metadata, &self, variant->data); \
+			/* Let setup failure terminate early. */ \
+			if (!_metadata->passed) \
+				return; \
+			_metadata->setup_completed = true; \
+			fixture_name##_##test_name(_metadata, &self, variant->data); \
+		} \
+		if (_metadata->setup_completed) \
+			fixture_name##_teardown(_metadata, &self); \
+		__test_check_assert(_metadata); \
 	} \
 	static struct __test_metadata \
 		      _##fixture_name##_##test_name##_object = { \
@@ -683,7 +689,7 @@ 
  */
 #define OPTIONAL_HANDLER(_assert) \
 	for (; _metadata->trigger; _metadata->trigger = \
-			__bail(_assert, _metadata->no_print, _metadata->step))
+			__bail(_assert, _metadata))
 
 #define __INC_STEP(_metadata) \
 	/* Keep "step" below 255 (which is used for "SKIP" reporting). */	\
@@ -830,6 +836,9 @@  struct __test_metadata {
 	bool timed_out;	/* did this test timeout instead of exiting? */
 	__u8 step;
 	bool no_print; /* manual trigger when TH_LOG_STREAM is not available */
+	bool aborted;	/* stopped test due to failed ASSERT */
+	bool setup_completed; /* did setup finish? */
+	jmp_buf env;	/* for exiting out of test early */
 	struct __test_results *results;
 	struct __test_metadata *prev, *next;
 };
@@ -848,16 +857,26 @@  static inline void __register_test(struct __test_metadata *t)
 	__LIST_APPEND(t->fixture->tests, t);
 }
 
-static inline int __bail(int for_realz, bool no_print, __u8 step)
+static inline int __bail(int for_realz, struct __test_metadata *t)
 {
+	/* if this is ASSERT, return immediately. */
 	if (for_realz) {
-		if (no_print)
-			_exit(step);
-		abort();
+		t->aborted = true;
+		longjmp(t->env, 1);
 	}
+	/* otherwise, end the for loop and continue. */
 	return 0;
 }
 
+static inline void __test_check_assert(struct __test_metadata *t)
+{
+	if (t->aborted) {
+		if (t->no_print)
+			_exit(t->step);
+		abort();
+	}
+}
+
 struct __test_metadata *__active_test;
 static void __timeout_handler(int sig, siginfo_t *info, void *ucontext)
 {