[PATCH] kunit: Check for kunit_parse_glob_filter() failure

[Dan Carpenter]

Smatch complains that the missing error checks would lead to a crash:

lib/kunit/executor_test.c:40 parse_filter_test()
error: double free of 'filter.test_glob'

We may as well do it right...

Fixes: a127b154a8f2 ("kunit: tool: allow filtering test cases via glob")
Signed-off-by: Dan Carpenter <dan.ca...@linaro.org>
---
lib/kunit/executor_test.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/lib/kunit/executor_test.c b/lib/kunit/executor_test.c
index b4f6f96b2844..176c9c9dfcfc 100644
--- a/lib/kunit/executor_test.c
+++ b/lib/kunit/executor_test.c
@@ -27,13 +27,15 @@ static void parse_filter_test(struct kunit *test)
{
struct kunit_glob_filter filter = {NULL, NULL};

- kunit_parse_glob_filter(&filter, "suite");
+ if (!kunit_parse_glob_filter(&filter, "suite"))
+ return;
KUNIT_EXPECT_STREQ(test, filter.suite_glob, "suite");
KUNIT_EXPECT_FALSE(test, filter.test_glob);
kfree(filter.suite_glob);
kfree(filter.test_glob);

- kunit_parse_glob_filter(&filter, "suite.test");
+ if (!kunit_parse_glob_filter(&filter, "suite.test"))
+ return;
KUNIT_EXPECT_STREQ(test, filter.suite_glob, "suite");
KUNIT_EXPECT_STREQ(test, filter.test_glob, "test");
kfree(filter.suite_glob);
--
2.39.2

[Rae Moar]

On Fri, Sep 15, 2023 at 8:58 AM Dan Carpenter <dan.ca...@linaro.org> wrote:
>
> Smatch complains that the missing error checks would lead to a crash:
>
> lib/kunit/executor_test.c:40 parse_filter_test()
> error: double free of 'filter.test_glob'
>
> We may as well do it right...
>
> Fixes: a127b154a8f2 ("kunit: tool: allow filtering test cases via glob")
> Signed-off-by: Dan Carpenter <dan.ca...@linaro.org>

Hello!

We definitely should add in checks for these errors. I have a couple
comments below.

Thanks!
-Rae

> ---
> lib/kunit/executor_test.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/lib/kunit/executor_test.c b/lib/kunit/executor_test.c
> index b4f6f96b2844..176c9c9dfcfc 100644
> --- a/lib/kunit/executor_test.c
> +++ b/lib/kunit/executor_test.c
> @@ -27,13 +27,15 @@ static void parse_filter_test(struct kunit *test)
> {
> struct kunit_glob_filter filter = {NULL, NULL};
>
> - kunit_parse_glob_filter(&filter, "suite");
> + if (!kunit_parse_glob_filter(&filter, "suite"))
> + return;

First, this is returning every time this test is run because the
kunit_parse_glob_filter returns 0 when there is no error. So this
should instead be checking for a result of not equal to 0.

Secondly, this should fail the test if the parsing returns an error.
So instead of returning I would recommend using a KUNIT_ASSERT
statement to check the result of this method is equal to 0.

> KUNIT_EXPECT_STREQ(test, filter.suite_glob, "suite");
> KUNIT_EXPECT_FALSE(test, filter.test_glob);
> kfree(filter.suite_glob);
> kfree(filter.test_glob);
>
> - kunit_parse_glob_filter(&filter, "suite.test");
> + if (!kunit_parse_glob_filter(&filter, "suite.test"))
> + return;

Similar to above I think this should be changed to a KUNIT_ASSERT
statement to ensure the result is equal to 0.

> KUNIT_EXPECT_STREQ(test, filter.suite_glob, "suite");
> KUNIT_EXPECT_STREQ(test, filter.test_glob, "test");
> kfree(filter.suite_glob);
> --
> 2.39.2
>

> --
> You received this message because you are subscribed to the Google Groups "KUnit Development" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to kunit-dev+...@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/kunit-dev/3ff9d019-75b6-45ee-9b03-9d6ec7c5364f%40moroto.mountain.

[Dan Carpenter]

Oh... Duh. Sorry. That's embarrassing.

>
> Secondly, this should fail the test if the parsing returns an error.
> So instead of returning I would recommend using a KUNIT_ASSERT
> statement to check the result of this method is equal to 0.

Will do. Thanks!

regards,
dan carpenter