Re: [PATCH 4/4] kunit: Fix the wrong error path in kunit_filter_suites()

From: David Gow <davidgow@google.com>
To: Jinjie Ruan <ruanjinjie@huawei.com>
Cc: brendan.higgins@linux.dev, skhan@linuxfoundation.org,
	jk@codeconstruct.com.au, dlatypov@google.com, rmoar@google.com,
	linux-kselftest@vger.kernel.org, kunit-dev@googlegroups.com
Subject: Re: [PATCH 4/4] kunit: Fix the wrong error path in kunit_filter_suites()
Date: Fri, 1 Sep 2023 13:18:13 +0800	[thread overview]
Message-ID: <CABVgOSk5yOxMoaEoi-GYiwZpHRs7ytTKwWF=CT7vBhOJrBaYcw@mail.gmail.com> (raw)
In-Reply-To: <20230831071655.2907683-5-ruanjinjie@huawei.com>

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

On Thu, 31 Aug 2023 at 15:17, 'Jinjie Ruan' via KUnit Development
<kunit-dev@googlegroups.com> wrote:
>
> Take the last kfree(parsed_filters) and add it to be the first. Take
> the first kfree(copy) and add it to be the last. The Best practice is to
> return these errors reversely.
>
> Fixes: 529534e8cba3 ("kunit: Add ability to filter attributes")
> Fixes: abbf73816b6f ("kunit: fix possible memory leak in kunit_filter_suites()")
> Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com>
> ---

Agreed, these should be freed in reverse order.

Would it make sense to initialise 'copy' to NULL, and free it if (copy
!= NULL), rather than if (*err)? As mentioned in the previous patch, I
think that'd be more correct.

We could also have several labels which target only the things which
actually have been allocated so far.

Thoughts?
-- David

>  lib/kunit/executor.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/lib/kunit/executor.c b/lib/kunit/executor.c
> index 7654c09c1ab1..da9444d22711 100644
> --- a/lib/kunit/executor.c
> +++ b/lib/kunit/executor.c
> @@ -229,16 +229,16 @@ kunit_filter_suites(const struct kunit_suite_set *suite_set,
>         filtered.end = copy;
>
>  err:
> -       if (*err)
> -               kfree(copy);
> +       if (filter_count)
> +               kfree(parsed_filters);
>
>         if (filter_glob) {
>                 kfree(parsed_glob.suite_glob);
>                 kfree(parsed_glob.test_glob);
>         }

I think this might also be potentially broken. If
parsed_glob.{suite,test}_glob are not successfully allocated,
filter_glob will still be set, and we'll kfree() something invalid.
Should we also init parsed_glob.* to NULL, and free them if non-NULL,
rather than relying on the presence of filter_glob?

>
> -       if (filter_count)
> -               kfree(parsed_filters);
> +       if (*err)
> +               kfree(copy);
>
>         return filtered;
>  }
> --
> 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-5-ruanjinjie%40huawei.com.

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