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

From: Rae Moar <rmoar@google.com>
To: Jinjie Ruan <ruanjinjie@huawei.com>
Cc: brendan.higgins@linux.dev, davidgow@google.com,
	skhan@linuxfoundation.org, jk@codeconstruct.com.au,
	dlatypov@google.com, linux-kselftest@vger.kernel.org,
	kunit-dev@googlegroups.com
Subject: Re: [PATCH 3/4] kunit: Fix possible memory leak in kunit_filter_suites()
Date: Thu, 31 Aug 2023 17:01:13 -0400	[thread overview]
Message-ID: <CA+GJov5hZ_ZHZbDHXhSwMAhjos2u-sjY6J6MkWmq4kLtg9Vzjg@mail.gmail.com> (raw)
In-Reply-To: <20230831071655.2907683-4-ruanjinjie@huawei.com>

On Thu, Aug 31, 2023 at 3:17 AM 'Jinjie Ruan' via KUnit Development
<kunit-dev@googlegroups.com> wrote:
>
> If both filter_glob and filters are not NULL, and kunit_parse_glob_filter()
> succeed, but kcalloc parsed_filters fails, the suite_glob and test_glob of
> parsed kzalloc in kunit_parse_glob_filter() will be leaked.
>
> Fixes: 1c9fd080dffe ("kunit: fix uninitialized variables bug in attributes filtering")
> Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com>

Hello!

Thanks for catching this! I just had two concerns listed below.

-Rae

> ---
>  lib/kunit/executor.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/lib/kunit/executor.c b/lib/kunit/executor.c
> index 25ce8d6e5fe7..7654c09c1ab1 100644
> --- a/lib/kunit/executor.c
> +++ b/lib/kunit/executor.c
> @@ -176,10 +176,8 @@ kunit_filter_suites(const struct kunit_suite_set *suite_set,
>         if (filters) {
>                 filter_count = kunit_get_filter_count(filters);
>                 parsed_filters = kcalloc(filter_count, sizeof(*parsed_filters), GFP_KERNEL);
> -               if (!parsed_filters) {
> -                       kfree(copy);
> -                       return filtered;
> -               }
> +               if (!parsed_filters)
> +                       goto err;

When this goes to the "err" label this will likely enter the if
(filter_count) statement and attempt to free parsed_filters. Since
parsed_filters is NULL in this case, it could be a cleaner practice to
set a new label, maybe "filter_err", to skip over that if statement.
Although, I realize this may require the order of the patches to
switch.

Additionally, the value of *err should definitely be set here before
"goto err". The kunit_run_all_tests() function checks this err value
in order to produce the correct error message and to prevent any
attempt to run tests. I would suggest: *err = -ENOMEM;

>                 for (j = 0; j < filter_count; j++)
>                         parsed_filters[j] = kunit_next_attr_filter(&filters, err);
>                 if (*err)
> --
> 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/20230831071655.2907683-4-ruanjinjie%40huawei.com.