Re: [PATCH 4/4] kunit: test: Fix the possible memory leak in executor_test

From: Rae Moar <rmoar@google.com>
To: Jinjie Ruan <ruanjinjie@huawei.com>
Cc: brendan.higgins@linux.dev, davidgow@google.com,
	skhan@linuxfoundation.org, dlatypov@google.com,
	janusz.krzysztofik@linux.intel.com,
	linux-kselftest@vger.kernel.org, kunit-dev@googlegroups.com
Subject: Re: [PATCH 4/4] kunit: test: Fix the possible memory leak in executor_test
Date: Tue, 19 Sep 2023 17:19:26 -0400	[thread overview]
Message-ID: <CA+GJov7Uya8r4T6EtQQ03KQ7BPRsCb8N3y687eDxDitnJ3Aifw@mail.gmail.com> (raw)
In-Reply-To: <20230914114629.1517650-5-ruanjinjie@huawei.com>

On Thu, Sep 14, 2023 at 7:47 AM 'Jinjie Ruan' via KUnit Development
<kunit-dev@googlegroups.com> wrote:
>
> If kunit_filter_suites() succeeds, not only copy but also filtered_suite
> and filtered_suite->test_cases should be freed.
>
> So use kunit_free_suite_set() to free the filtered_suite,
> filtered_suite->test_cases and copy as kunit_module_exit() and
> kunit_run_all_tests() do it.
>
> Fixes: e5857d396f35 ("kunit: flatten kunit_suite*** to kunit_suite** in .kunit_test_suites")
> Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com>

Hello!

This looks mostly good to me. But I have one notable comment. See below.

Thanks!
-Rae

> ---
>  lib/kunit/executor_test.c | 24 ++++++++++++++++++------
>  1 file changed, 18 insertions(+), 6 deletions(-)
>
> diff --git a/lib/kunit/executor_test.c b/lib/kunit/executor_test.c
> index b4f6f96b2844..987b81dce01e 100644
> --- a/lib/kunit/executor_test.c
> +++ b/lib/kunit/executor_test.c
> @@ -56,7 +56,6 @@ static void filter_suites_test(struct kunit *test)
>         got = kunit_filter_suites(&suite_set, "suite2", NULL, NULL, &err);
>         KUNIT_ASSERT_NOT_ERR_OR_NULL(test, got.start);
>         KUNIT_ASSERT_EQ(test, err, 0);
> -       kfree_at_end(test, got.start);
>
>         /* Validate we just have suite2 */
>         KUNIT_ASSERT_NOT_ERR_OR_NULL(test, got.start[0]);
> @@ -64,6 +63,9 @@ static void filter_suites_test(struct kunit *test)
>
>         /* Contains one element (end is 1 past end) */
>         KUNIT_ASSERT_EQ(test, got.end - got.start, 1);
> +
> +       if (!err)
> +               kunit_free_suite_set(got);

I definitely appreciate the change to free all of "got" rather than
just "got.start".

However, kfree_at_end used deferred actions to ensure the kfree would
occur at the end of the test. With this change, if the test fails the
suite set could not be freed.

Intead, is there any way to alter the function kfree_at_end (could be
renamed) to take in "got" and then use deferred actions to ensure
kunit_free_suite_set is called at the end of the test?

Let me know what you think about this.

