LinuxLists.cc - [PATCH] selftests/harness: Prevent infinite loop due to Assert in FIXTURE

2024-03-14 22:44:15

Subject: [PATCH] selftests/harness: Prevent infinite loop due to Assert in FIXTURE_TEARDOWN

2024-03-24 21:45:34

Subject: [PATCH] selftests/harness: Prevent infinite loop due to Assert in FIXTURE_TEARDOWN

This patch addresses an issue in the selftests/harness where an assertion within FIXTURE_TEARDOWN could trigger an infinite loop.
The problem arises because the teardown procedure is meant to execute once,
but the presence of failing assertions (ASSERT_EQ(0, 1))
leads to repeated attempts to execute teardown due to the long jump mechanism used by the harness for handling assertions.

To resolve this, the patch ensures that the teardown process runs only once, regardless of assertion outcomes,
preventing the infinite loop and allowing tests to fail.

A simple test demo, also this is related to the issue mentioned in this patch
https://patchwork.kernel.org/project/linux-kselftest/patch/[email protected]/

#include "kselftest_harness.h"

FIXTURE(f)
{
int fd;
};

FIXTURE_SETUP(f)
{
self->fd = 0;
}

FIXTURE_TEARDOWN(f)
{
TH_LOG("TEARDOWN");
ASSERT_EQ(0, 1);
self->fd = -1;
}

TEST_F(f, open_close)
{
ASSERT_NE(self->fd, 1);
}

TEST_HARNESS_MAIN

Signed-off-by: Shengyu Li <[email protected]>
---
tools/testing/selftests/kselftest_harness.h | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/kselftest_harness.h b/tools/testing/selftests/kselftest_harness.h
index 4fd735e48ee7..230d62884885 100644
--- a/tools/testing/selftests/kselftest_harness.h
+++ b/tools/testing/selftests/kselftest_harness.h
@@ -383,6 +383,7 @@
FIXTURE_DATA(fixture_name) self; \
pid_t child = 1; \
int status = 0; \
+ bool jmp = false; \
memset(&self, 0, sizeof(FIXTURE_DATA(fixture_name))); \
if (setjmp(_metadata->env) == 0) { \
/* Use the same _metadata. */ \
@@ -399,8 +400,10 @@
_metadata->exit_code = KSFT_FAIL; \
} \
} \
+ else \
+ jmp = true; \
if (child == 0) { \
- if (_metadata->setup_completed && !_metadata->teardown_parent) \
+ if (_metadata->setup_completed && !_metadata->teardown_parent && !jmp) \
fixture_name##_teardown(_metadata, &self, variant->data); \
_exit(0); \
} \
--
2.25.1

2024-03-26 19:28:51

by Shuah Khan

[permalink] [raw]

Subject: Re: [PATCH] selftests/harness: Prevent infinite loop due to Assert in FIXTURE_TEARDOWN

On 3/14/24 16:43, Shengyu Li wrote:
> This patch addresses an issue in the selftests/harness where an assertion within FIXTURE_TEARDOWN could trigger an infinite loop. The problem arises because the teardown procedure is meant to execute once, but the presence of failing assertions (ASSERT_EQ(0, 1)) leads to repeated attempts to execute teardown due to the long jump mechanism used by the harness for handling assertions.
>
> To resolve this, the patch ensures that the teardown process runs only once, regardless of assertion outcomes, preventing the infinite loop and allowing tests to fail.
>

Please make sure change log is formatted with 70-75 character
long lines.

Please include information on how you found this problem.

Send v2 with that change.

> Signed-off-by: Shengyu Li <[email protected]>
> ---
> tools/testing/selftests/kselftest_harness.h | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/tools/testing/selftests/kselftest_harness.h b/tools/testing/selftests/kselftest_harness.h
> index 4fd735e48ee7..230d62884885 100644
> --- a/tools/testing/selftests/kselftest_harness.h
> +++ b/tools/testing/selftests/kselftest_harness.h
> @@ -383,6 +383,7 @@
> FIXTURE_DATA(fixture_name) self; \
> pid_t child = 1; \
> int status = 0; \
> + bool jmp = false; \
> memset(&self, 0, sizeof(FIXTURE_DATA(fixture_name))); \
> if (setjmp(_metadata->env) == 0) { \
> /* Use the same _metadata. */ \
> @@ -399,8 +400,10 @@
> _metadata->exit_code = KSFT_FAIL; \
> } \
> } \
> + else \
> + jmp = true; \
> if (child == 0) { \
> - if (_metadata->setup_completed && !_metadata->teardown_parent) \
> + if (_metadata->setup_completed && !_metadata->teardown_parent && !jmp) \
> fixture_name##_teardown(_metadata, &self, variant->data); \
> _exit(0); \
> } \

thanks,
-- Shuah

2024-03-26 21:13:44

by Shengyu Li

[permalink] [raw]

Subject: [PATCH v2] selftests/harness: Prevent infinite loop due to Assert in FIXTURE_TEARDOWN

This patch addresses an issue in the selftests/harness where an
assertion within FIXTURE_TEARDOWN could trigger an infinite loop.
The problem arises because the teardown procedure is meant to
execute once, but the presence of failing assertions (ASSERT_EQ(0, 1))
leads to repeated attempts to execute teardown due to
the long jump mechanism used by the harness for handling assertions.

