Re: [PATCH v3 3/4] kunit: Fix possible memory leak in kunit_filter_suites()

From: David Gow <davidgow@google.com>
To: Jinjie Ruan <ruanjinjie@huawei.com>
Cc: brendan.higgins@linux.dev, skhan@linuxfoundation.org,
	dlatypov@google.com, rmoar@google.com,
	janusz.krzysztofik@linux.intel.com,
	linux-kselftest@vger.kernel.org, kunit-dev@googlegroups.com
Subject: Re: [PATCH v3 3/4] kunit: Fix possible memory leak in kunit_filter_suites()
Date: Fri, 22 Sep 2023 15:47:57 +0800	[thread overview]
Message-ID: <CABVgOSkYP196nDscFaNkbDTLsWCTupDWubKTSB7jMnHz1_6_wg@mail.gmail.com> (raw)
In-Reply-To: <20230922071020.2554677-4-ruanjinjie@huawei.com>

[-- Attachment #1: Type: text/plain, Size: 4105 bytes --]

On Fri, 22 Sept 2023 at 15:11, 'Jinjie Ruan' via KUnit Development
<kunit-dev@googlegroups.com> wrote:
>
> If the outer layer for loop is iterated more than once and it fails not
> in the first iteration, the filtered_suite and filtered_suite->test_cases
> allocated in the last kunit_filter_attr_tests() in last inner for loop
> is leaked.
>
> So add a new free_filtered_suite err label and free the filtered_suite
> and filtered_suite->test_cases so far. And change kmalloc_array of copy
> to kcalloc to Clear the copy to make the kfree safe.
>
> Fixes: 5d31f71efcb6 ("kunit: add kunit.filter_glob cmdline option to filter suites")
> Fixes: 529534e8cba3 ("kunit: Add ability to filter attributes")
> Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com>
> Reviewed-by: Rae Moar <rmoar@google.com>
> ---
> v2:
> - Add Reviewed-by.
> ---

This looks good to me. There are a couple of things in this code which
probably could be cleaned up (as I noted in v2), but that's something
we can do separately.

Reviewed-by: David Gow <davidgow@google.com>

Cheers,
-- David

>  lib/kunit/executor.c | 17 +++++++++++++----
>  1 file changed, 13 insertions(+), 4 deletions(-)
>
> diff --git a/lib/kunit/executor.c b/lib/kunit/executor.c
> index 9358ed2df839..1236b3cd2fbb 100644
> --- a/lib/kunit/executor.c
> +++ b/lib/kunit/executor.c
> @@ -157,10 +157,11 @@ kunit_filter_suites(const struct kunit_suite_set *suite_set,
>         struct kunit_suite_set filtered = {NULL, NULL};
>         struct kunit_glob_filter parsed_glob;
>         struct kunit_attr_filter *parsed_filters = NULL;
> +       struct kunit_suite * const *suites;
>
>         const size_t max = suite_set->end - suite_set->start;
>
> -       copy = kmalloc_array(max, sizeof(*filtered.start), GFP_KERNEL);
> +       copy = kcalloc(max, sizeof(*filtered.start), GFP_KERNEL);
>         if (!copy) { /* won't be able to run anything, return an empty set */
>                 return filtered;
>         }
> @@ -195,7 +196,7 @@ kunit_filter_suites(const struct kunit_suite_set *suite_set,
>                                         parsed_glob.test_glob);
>                         if (IS_ERR(filtered_suite)) {
>                                 *err = PTR_ERR(filtered_suite);
> -                               goto free_parsed_filters;
> +                               goto free_filtered_suite;
>                         }
>                 }
>                 if (filter_count > 0 && parsed_filters != NULL) {
> @@ -212,11 +213,11 @@ kunit_filter_suites(const struct kunit_suite_set *suite_set,
>                                 filtered_suite = new_filtered_suite;
>
>                                 if (*err)
> -                                       goto free_parsed_filters;
> +                                       goto free_filtered_suite;
>
>                                 if (IS_ERR(filtered_suite)) {
>                                         *err = PTR_ERR(filtered_suite);
> -                                       goto free_parsed_filters;
> +                                       goto free_filtered_suite;
>                                 }
>                                 if (!filtered_suite)
>                                         break;
> @@ -231,6 +232,14 @@ kunit_filter_suites(const struct kunit_suite_set *suite_set,
>         filtered.start = copy_start;
>         filtered.end = copy;
>
> +free_filtered_suite:
> +       if (*err) {
> +               for (suites = copy_start; suites < copy; suites++) {
> +                       kfree((*suites)->test_cases);
> +                       kfree(*suites);
> +               }
> +       }
> +
>  free_parsed_filters:
>         if (filter_count)
>                 kfree(parsed_filters);
> --
> 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/20230922071020.2554677-4-ruanjinjie%40huawei.com.

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4003 bytes --]