>  }
>
>  static void filter_suites_test_glob_test(struct kunit *test)
> @@ -82,7 +84,6 @@ static void filter_suites_test_glob_test(struct kunit *test)
>         got = kunit_filter_suites(&suite_set, "suite2.test2", NULL, NULL, &err);
>         KUNIT_ASSERT_NOT_ERR_OR_NULL(test, got.start);
>         KUNIT_ASSERT_EQ(test, err, 0);
> -       kfree_at_end(test, got.start);
>
>         /* Validate we just have suite2 */
>         KUNIT_ASSERT_NOT_ERR_OR_NULL(test, got.start[0]);
> @@ -93,6 +94,9 @@ static void filter_suites_test_glob_test(struct kunit *test)
>         KUNIT_ASSERT_NOT_ERR_OR_NULL(test, got.start[0]->test_cases);
>         KUNIT_EXPECT_STREQ(test, (const char *)got.start[0]->test_cases[0].name, "test2");
>         KUNIT_EXPECT_FALSE(test, got.start[0]->test_cases[1].name);
> +
> +       if (!err)
> +               kunit_free_suite_set(got);
>  }
>
>  static void filter_suites_to_empty_test(struct kunit *test)
> @@ -109,10 +113,12 @@ static void filter_suites_to_empty_test(struct kunit *test)
>
>         got = kunit_filter_suites(&suite_set, "not_found", NULL, NULL, &err);
>         KUNIT_ASSERT_EQ(test, err, 0);
> -       kfree_at_end(test, got.start); /* just in case */
>
>         KUNIT_EXPECT_PTR_EQ_MSG(test, got.start, got.end,
>                                 "should be empty to indicate no match");
> +
> +       if (!err)
> +               kunit_free_suite_set(got);
>  }
>
>  static void parse_filter_attr_test(struct kunit *test)
> @@ -172,7 +178,6 @@ static void filter_attr_test(struct kunit *test)
>         got = kunit_filter_suites(&suite_set, NULL, filter, NULL, &err);
>         KUNIT_ASSERT_NOT_ERR_OR_NULL(test, got.start);
>         KUNIT_ASSERT_EQ(test, err, 0);
> -       kfree_at_end(test, got.start);
>
>         /* Validate we just have normal_suite */
>         KUNIT_ASSERT_NOT_ERR_OR_NULL(test, got.start[0]);
> @@ -183,6 +188,9 @@ static void filter_attr_test(struct kunit *test)
>         KUNIT_ASSERT_NOT_ERR_OR_NULL(test, got.start[0]->test_cases);
>         KUNIT_EXPECT_STREQ(test, got.start[0]->test_cases[0].name, "normal");
>         KUNIT_EXPECT_FALSE(test, got.start[0]->test_cases[1].name);
> +
> +       if (!err)
> +               kunit_free_suite_set(got);
>  }
>
>  static void filter_attr_empty_test(struct kunit *test)
> @@ -200,10 +208,12 @@ static void filter_attr_empty_test(struct kunit *test)
>
>         got = kunit_filter_suites(&suite_set, NULL, filter, NULL, &err);
>         KUNIT_ASSERT_EQ(test, err, 0);
> -       kfree_at_end(test, got.start); /* just in case */
>
>         KUNIT_EXPECT_PTR_EQ_MSG(test, got.start, got.end,
>                                 "should be empty to indicate no match");
> +
> +       if (!err)
> +               kunit_free_suite_set(got);
>  }
>
>  static void filter_attr_skip_test(struct kunit *test)
> @@ -222,7 +232,6 @@ static void filter_attr_skip_test(struct kunit *test)
>         got = kunit_filter_suites(&suite_set, NULL, filter, "skip", &err);
>         KUNIT_ASSERT_NOT_ERR_OR_NULL(test, got.start);
>         KUNIT_ASSERT_EQ(test, err, 0);
> -       kfree_at_end(test, got.start);
>
>         /* Validate we have both the slow and normal test */
>         KUNIT_ASSERT_NOT_ERR_OR_NULL(test, got.start[0]->test_cases);
> @@ -233,6 +242,9 @@ static void filter_attr_skip_test(struct kunit *test)
>         /* Now ensure slow is skipped and normal is not */
>         KUNIT_EXPECT_EQ(test, got.start[0]->test_cases[0].status, KUNIT_SKIPPED);
>         KUNIT_EXPECT_FALSE(test, got.start[0]->test_cases[1].status);
> +
> +       if (!err)
> +               kunit_free_suite_set(got);
>  }
>
>  static struct kunit_case executor_test_cases[] = {
> --
> 2.34.1
>
> --
> 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+unsubscribe@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/kunit-dev/20230914114629.1517650-5-ruanjinjie%40huawei.com.