| Message ID | 20230914114629.1517650-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, Sep 14, 2023 at 7:47 AM '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> Hello! This looks good to me. I just have one comment below. Reviewed-by: Rae Moar <rmoar@google.com> 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. > 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/20230914114629.1517650-4-ruanjinjie%40huawei.com.
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@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> > > Hello! > > This looks good to me. I just have one comment below. > > Reviewed-by: Rae Moar <rmoar@google.com> > > 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@googlegroups.com. >> To view this discussion on the web visit https://groups.google.com/d/msgid/kunit-dev/20230914114629.1517650-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> --- lib/kunit/executor.c | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-)