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-ruanjin....