To resolve this, the patch ensures that the teardown process
runs only once, regardless of assertion outcomes, preventing
the infinite loop and allowing tests to fail.

A simple test demo(test.c):
#include "kselftest_harness.h"

FIXTURE(f)
{
int fd;
};

FIXTURE_SETUP(f)
{
self->fd = 0;
}

FIXTURE_TEARDOWN(f)
{
TH_LOG("TEARDOWN");
ASSERT_EQ(0, 1);
self->fd = -1;
}

TEST_F(f, open_close)
{
ASSERT_NE(self->fd, 1);
}

TEST_HARNESS_MAIN

will always output the following output due to a dead loop until timeout:
# test.c:15:open_close:TEARDOWN
# test.c:16:open_close:Expected 0 (0) == 1 (1)
# test.c:15:open_close:TEARDOWN
# test.c:16:open_close:Expected 0 (0) == 1 (1)
...

But here's what we should and expect to get:
TAP version 13
1..1
# Starting 1 tests from 2 test cases.
# RUN f.open_close ...
# test.c:15:open_close:TEARDOWN
# test.c:16:open_close:Expected 0 (0) == 1 (1)
# open_close: Test terminated by assertion
# FAIL f.open_close
not ok 1 f.open_close
# FAILED: 0 / 1 tests passed.
# Totals: pass:0 fail:1 xfail:0 xpass:0 skip:0 error:0

also this is related to the issue mentioned in this patch
https://patchwork.kernel.org/project/linux-kselftest/patch/[email protected]/

Signed-off-by: Shengyu Li <[email protected]>
---
tools/testing/selftests/kselftest_harness.h | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/kselftest_harness.h b/tools/testing/selftests/kselftest_harness.h
index 4fd735e48ee7..230d62884885 100644
--- a/tools/testing/selftests/kselftest_harness.h
+++ b/tools/testing/selftests/kselftest_harness.h
@@ -383,6 +383,7 @@
FIXTURE_DATA(fixture_name) self; \
pid_t child = 1; \
int status = 0; \
+ bool jmp = false; \
memset(&self, 0, sizeof(FIXTURE_DATA(fixture_name))); \
if (setjmp(_metadata->env) == 0) { \
/* Use the same _metadata. */ \
@@ -399,8 +400,10 @@
_metadata->exit_code = KSFT_FAIL; \
} \
} \
+ else \
+ jmp = true; \
if (child == 0) { \
- if (_metadata->setup_completed && !_metadata->teardown_parent) \
+ if (_metadata->setup_completed && !_metadata->teardown_parent && !jmp) \
fixture_name##_teardown(_metadata, &self, variant->data); \
_exit(0); \
} \
--
2.25.1

2024-04-04 16:48:09

by Shuah Khan

[permalink] [raw]

Subject: Re: [PATCH v2] selftests/harness: Prevent infinite loop due to Assert in FIXTURE_TEARDOWN

On 3/26/24 15:13, Shengyu Li wrote:
> This patch addresses an issue in the selftests/harness where an
> assertion within FIXTURE_TEARDOWN could trigger an infinite loop.
> The problem arises because the teardown procedure is meant to
> execute once, but the presence of failing assertions (ASSERT_EQ(0, 1))
> leads to repeated attempts to execute teardown due to
> the long jump mechanism used by the harness for handling assertions.
>
> To resolve this, the patch ensures that the teardown process
> runs only once, regardless of assertion outcomes, preventing
> the infinite loop and allowing tests to fail.
>
> A simple test demo(test.c):
> #include "kselftest_harness.h"
>
> FIXTURE(f)
> {
> int fd;
> };
>
> FIXTURE_SETUP(f)
> {
> self->fd = 0;
> }
>
> FIXTURE_TEARDOWN(f)
> {
> TH_LOG("TEARDOWN");
> ASSERT_EQ(0, 1);
> self->fd = -1;
> }
>
> TEST_F(f, open_close)
> {
> ASSERT_NE(self->fd, 1);
> }
>
> TEST_HARNESS_MAIN
>
> will always output the following output due to a dead loop until timeout:
> # test.c:15:open_close:TEARDOWN
> # test.c:16:open_close:Expected 0 (0) == 1 (1)
> # test.c:15:open_close:TEARDOWN
> # test.c:16:open_close:Expected 0 (0) == 1 (1)
> ...
>
> But here's what we should and expect to get:
> TAP version 13
> 1..1
> # Starting 1 tests from 2 test cases.
> # RUN f.open_close ...
> # test.c:15:open_close:TEARDOWN
> # test.c:16:open_close:Expected 0 (0) == 1 (1)
> # open_close: Test terminated by assertion
> # FAIL f.open_close
> not ok 1 f.open_close
> # FAILED: 0 / 1 tests passed.
> # Totals: pass:0 fail:1 xfail:0 xpass:0 skip:0 error:0
>
> also this is related to the issue mentioned in this patch
> https://patchwork.kernel.org/project/linux-kselftest/patch/[email protected]/
>

Adding people who participated in the above thread discussion.
Please try this patch out to see if this fixes the problem
with iommu test.

>
>
> Signed-off-by: Shengyu Li <[email protected]>

Than you for finding and fixing this. Applied now to linux-kselftes fixes
branch for the next rc.

thanks,
-- Shuah