On 2023/9/20 5:18, Rae Moar wrote:
> On Thu, Sep 14, 2023 at 7:47 AM 'Jinjie Ruan' via KUnit Development
> <kunit-dev@xxxxxxxxxxxxxxxx> 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@xxxxxxxxxx>
>
> Hello!
>
> This looks good to me. I just have one comment below.
>
> Reviewed-by: Rae Moar <rmoar@xxxxxxxxxx>
>
> Thanks!
> -Rae
>
>> ---
>> 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);
>> + }
>> + }
>> +
>
> As this is pretty similar code to kunit_free_suite_set, I wish you
> could use that method instead but I'm not actually sure it would be
> cleaner.
There is a slight difference between here and kunit_free_suite_set(), it
do not kfree(suite_set.start) which is kfree(copy_start) here as it is
the first kcalloc.
>
>
>> 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@xxxxxxxxxxxxxxxx.
>> To view this discussion on the web visit https://groups.google.com/d/msgid/kunit-dev/20230914114629.1517650-4-ruanjinjie%40huawei.com.
>
