On Thu, 31 Aug 2023 at 15:17, 'Jinjie Ruan' via KUnit Development
<kunit-dev@xxxxxxxxxxxxxxxx> 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@xxxxxxxxxx>
> ---
As Rae points out, I don't think this is correct. If filter_count is
non-zero, but parsed_filters is NULL due to an allocation failure,
we'll still end up trying to kfree() a NULL pointer.
That's why this didn't goto err; before: we explicitly _don't_ want to
free parsed_filters here. We could set filter_count = 0 before goto
err, but I think either wrapping the kfree() call in if
(parsed_filters), or
Indeed, this may be the case for most of the cleanup here: we're
checking if we intended to allocate something (e.g., the filter_count
is nonzero, the filter_glob is set), rather than whether we
successfully allocated something, so can kfree(NULL) on failure. The
issues with ordering (that you fix in the next patch) could help if we
had several labels to jump to after each allocation, or just checking
that what we're freeing was non-NULL (and initialising them to NULL if
needed).
Thanks,
-- David
> 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;
> 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@xxxxxxxxxxxxxxxx.
> To view this discussion on the web visit https://groups.google.com/d/msgid/kunit-dev/20230831071655.2907683-4-ruanjinjie%40huawei.com.
Attachment:
smime.p7s
Description: S/MIME Cryptographic Signature
