On Thu, 21 Sept 2023 at 09:41, '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, though I admit that this code is starting to get a bit too complicated...
A few thoughts below, but they're more notes-to-self for a future refactoring than something I think this patch needs.
Reviewed-by: David Gow davidgow@google.com
-- 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;
Do we really need both filtered.start and copy_start, and filtered.end / copy? The only case where they're different would be when an error occurs, and it should be easy to simply reset them to NULL then, anyway.
+free_filtered_suite:
if (*err) {for (suites = copy_start; suites < copy; suites++) {kfree((*suites)->test_cases);kfree(*suites);}
We possibly should set filtered = {NULL, NULL} here. It's not actually possible for them to be non-NULL at this point, so it is redundant, but it's not easy to tell (and it looks like this could be returning a freed pointer here, even though it's not).
}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/20230921014008.3887257-4-ruanjin....