| Message ID | 20230831071655.2907683-5-ruanjinjie@huawei.com (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Brendan Higgins |
| Headers | show |
| Series | kunit: Fix some bugs in kunit_filter_suites() | expand |
On Thu, Aug 31, 2023 at 3:17 AM 'Jinjie Ruan' via KUnit Development <kunit-dev@googlegroups.com> wrote: > > Take the last kfree(parsed_filters) and add it to be the first. Take > the first kfree(copy) and add it to be the last. The Best practice is to > return these errors reversely. > > Fixes: 529534e8cba3 ("kunit: Add ability to filter attributes") > Fixes: abbf73816b6f ("kunit: fix possible memory leak in kunit_filter_suites()") > Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com> Hello! This seems fine to me. See my comments from the last patch regarding a potential change in the order of the patches. Otherwise, this works for me. Reviewed-by: Rae Moar <rmoar@google.com> Thanks! -Rae > --- > lib/kunit/executor.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/lib/kunit/executor.c b/lib/kunit/executor.c > index 7654c09c1ab1..da9444d22711 100644 > --- a/lib/kunit/executor.c > +++ b/lib/kunit/executor.c > @@ -229,16 +229,16 @@ kunit_filter_suites(const struct kunit_suite_set *suite_set, > filtered.end = copy; > > err: > - if (*err) > - kfree(copy); > + if (filter_count) > + kfree(parsed_filters); > > if (filter_glob) { > kfree(parsed_glob.suite_glob); > kfree(parsed_glob.test_glob); > } > > - if (filter_count) > - kfree(parsed_filters); > + if (*err) > + kfree(copy); > > return filtered; > } > -- > 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/20230831071655.2907683-5-ruanjinjie%40huawei.com.
On Thu, 31 Aug 2023 at 15:17, 'Jinjie Ruan' via KUnit Development <kunit-dev@googlegroups.com> wrote: > > Take the last kfree(parsed_filters) and add it to be the first. Take > the first kfree(copy) and add it to be the last. The Best practice is to > return these errors reversely. > > Fixes: 529534e8cba3 ("kunit: Add ability to filter attributes") > Fixes: abbf73816b6f ("kunit: fix possible memory leak in kunit_filter_suites()") > Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com> > --- Agreed, these should be freed in reverse order. Would it make sense to initialise 'copy' to NULL, and free it if (copy != NULL), rather than if (*err)? As mentioned in the previous patch, I think that'd be more correct. We could also have several labels which target only the things which actually have been allocated so far. Thoughts? -- David > lib/kunit/executor.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/lib/kunit/executor.c b/lib/kunit/executor.c > index 7654c09c1ab1..da9444d22711 100644 > --- a/lib/kunit/executor.c > +++ b/lib/kunit/executor.c > @@ -229,16 +229,16 @@ kunit_filter_suites(const struct kunit_suite_set *suite_set, > filtered.end = copy; > > err: > - if (*err) > - kfree(copy); > + if (filter_count) > + kfree(parsed_filters); > > if (filter_glob) { > kfree(parsed_glob.suite_glob); > kfree(parsed_glob.test_glob); > } I think this might also be potentially broken. If parsed_glob.{suite,test}_glob are not successfully allocated, filter_glob will still be set, and we'll kfree() something invalid. Should we also init parsed_glob.* to NULL, and free them if non-NULL, rather than relying on the presence of filter_glob? > > - if (filter_count) > - kfree(parsed_filters); > + if (*err) > + kfree(copy); > > return filtered; > } > -- > 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/20230831071655.2907683-5-ruanjinjie%40huawei.com.
On 2023/9/1 13:18, David Gow wrote: > On Thu, 31 Aug 2023 at 15:17, 'Jinjie Ruan' via KUnit Development > <kunit-dev@googlegroups.com> wrote: >> >> Take the last kfree(parsed_filters) and add it to be the first. Take >> the first kfree(copy) and add it to be the last. The Best practice is to >> return these errors reversely. >> >> Fixes: 529534e8cba3 ("kunit: Add ability to filter attributes") >> Fixes: abbf73816b6f ("kunit: fix possible memory leak in kunit_filter_suites()") >> Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com> >> --- > > Agreed, these should be freed in reverse order. > > Would it make sense to initialise 'copy' to NULL, and free it if (copy > != NULL), rather than if (*err)? As mentioned in the previous patch, I > think that'd be more correct. There is a problem because the normal return also go through the all err paths but 'copy' is not NULL and should not be freed. > > We could also have several labels which target only the things which > actually have been allocated so far. That's a good idea! > > Thoughts? > -- David > >> lib/kunit/executor.c | 8 ++++---- >> 1 file changed, 4 insertions(+), 4 deletions(-) >> >> diff --git a/lib/kunit/executor.c b/lib/kunit/executor.c >> index 7654c09c1ab1..da9444d22711 100644 >> --- a/lib/kunit/executor.c >> +++ b/lib/kunit/executor.c >> @@ -229,16 +229,16 @@ kunit_filter_suites(const struct kunit_suite_set *suite_set, >> filtered.end = copy; >> >> err: >> - if (*err) >> - kfree(copy); >> + if (filter_count) >> + kfree(parsed_filters); >> >> if (filter_glob) { >> kfree(parsed_glob.suite_glob); >> kfree(parsed_glob.test_glob); >> } > > I think this might also be potentially broken. If > parsed_glob.{suite,test}_glob are not successfully allocated, > filter_glob will still be set, and we'll kfree() something invalid. > Should we also init parsed_glob.* to NULL, and free them if non-NULL, > rather than relying on the presence of filter_glob? if not successsfully allocated, it will be NULL and kfree NULL is ok. > > > >> >> - if (filter_count) >> - kfree(parsed_filters); >> + if (*err) >> + kfree(copy); >> >> return filtered; >> } >> -- >> 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/20230831071655.2907683-5-ruanjinjie%40huawei.com.
diff --git a/lib/kunit/executor.c b/lib/kunit/executor.c index 7654c09c1ab1..da9444d22711 100644 --- a/lib/kunit/executor.c +++ b/lib/kunit/executor.c @@ -229,16 +229,16 @@ kunit_filter_suites(const struct kunit_suite_set *suite_set, filtered.end = copy; err: - if (*err) - kfree(copy); + if (filter_count) + kfree(parsed_filters); if (filter_glob) { kfree(parsed_glob.suite_glob); kfree(parsed_glob.test_glob); } - if (filter_count) - kfree(parsed_filters); + if (*err) + kfree(copy); return filtered; }
Take the last kfree(parsed_filters) and add it to be the first. Take the first kfree(copy) and add it to be the last. The Best practice is to return these errors reversely. Fixes: 529534e8cba3 ("kunit: Add ability to filter attributes") Fixes: abbf73816b6f ("kunit: fix possible memory leak in kunit_filter_suites()") Signed-off-by: Jinjie Ruan <ruanjinjie@huawei.com> --- lib/kunit/executor.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)