| Message ID | 20230921014008.3887257-4-ruanjinjie@huawei.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 24de14c98b37ea40a7e493dfd0d93b400b6efbca |
| Delegated to: | Brendan Higgins |
| Headers | show |
| Series | kunit: Fix some bugs in kunit | expand |
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-ruanjinjie%40huawei.com.
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); + } + } + free_parsed_filters: if (filter_count) kfree(parsed_filters);
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. --- lib/kunit/executor.c | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-)